chore: fix wds select widget bugs + refactor #38304
Conversation
WalkthroughThe pull request involves a comprehensive refactoring of the Select and ComboBox widgets in the UI builder module. The changes primarily focus on restructuring the widget configurations, updating validation logic, and modifying how options are handled. The modifications include removing several configuration files, introducing new validation functions, and updating the widget's inheritance structure to leverage a more streamlined approach to widget management. Changes
Assessment against linked issues
Possibly related PRs
Suggested labels
Suggested reviewers
Poem
📜 Recent review detailsConfiguration used: .coderabbit.yaml 📒 Files selected for processing (1)
💤 Files with no reviewable changes (1)
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
Anvil Pod
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 4
🧹 Nitpick comments (14)
app/client/packages/design-system/widgets/src/components/Select/src/styles.module.css (1)
26-29: Consider using design tokens for placeholder opacityWhile using
--color-fg-neutral-subtleis good for consistency, consider if this should be a specific design token for placeholder text to maintain semantic meaning across different form elements..selectTriggerButton [data-select-text][data-placeholder] { - color: var(--color-fg-neutral-subtle); + color: var(--placeholder-text-color, var(--color-fg-neutral-subtle)); }app/client/src/modules/ui-builder/ui/wds/WDSSelectWidget/config/settersConfig.ts (1)
19-23: Consider validating defaultOptionValue.
If there are constraints on valid option values, you could add checks to prevent invalid assignments.app/client/src/modules/ui-builder/ui/wds/WDSSelectWidget/widget/helpers.ts (3)
1-5: Review Imports for Efficiency
Imports from lodash look appropriate for the tasks at hand. If performance or bundle size is a concern, consider using per-method imports (e.g., importing directly from "lodash/get") to reduce potential code bloat.
Line range hint
12-25: Internationalization Consideration
Returning the hard-coded string "Please select an option" might be fine for now, but consider externalizing this string or using an i18n library if localization matters.
70-78: Maintain Consistent Expression Conventions
The generated expression strings are correct. For consistency, consider storing the "map" callback or property references in constants to reduce duplication and make future modifications simpler.app/client/src/modules/ui-builder/ui/wds/WDSSelectWidget/config/autocompleteConfig.ts (3)
8-11: Clarify filterText usage
Consider whether additional documentation is needed to describe how server-side filtering is triggered and handled.
12-16: Describe valid formats or length constraints
For “selectedOptionValue”, provide any applicable constraints or formats (e.g., string length) if they exist to tighten validation.
17-21: Check naming consistency
“selectedOptionLabel” is informative, but ensure naming patterns remain consistent with “selectedOptionValue” and any other related config to avoid confusion.app/client/src/modules/ui-builder/ui/wds/WDSSelectWidget/config/propertyPaneConfig/validations/defaultOptionValueValidation.ts (2)
16-26: Helper function clarity
The helper function “hasLabelValue” is concise, but consider explicitly handling type checks for the .label and .value properties if deeper validation is needed.🧰 Tools
🪛 Biome (1.9.4)
[error] 21-21: 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] 22-22: 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)
31-39: Parsing from JSON
Catching exceptions silently is acceptable here, but if debugging is needed, consider logging parse errors in development builds.app/client/src/modules/ui-builder/ui/wds/WDSSelectWidget/config/methodsConfig.ts (1)
32-62: Comprehensive property updates
“getPropertyUpdatesForQueryBinding” effectively merges new dynamic properties with existing ones. Ensure no collisions if a large set of properties is being handled.app/client/src/modules/ui-builder/ui/wds/WDSSelectWidget/config/propertyPaneConfig/validations/valueKeyValidation.ts (1)
109-124: Check for partial duplication or case-sensitivity
Currently, the uniqueness check ensures there are no exact duplicate options. If you expect more nuanced checks (e.g., case-insensitive or partial matches), you can enhance this logic or highlight that advanced checks aren’t supported out of the box.app/client/src/modules/ui-builder/ui/wds/WDSSelectWidget/widget/derived.js (2)
11-13: Guard against undefined or empty sourceData
While you return early if sourceData is empty at line 9, consider additional safety checks around sourceData[0], to avoid runtime errors if this code is modified in the future.
60-64: Confirm necessity of the moment parameter
The function getIsValid doesn’t appear to utilize the moment parameter. If it truly isn’t needed, removing it could simplify the code.
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (30)
app/client/packages/design-system/widgets/src/components/Select/src/styles.module.css(1 hunks)app/client/src/modules/ui-builder/ui/wds/WDSComboBoxWidget/config/anvilConfig.ts(0 hunks)app/client/src/modules/ui-builder/ui/wds/WDSComboBoxWidget/config/autocompleteConfig.ts(0 hunks)app/client/src/modules/ui-builder/ui/wds/WDSComboBoxWidget/config/defaultsConfig.ts(0 hunks)app/client/src/modules/ui-builder/ui/wds/WDSComboBoxWidget/config/index.ts(0 hunks)app/client/src/modules/ui-builder/ui/wds/WDSComboBoxWidget/config/metaConfig.ts(0 hunks)app/client/src/modules/ui-builder/ui/wds/WDSComboBoxWidget/config/methodsConfig.ts(0 hunks)app/client/src/modules/ui-builder/ui/wds/WDSComboBoxWidget/config/propertyPaneConfig/contentConfig.test.ts(0 hunks)app/client/src/modules/ui-builder/ui/wds/WDSComboBoxWidget/config/propertyPaneConfig/contentConfig.ts(0 hunks)app/client/src/modules/ui-builder/ui/wds/WDSComboBoxWidget/config/propertyPaneConfig/index.ts(0 hunks)app/client/src/modules/ui-builder/ui/wds/WDSComboBoxWidget/config/propertyPaneConfig/validations/index.ts(0 hunks)app/client/src/modules/ui-builder/ui/wds/WDSComboBoxWidget/config/propertyPaneConfig/validations/optionsCustomValidation.ts(0 hunks)app/client/src/modules/ui-builder/ui/wds/WDSComboBoxWidget/config/settersConfig.ts(0 hunks)app/client/src/modules/ui-builder/ui/wds/WDSComboBoxWidget/widget/helpers.ts(0 hunks)app/client/src/modules/ui-builder/ui/wds/WDSComboBoxWidget/widget/index.tsx(1 hunks)app/client/src/modules/ui-builder/ui/wds/WDSComboBoxWidget/widget/types.ts(0 hunks)app/client/src/modules/ui-builder/ui/wds/WDSSelectWidget/config/autocompleteConfig.ts(1 hunks)app/client/src/modules/ui-builder/ui/wds/WDSSelectWidget/config/defaultsConfig.ts(2 hunks)app/client/src/modules/ui-builder/ui/wds/WDSSelectWidget/config/methodsConfig.ts(1 hunks)app/client/src/modules/ui-builder/ui/wds/WDSSelectWidget/config/propertyPaneConfig/contentConfig.tsx(2 hunks)app/client/src/modules/ui-builder/ui/wds/WDSSelectWidget/config/propertyPaneConfig/validations/defaultOptionValidation.ts(0 hunks)app/client/src/modules/ui-builder/ui/wds/WDSSelectWidget/config/propertyPaneConfig/validations/defaultOptionValueValidation.ts(1 hunks)app/client/src/modules/ui-builder/ui/wds/WDSSelectWidget/config/propertyPaneConfig/validations/index.ts(1 hunks)app/client/src/modules/ui-builder/ui/wds/WDSSelectWidget/config/propertyPaneConfig/validations/labelKeyValidation.ts(1 hunks)app/client/src/modules/ui-builder/ui/wds/WDSSelectWidget/config/propertyPaneConfig/validations/optionsCustomValidation.ts(0 hunks)app/client/src/modules/ui-builder/ui/wds/WDSSelectWidget/config/propertyPaneConfig/validations/valueKeyValidation.ts(1 hunks)app/client/src/modules/ui-builder/ui/wds/WDSSelectWidget/config/settersConfig.ts(1 hunks)app/client/src/modules/ui-builder/ui/wds/WDSSelectWidget/widget/derived.js(1 hunks)app/client/src/modules/ui-builder/ui/wds/WDSSelectWidget/widget/helpers.ts(2 hunks)app/client/src/modules/ui-builder/ui/wds/WDSSelectWidget/widget/index.tsx(7 hunks)
💤 Files with no reviewable changes (16)
- app/client/src/modules/ui-builder/ui/wds/WDSComboBoxWidget/config/propertyPaneConfig/index.ts
- app/client/src/modules/ui-builder/ui/wds/WDSComboBoxWidget/config/propertyPaneConfig/validations/index.ts
- app/client/src/modules/ui-builder/ui/wds/WDSComboBoxWidget/widget/types.ts
- app/client/src/modules/ui-builder/ui/wds/WDSComboBoxWidget/widget/helpers.ts
- app/client/src/modules/ui-builder/ui/wds/WDSComboBoxWidget/config/autocompleteConfig.ts
- app/client/src/modules/ui-builder/ui/wds/WDSComboBoxWidget/config/defaultsConfig.ts
- app/client/src/modules/ui-builder/ui/wds/WDSComboBoxWidget/config/settersConfig.ts
- app/client/src/modules/ui-builder/ui/wds/WDSComboBoxWidget/config/metaConfig.ts
- app/client/src/modules/ui-builder/ui/wds/WDSComboBoxWidget/config/propertyPaneConfig/contentConfig.test.ts
- app/client/src/modules/ui-builder/ui/wds/WDSComboBoxWidget/config/anvilConfig.ts
- app/client/src/modules/ui-builder/ui/wds/WDSComboBoxWidget/config/index.ts
- app/client/src/modules/ui-builder/ui/wds/WDSComboBoxWidget/config/propertyPaneConfig/contentConfig.ts
- app/client/src/modules/ui-builder/ui/wds/WDSComboBoxWidget/config/propertyPaneConfig/validations/optionsCustomValidation.ts
- app/client/src/modules/ui-builder/ui/wds/WDSSelectWidget/config/propertyPaneConfig/validations/optionsCustomValidation.ts
- app/client/src/modules/ui-builder/ui/wds/WDSComboBoxWidget/config/methodsConfig.ts
- app/client/src/modules/ui-builder/ui/wds/WDSSelectWidget/config/propertyPaneConfig/validations/defaultOptionValidation.ts
🧰 Additional context used
🪛 Biome (1.9.4)
app/client/src/modules/ui-builder/ui/wds/WDSSelectWidget/config/propertyPaneConfig/validations/defaultOptionValueValidation.ts
[error] 21-21: 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] 22-22: 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/WDSSelectWidget/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)
app/client/src/modules/ui-builder/ui/wds/WDSComboBoxWidget/widget/index.tsx
[error] 14-14: Using super in a static context can be confusing.
super refers to a parent class.
Unsafe fix: Use the class name instead.
(lint/complexity/noThisInStatic)
[error] 21-21: Using super in a static context can be confusing.
super refers to a parent class.
Unsafe fix: Use the class name instead.
(lint/complexity/noThisInStatic)
🔇 Additional comments (44)
app/client/packages/design-system/widgets/src/components/Select/src/styles.module.css (1)
Line range hint 1-25: LGTM! Well-documented padding adjustments
The padding reorganization from .selectInputGroup to .selectTriggerButton is a good solution for ensuring width consistency between the popover and trigger button. The code comments effectively explain the reasoning.
app/client/src/modules/ui-builder/ui/wds/WDSSelectWidget/config/settersConfig.ts (2)
11-14: Implementation of setRequired looks good.
Straightforward approach and aligns with existing structure for boolean fields.
15-15: setOptions remains consistent.
No issues identified with this setter's definition or usage.
app/client/src/modules/ui-builder/ui/wds/WDSSelectWidget/widget/helpers.ts (1)
56-68: Improve Type Safety
The function treats each unique label as an empty string. Ensure that any future usage of these autocomplete items can handle the string value appropriately or consider storing more relevant default values.
app/client/src/modules/ui-builder/ui/wds/WDSComboBoxWidget/widget/index.tsx (5)
2-3: Imports look good.
All required modules seem appropriately referenced.
6-7: Widget inheritance is correct.
Switching from extending BaseWidget to extending WDSSelectWidget is consistent with the PR's refactoring goals.
28-29: Forcing a re-render is well-handled.
Using a 'key' derived from option values to avoid react-aria errors is a neat approach.
Also applies to: 31-38
46-47: Consistent event handling.
Binding onSelectionChange to this.handleChange aligns with the superclass logic.
51-56: List rendering is straightforward.
Mapping over each option and specifying textValue improves accessibility.
app/client/src/modules/ui-builder/ui/wds/WDSSelectWidget/widget/index.tsx (9)
5-5: New Key type import.
Using the Key type is a clean way to clarify handleChange parameters.
23-25: Derived property imports.
Bringing in parseDerivedProperties and derivedPropertyFns helps avoid duplication.
52-53: Dependency map changes.
Referencing "sourceData" instead of "options" is consistent with the new property structure.
70-76: Dynamic derived properties.
parseDerivedProperties is a robust way to define property logic.
97-97: Resetting isDirty on default value change.
This logic ensures consistency when the default option changes.
111-111: Handle null selections gracefully.
The early return for null updatedValue safely avoids unnecessary updates.
Also applies to: 114-115
145-155: Destructuring approach for widget props.
Using placeholderText and validation results keeps the rendering flow straightforward.
163-163: Key prop usage.
Forcing a re-render is a good workaround for the known react-aria limitation.
168-173: Option mapping.
Using id and key with the same option value is valid here.
app/client/src/modules/ui-builder/ui/wds/WDSSelectWidget/config/propertyPaneConfig/contentConfig.tsx (8)
1-1: React import is standard.
No concerns here.
6-6: Validation import.
Adding defaultOptionValueValidation is consistent with the new default option logic.
9-9: Value key validation.
Appropriate for user-defined property checks.
10-18: Helper imports.
These new helpers streamline user input expressions and label-value configurations.
75-118: sourceData property.
ONE_CLICK_BINDING_CONTROL for arrays of objects is a neat approach, ensuring data binding from APIs is easy.
119-150: optionLabel property.
Lets users specify how items are displayed in the widget. Good addition.
151-181: optionValue property.
Aligns with the label-value pattern and ensures correct uniqueness.
184-214: defaultOptionValue.
Tied to the new validation for better consistency.
app/client/src/modules/ui-builder/ui/wds/WDSSelectWidget/config/propertyPaneConfig/validations/index.ts (1)
1-1: Export of defaultOptionValueValidation.
Removing older unused validations reduces clutter.
app/client/src/modules/ui-builder/ui/wds/WDSSelectWidget/config/defaultsConfig.ts (4)
7-15: sourceData replaces options.
These changes align with the updated property structure and data approach.
16-17: optionLabel and optionValue defaults.
Simplifies usage by providing a sane default for label/value fields.
27-27: dynamicPropertyPathList usage.
Enables dynamic updates to sourceData.
28-28: Placeholder text.
Enhances clarity for end users.
app/client/src/modules/ui-builder/ui/wds/WDSSelectWidget/config/autocompleteConfig.ts (2)
5-6: Good doc improvement
The updated description clarifies the widget's purpose more precisely and aligns with the dropdown widget reference.
22-24: Verify boolean usage
“isDisabled”, “isValid”, and “isDirty” are marked as “bool”. Verify that these properties reflect booleans throughout the code for consistency.
app/client/src/modules/ui-builder/ui/wds/WDSSelectWidget/config/propertyPaneConfig/validations/labelKeyValidation.ts (5)
1-4: Type imports look good
Imports ensure type safety, making the validation function more robust.
16-27: Validate handling of empty string
You’re treating an empty string as invalid. If users can enter an empty label, confirm whether this is intentional.
29-35: String-only path
Logic for string values is clear. Good job returning a valid parsed value.
36-51: Array validation approach
The method neatly distinguishes valid and invalid array elements. This is straightforward and efficient.
52-63: Fallback case
Returning a default object for non-string/array values is a proper fallback.
app/client/src/modules/ui-builder/ui/wds/WDSSelectWidget/config/propertyPaneConfig/validations/defaultOptionValueValidation.ts (2)
41-55: Sensible checks
Allowing strings, numbers, or {label, value} objects ensures flexibility. The error message for invalid types is clear.
57-62: Structured return
Returning a ValidationResponse with messages is a good pattern for surfacing user-facing error details.
app/client/src/modules/ui-builder/ui/wds/WDSSelectWidget/config/methodsConfig.ts (2)
19-21: Ensure proper initialization
Switching from “options” to “sourceData” is sensible for a more generalized naming. Just make sure “sourceData” is correctly initialized before usage.
25-31: Query config retrieval
“getQueryGenerationConfig” nicely isolates the config logic. Keep it consistent with the overall data model.
app/client/src/modules/ui-builder/ui/wds/WDSSelectWidget/config/propertyPaneConfig/validations/valueKeyValidation.ts (1)
16-26: Ensure consistent empty-value handling
If the value is empty or nullish, it's marked invalid. This logic is fine, though consider whether you'll need to unify it with other scenarios (such as arrays) for consistent UX and to avoid confusion around what “empty” means in different contexts.
app/client/src/modules/ui-builder/ui/wds/WDSSelectWidget/widget/derived.js (1)
66-88: Validate server-side filtering logic
The logic for resetting the option value when serverSideFiltering is false or when props.value is not a plain object appears correct. Ensure you have test coverage for these edge cases, such as transitioning from serverSideFiltering to a non-filtered environment, to confirm no unintentional resets occur.
app/client/src/modules/ui-builder/ui/wds/WDSSelectWidget/widget/helpers.ts
Show resolved
Hide resolved
app/client/src/modules/ui-builder/ui/wds/WDSComboBoxWidget/widget/index.tsx
Show resolved
Hide resolved
...ui/wds/WDSSelectWidget/config/propertyPaneConfig/validations/defaultOptionValueValidation.ts
Show resolved
Hide resolved
...i-builder/ui/wds/WDSSelectWidget/config/propertyPaneConfig/validations/valueKeyValidation.ts
Show resolved
Hide resolved
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (2)
app/client/src/modules/ui-builder/ui/wds/WDSComboBoxWidget/widget/index.tsx (1)
42-45: Consider extracting key generation logicThe key generation logic could be moved to a separate method for better maintainability.
+ private getOptionsKey(options: Array<{ value: string; label: string }>) { + return options.map((option) => option.value).join(","); + } getWidgetView() { const { labelTooltip, placeholderText, selectedOptionValue, ...rest } = this.props; const validation = validateInput(this.props); const options = (isArray(this.props.options) ? this.props.options : []) as { value: string; label: string; }[]; - const key = options.map((option) => option.value).join(","); + const key = this.getOptionsKey(options);app/client/src/modules/ui-builder/ui/wds/WDSSelectWidget/config/propertyPaneConfig/contentConfig.tsx (1)
208-214: Consider improving helper text formattingThe helper text could be more visually appealing with proper styling.
- helperText: ( - <div style={{ marginTop: "10px" }}> - Make sure the default value is present in the source data to have it - selected by default in the UI. - </div> - ), + helperText: ( + <div className="helper-text"> + Make sure the default value is present in the source data to have it + selected by default in the UI. + </div> + ),
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
app/client/src/modules/ui-builder/ui/wds/WDSComboBoxWidget/widget/index.tsx(1 hunks)app/client/src/modules/ui-builder/ui/wds/WDSSelectWidget/config/autocompleteConfig.ts(1 hunks)app/client/src/modules/ui-builder/ui/wds/WDSSelectWidget/config/methodsConfig.ts(1 hunks)app/client/src/modules/ui-builder/ui/wds/WDSSelectWidget/config/propertyPaneConfig/contentConfig.tsx(2 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- app/client/src/modules/ui-builder/ui/wds/WDSSelectWidget/config/methodsConfig.ts
🧰 Additional context used
🪛 Biome (1.9.4)
app/client/src/modules/ui-builder/ui/wds/WDSComboBoxWidget/widget/index.tsx
[error] 14-14: Using super in a static context can be confusing.
super refers to a parent class.
Unsafe fix: Use the class name instead.
(lint/complexity/noThisInStatic)
[error] 21-21: Using super in a static context can be confusing.
super refers to a parent class.
Unsafe fix: Use the class name instead.
(lint/complexity/noThisInStatic)
[error] 28-28: Using super in a static context can be confusing.
super refers to a parent class.
Unsafe fix: Use the class name instead.
(lint/complexity/noThisInStatic)
🔇 Additional comments (3)
app/client/src/modules/ui-builder/ui/wds/WDSSelectWidget/config/autocompleteConfig.ts (1)
5-20: LGTM! Configuration updates enhance widget functionality
The new properties and updated documentation improve the widget's capabilities while maintaining clear documentation.
app/client/src/modules/ui-builder/ui/wds/WDSComboBoxWidget/widget/index.tsx (1)
14-15: Replace 'super' with class name in static methods
Using 'super' in static context can be confusing. Use the class name directly.
Also applies to: 21-23, 28-31
🧰 Tools
🪛 Biome (1.9.4)
[error] 14-14: Using super in a static context can be confusing.
super refers to a parent class.
Unsafe fix: Use the class name instead.
(lint/complexity/noThisInStatic)
app/client/src/modules/ui-builder/ui/wds/WDSSelectWidget/config/propertyPaneConfig/contentConfig.tsx (1)
75-116: LGTM! Improved data binding configuration
The renaming of 'options' to 'sourceData' and the enhanced validation configuration improve clarity and robustness.
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (3)
app/client/src/modules/ui-builder/ui/wds/WDSSelectWidget/config/defaultsConfig.ts (1)
9-10: Consider adding JSDoc comments for option bindingThe optionLabel and optionValue keys would benefit from documentation explaining their expected data structure.
+/** Key in sourceData that provides the display text */ optionLabel: "name", +/** Key in sourceData that provides the value */ optionValue: "code",app/client/src/modules/ui-builder/ui/wds/WDSSelectWidget/config/propertyPaneConfig/contentConfig.tsx (2)
113-144: Consider enhancing type safety for optionLabelThe implementation looks good, but consider adding a type guard in the validation function to ensure type safety at runtime.
validation: { type: ValidationTypes.FUNCTION, params: { fn: labelKeyValidation, expected: { type: "String or Array<string>", + typeGuard: (value: unknown): value is string | string[] => { + return typeof value === "string" || + (Array.isArray(value) && value.every(v => typeof v === "string")); + }, example: `color | ["blue", "green"]`, autocompleteDataType: AutocompleteDataType.STRING, }, }, },
165-174: Add array bounds validation for optionValueConsider adding a maximum array length validation to prevent performance issues with large arrays.
validation: { type: ValidationTypes.FUNCTION, params: { fn: valueKeyValidation, expected: { type: "String or Array<string | number | boolean>", + maxArrayLength: 1000, example: `color | [1, "orange"]`, autocompleteDataType: AutocompleteDataType.STRING, }, }, },
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
app/client/src/modules/ui-builder/ui/wds/WDSSelectWidget/config/defaultsConfig.ts(2 hunks)app/client/src/modules/ui-builder/ui/wds/WDSSelectWidget/config/propertyPaneConfig/contentConfig.tsx(2 hunks)app/client/src/modules/ui-builder/ui/wds/WDSSelectWidget/widget/constants.ts(1 hunks)
✅ Files skipped from review due to trivial changes (1)
- app/client/src/modules/ui-builder/ui/wds/WDSSelectWidget/widget/constants.ts
🔇 Additional comments (5)
app/client/src/modules/ui-builder/ui/wds/WDSSelectWidget/config/defaultsConfig.ts (4)
3-3: LGTM: Clean import addition
20-20: LGTM: Proper dynamic property configuration
The dynamic property configuration correctly enables sourceData updates.
21-21: LGTM: Clear default placeholder text
8-8: Verify SAMPLE_DATA structure compatibility
The switch to dynamic sourceData looks good, but let's verify the structure.
✅ Verification successful
SAMPLE_DATA structure is compatible with select widget requirements
The SAMPLE_DATA structure in constants.ts contains objects with name and code fields, which aligns perfectly with the standard label/value pattern needed for select widgets.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Verify SAMPLE_DATA structure matches the new configuration
# Check if SAMPLE_DATA has the required 'name' and 'code' fields
ast-grep --pattern 'const SAMPLE_DATA = $_' ../widget/constants.ts
Length of output: 138
Script:
#!/bin/bash
# Search for SAMPLE_DATA constant in the repository
rg "const SAMPLE_DATA" -A 5
# Find all files related to WDSSelectWidget to locate the constants
fd "WDSSelectWidget"
Length of output: 1113
app/client/src/modules/ui-builder/ui/wds/WDSSelectWidget/config/propertyPaneConfig/contentConfig.tsx (1)
77-112: Verify backward compatibility for the options → sourceData rename
The property rename from "options" to "sourceData" looks good, but we should ensure existing implementations won't break.
✅ Verification successful
Property rename from "options" to "sourceData" is safe to proceed
The interface WDSSelectWidgetProps still maintains the options property, and the widget internally derives the options from sourceData through its derived properties map. This ensures backward compatibility while allowing the new property name in the UI.
Key findings:
- The widget's interface keeps the
optionsproperty - The widget derives
optionsfromsourceDataingetDerivedPropertiesMap - The change only affects the property pane configuration, not the widget's core functionality
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for existing usages of the old "options" property
rg -l "options.*WDSSelectWidget" --type ts --type tsx
Length of output: 83
Script:
#!/bin/bash
# Search for existing usages of the "options" property in TypeScript files
rg "options.*WDSSelectWidget" -t ts
# Also search in JavaScript files
rg "options.*WDSSelectWidget" -t js
# Search for any references to WDSSelectWidget to understand its usage
rg "WDSSelectWidget" -t ts -t js
# Look for test files related to WDSSelectWidget
fd "WDSSelectWidget.*test"
Length of output: 3165
Script:
#!/bin/bash
# Let's check the widget types and props interface to see the property definitions
ast-grep --pattern 'interface WDSSelectWidgetProps extends WidgetProps {
$$$
}'
# Check the actual implementation to see how options/sourceData is used
ast-grep --pattern 'class WDSSelectWidget extends BaseWidget<$_, $_> {
$$$
}'
# Look for any migration or upgrade related files
fd "migration|upgrade" --type f
Length of output: 30010
...nt/src/modules/ui-builder/ui/wds/WDSSelectWidget/config/propertyPaneConfig/contentConfig.tsx
Show resolved
Hide resolved
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (3)
app/client/src/modules/ui-builder/ui/wds/WDSComboBoxWidget/widget/index.tsx (1)
14-15:⚠️ Potential issueReplace 'super' with direct class reference in static methods
Using
superin static methods can be confusing and potentially problematic. Reference the parent class directly.static getConfig() { return { - ...super.getConfig(), + ...WDSSelectWidget.getConfig(), name: "ComboBox", }; } static getDefaults() { return { - ...super.getDefaults(), + ...WDSSelectWidget.getDefaults(), widgetName: "ComboBox", }; } static getMethods() { return { - ...super.getMethods(), + ...WDSSelectWidget.getMethods(), IconCmp: ComboboxSelectIcon, ThumbnailCmp: ComboboxSelectThumbnail, }; }Also applies to: 21-22, 28-30
🧰 Tools
🪛 Biome (1.9.4)
[error] 14-14: Using super in a static context can be confusing.
super refers to a parent class.
Unsafe fix: Use the class name instead.(lint/complexity/noThisInStatic)
app/client/src/modules/ui-builder/ui/wds/WDSSelectWidget/config/propertyPaneConfig/validations/defaultOptionValueValidation.ts (1)
16-27:⚠️ Potential issueUse Object.hasOwn()
Replace direct hasOwnProperty calls with Object.hasOwn() to prevent potential prototype pollution issues.
- obj.hasOwnProperty("label") && - obj.hasOwnProperty("value") && + Object.hasOwn(obj, "label") && + Object.hasOwn(obj, "value") &&🧰 Tools
🪛 Biome (1.9.4)
[error] 22-22: 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] 23-23: 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/WDSSelectWidget/config/propertyPaneConfig/validations/valueKeyValidation.ts (1)
58-71:⚠️ Potential issueReplace hasOwnProperty with Object.hasOwn
Use Object.hasOwn for safer property checks.
- props.sourceData[0].hasOwnProperty(String(value[0])) + Object.hasOwn(props.sourceData[0], String(value[0]))🧰 Tools
🪛 Biome (1.9.4)
[error] 60-60: 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 (9)
app/client/src/modules/ui-builder/ui/wds/WDSSelectWidget/widget/index.tsx (3)
70-76: LGTM! Consider adding JSDoc comments for derived properties.The refactoring to use parseDerivedProperties improves maintainability. Consider adding documentation to explain the purpose of each derived property.
Line range hint
97-104: Consider memoizing the isDirty check in componentDidUpdate.The lifecycle and event handling changes look good, but there's room for optimization:
- The componentDidUpdate could benefit from memoization if defaultOptionValue changes frequently
- The null check in handleChange is a good safety addition
Consider this optimization:
+ shouldComponentUpdate(nextProps: WDSSelectWidgetProps) { + return ( + this.props.defaultOptionValue !== nextProps.defaultOptionValue || + this.props.isDirty !== nextProps.isDirty + ); + }Also applies to: 111-115
148-155: Optimize key generation for large option sets.While the key generation fixes the React-aria issue, concatenating all values might be inefficient for large datasets.
Consider using a more efficient key generation approach:
- const key = options.map((option) => option.value).join(","); + const key = options.length + '_' + (options[0]?.value ?? '') + '_' + (options[options.length - 1]?.value ?? '');The ListBoxItem rendering looks good with proper key and id usage.
Also applies to: 170-175
app/client/src/modules/ui-builder/ui/wds/WDSComboBoxWidget/widget/index.tsx (2)
38-41: Improve type safety of options handlingConsider defining a proper interface for the options type instead of inline type casting.
+interface ComboBoxOption { + value: string; + label: string; +} -const options = (isArray(this.props.options) ? this.props.options : []) as { - value: string; - label: string; -}; +const options = (isArray(this.props.options) ? this.props.options : []) as ComboBoxOption[];
52-54: Simplify validation status checkThe validation status check can be simplified using a boolean expression.
-isInvalid={ - validation.validationStatus === "invalid" && this.props.isDirty -} +isInvalid={validation.validationStatus === "invalid" && this.props.isDirty}app/client/src/modules/ui-builder/ui/wds/WDSSelectWidget/config/propertyPaneConfig/validations/defaultOptionValueValidation.ts (1)
29-35: Consider type narrowing for JSON parsingThe error handling is good, but consider narrowing the return type to improve type safety.
- function tryParseJSON(value: string): unknown { + function tryParseJSON(value: string): Record<string, unknown> | string {app/client/src/modules/ui-builder/ui/wds/WDSSelectWidget/config/propertyPaneConfig/validations/valueKeyValidation.ts (3)
30-53: Optimize sourceData key validationConsider caching the keys Set for better performance when validating multiple values.
+ const getSourceDataKeys = _.memoize((sourceData) => { + return sourceData.reduce((keys, curr) => { + Object.keys(curr).forEach((d) => keys.add(d)); + return keys; + }, new Set()); + }); if (_.isString(value)) { const sourceData = _.isArray(props.sourceData) ? props.sourceData : []; - const keys = sourceData.reduce((keys, curr) => { - Object.keys(curr).forEach((d) => keys.add(d)); - return keys; - }, new Set()); + const keys = getSourceDataKeys(sourceData);
73-92: Simplify array validation logicThe array validation could be simplified using array methods.
- const errorIndex = value.findIndex( - (d) => - !(_.isString(d) || (_.isNumber(d) && !_.isNaN(d)) || _.isBoolean(d)), - ); + const isValidType = (d: unknown): boolean => + _.isString(d) || (_.isNumber(d) && !_.isNaN(d)) || _.isBoolean(d); + const errorIndex = value.findIndex(d => !isValidType(d));
106-121: Optimize uniqueness validationConsider using Set for more efficient uniqueness validation.
- const isValid = options.every( - (d: unknown, i: number, arr: unknown[]) => arr.indexOf(d) === i, - ); + const isValid = new Set(options).size === options.length;
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (5)
app/client/src/modules/ui-builder/ui/wds/WDSComboBoxWidget/widget/index.tsx(1 hunks)app/client/src/modules/ui-builder/ui/wds/WDSSelectWidget/config/propertyPaneConfig/validations/defaultOptionValueValidation.ts(1 hunks)app/client/src/modules/ui-builder/ui/wds/WDSSelectWidget/config/propertyPaneConfig/validations/labelKeyValidation.ts(1 hunks)app/client/src/modules/ui-builder/ui/wds/WDSSelectWidget/config/propertyPaneConfig/validations/valueKeyValidation.ts(1 hunks)app/client/src/modules/ui-builder/ui/wds/WDSSelectWidget/widget/index.tsx(7 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- app/client/src/modules/ui-builder/ui/wds/WDSSelectWidget/config/propertyPaneConfig/validations/labelKeyValidation.ts
🧰 Additional context used
🪛 Biome (1.9.4)
app/client/src/modules/ui-builder/ui/wds/WDSSelectWidget/config/propertyPaneConfig/validations/defaultOptionValueValidation.ts
[error] 22-22: 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] 23-23: 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/WDSSelectWidget/config/propertyPaneConfig/validations/valueKeyValidation.ts
[error] 60-60: 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/WDSComboBoxWidget/widget/index.tsx
[error] 14-14: Using super in a static context can be confusing.
super refers to a parent class.
Unsafe fix: Use the class name instead.
(lint/complexity/noThisInStatic)
[error] 21-21: Using super in a static context can be confusing.
super refers to a parent class.
Unsafe fix: Use the class name instead.
(lint/complexity/noThisInStatic)
[error] 28-28: Using super in a static context can be confusing.
super refers to a parent class.
Unsafe fix: Use the class name instead.
(lint/complexity/noThisInStatic)
🔇 Additional comments (5)
app/client/src/modules/ui-builder/ui/wds/WDSSelectWidget/widget/index.tsx (2)
5-5: LGTM! Import changes align with TypeScript best practices.
The addition of the React.Key type and derived property utilities supports the widget's enhanced type safety and configuration management.
Also applies to: 23-24
52-53: Verify the migration from 'options' to 'sourceData' across implementations.
The dependency change from 'options' to 'sourceData' represents a significant architectural shift.
✅ Verification successful
The migration from 'options' to 'sourceData' is correctly implemented
The change is part of a well-structured data flow where:
- 'sourceData' is the input data source
- Widget internally transforms 'sourceData' into 'options' via derived properties
- All component implementations consistently use this pattern
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for existing usages of the old 'options' dependency
rg -l "options.*getDependencyMap" --type ts
Length of output: 45
Script:
#!/bin/bash
# Search for files containing 'sourceData' in WDSSelectWidget directory
rg -l "sourceData" app/client/src/modules/ui-builder/ui/wds/WDSSelectWidget/
# Search for any select widget implementations using getDependencyMap
rg "getDependencyMap" app/client/src/modules/ui-builder/ui/wds/WDSSelectWidget/ -A 5 -B 5
# Search for any references to options in select widget files
rg "options" app/client/src/modules/ui-builder/ui/wds/WDSSelectWidget/ -A 2 -B 2
Length of output: 17124
app/client/src/modules/ui-builder/ui/wds/WDSComboBoxWidget/widget/index.tsx (1)
42-45: Consider addressing the underlying React-aria issue
The key-based re-render workaround suggests an underlying issue with React-aria's item ID handling. Consider:
- Investigating if there's a newer version of React-aria that fixes this
- Opening an issue with React-aria if this is a bug
app/client/src/modules/ui-builder/ui/wds/WDSSelectWidget/config/propertyPaneConfig/validations/defaultOptionValueValidation.ts (2)
6-15: LGTM! Clear function signature and documentation.
The validation rules are well-documented and the function signature properly handles unknown input types.
37-57: Consider explicit array handling
The validation logic should explicitly handle array inputs since the widget might receive array values.
Run this script to check array handling in the codebase:
jsartisan
added
ok-to-test
| }[]; | ||
| // This is key is used to force re-render of the widget when the options change. | ||
| // Why force re-render on options change? | ||
| // Sometimes when the user is changing options, the select throws an error ( related to react-aria code ) saying "cannot change id of item". |
There was a problem hiding this comment.
What does it mean sometimes? We don't know the exact reason for this?
There was a problem hiding this comment.
Ok "sometimes" seems wrong word here but the issue is that ids should not change while re-rendering and when we were changing the values from property panes, ids were changing and react-aria was throwing an error.
There was a problem hiding this comment.
Got it. Please change the comment.
| /** | ||
| * Validation rules: | ||
| * 1. Can be a string (representing a key in sourceData) | ||
| * 2. Can be an Array of string, number, boolean (for direct option values) |
There was a problem hiding this comment.
I don't quite understand why we need to check that the key can be an array. Could you please clarify this?
There was a problem hiding this comment.
I took this logic from the fixed layout component. the value comes as an array if we use evaluations inside the property like {{item.name}} and it just takes first value if they are all same. I know it's confusing and I talked to rahul about it that why we even need it. We might not even need it. I'll talk to vadim and carina after holidays about this because this feature of supporting {{item.name}} makes no sense to me. Not sure what usecases it solves.
There was a problem hiding this comment.
At first I thought it was to make dependent fields, but it doesn't work at all the way I expected. Please discuss this.
| value: values[i], | ||
| })); | ||
| }, | ||
| // |
There was a problem hiding this comment.
// — all this comments should be deleted.
There was a problem hiding this comment.
Yea right, with the new utility ( parseDerivedProperties) , we can remove these.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (3)
app/client/src/modules/ui-builder/ui/wds/WDSSelectWidget/widget/index.tsx (3)
70-76: LGTM: Improved derived properties implementation.The refactored derived properties implementation using
parseDerivedPropertiesis more maintainable and flexible.Consider adding JSDoc comments to document the purpose of each derived property function in the
derivedPropertyFnsfile.
111-114: Consider enhancing error handling in handleChange.While the null check is good, consider logging or handling the null case more explicitly for debugging purposes.
- if (updatedValue === null) return; + if (updatedValue === null) { + // Log or handle the null case explicitly + console.debug('Select widget: No value selected'); + return; + }
148-156: LGTM: Improved widget view implementation with a performance note.The changes improve type safety and solve the React-aria id issue. However, consider memoizing the key generation for large option lists.
+ const getOptionsKey = React.useMemo( + () => options.map((option) => option.value).join(","), + [options] + ); - const key = options.map((option) => option.value).join(",");Also applies to: 171-176
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
app/client/src/modules/ui-builder/ui/wds/WDSSelectWidget/widget/derived.js(1 hunks)app/client/src/modules/ui-builder/ui/wds/WDSSelectWidget/widget/index.tsx(7 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- app/client/src/modules/ui-builder/ui/wds/WDSSelectWidget/widget/derived.js
🔇 Additional comments (3)
app/client/src/modules/ui-builder/ui/wds/WDSSelectWidget/widget/index.tsx (3)
5-5: LGTM: Import changes align with the new implementation.
The new imports support the updated type definitions and utility functions needed for the refactored code.
Also applies to: 23-25
Line range hint 97-105: LGTM: Proper handling of default value changes.
The lifecycle method correctly handles the reset of isDirty state when defaultOptionValue changes.
52-53: Verify dependency changes across the codebase.
The dependency change from 'options' to 'sourceData' needs verification to ensure all dependent components are updated accordingly.
✅ Verification successful
Dependency change is consistent with widget's implementation
The change from 'options' to 'sourceData' in the dependency map is correct and consistent with the widget's implementation. The widget still uses 'options' internally but derives it from 'sourceData', establishing the correct dependency chain.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for any remaining references to 'options' in dependency maps
ast-grep --pattern 'getDependencyMap() {
$$$
options: $_
$$$
}'
# Search for new sourceData dependencies to ensure consistency
ast-grep --pattern 'getDependencyMap() {
$$$
sourceData: $_
$$$
}'
Length of output: 146
Script:
#!/bin/bash
# Search for files containing 'getDependencyMap' function
rg -l "getDependencyMap"
# Search for any references to 'options' in dependency-related contexts
rg "options.*:" -A 2 -B 2
# Search for any references to 'sourceData' in dependency-related contexts
rg "sourceData.*:" -A 2 -B 2
# Search specifically in the WDSSelectWidget directory
fd . "app/client/src/modules/ui-builder/ui/wds/WDSSelectWidget" --type f --exec rg -l "options|sourceData" {}
Length of output: 66102
Script:
#!/bin/bash
# Search for getDependencyMap implementations in WDSSelectWidget
rg -A 10 -B 2 "getDependencyMap.*{" app/client/src/modules/ui-builder/ui/wds/WDSSelectWidget/widget/index.tsx
# Search for any references to sourceData in WDSSelectWidget
rg -A 2 -B 2 "sourceData" app/client/src/modules/ui-builder/ui/wds/WDSSelectWidget/widget/index.tsx
# Search for any references to options in WDSSelectWidget
rg -A 2 -B 2 "options" app/client/src/modules/ui-builder/ui/wds/WDSSelectWidget/widget/index.tsx
Length of output: 1737
Fixes #38197 /ok-to-test tags="@tag.Anvil" <!-- This is an auto-generated comment: release notes by coderabbit.ai --> ## Summary by CodeRabbit ## Summary by CodeRabbit - **New Features** - Enhanced styling for select input components, improving visual consistency. - Introduced new validation functions for widget properties, ensuring better data integrity. - Added properties for improved configuration of select widgets, including dynamic data binding. - New utility functions for handling options in select widgets, enhancing functionality. - Introduced a constant for sample data to facilitate testing and development. - **Bug Fixes** - Resolved issues with widget rendering and responsiveness to property changes. - **Refactor** - Streamlined widget implementation by leveraging inherited functionalities and simplifying methods. - Updated methods to improve handling of derived properties and options. - Removed obsolete configuration files and validation functions to clean up the codebase. <!-- end of auto-generated comment: release notes by coderabbit.ai --> <!-- This is an auto-generated comment: Cypress test results --> > [!TIP] > 🟢 🟢 🟢 All cypress tests have passed! 🎉 🎉 🎉 > Workflow run: <https://github.com/appsmithorg/appsmith/actions/runs/12480977859> > Commit: d669e05 > <a href="https://internal.appsmith.com/app/cypress-dashboard/rundetails-65890b3c81d7400d08fa9ee5?branch=master&workflowId=12480977859&attempt=1" target="_blank">Cypress dashboard</a>. > Tags: `@tag.Anvil` > Spec: > <hr>Tue, 24 Dec 2024 11:43:59 UTC <!-- end of auto-generated comment: Cypress test results -->
Fixes #38197
/ok-to-test tags="@tag.Anvil"
Summary by CodeRabbit
Summary by CodeRabbit
New Features
Bug Fixes
Refactor
Tip
🟢 🟢 🟢 All cypress tests have passed! 🎉 🎉 🎉
Workflow run: https://github.com/appsmithorg/appsmith/actions/runs/12480977859
Commit: d669e05
Cypress dashboard.
Tags:
@tag.AnvilSpec:
Tue, 24 Dec 2024 11:43:59 UTC