fix: remove Select key value selects, fix default #36158
Conversation
WalkthroughThe changes enhance the validation logic within the Changes
Possibly related PRs
Poem
Tip New featuresWalkthrough comment now includes:
Notes:
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
|
znamenskii-ilia
changed the title
|
/build-deploy-preview skip-tests=true |
|
Deploying Your Preview: https://github.com/appsmithorg/appsmith/actions/runs/10737042545. |
![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
Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Files selected for processing (5)
- app/client/src/widgets/wds/WDSComboBoxWidget/config/propertyPaneConfig/validations/optionsCustomValidation.ts (1 hunks)
- app/client/src/widgets/wds/WDSComboBoxWidget/widget/index.tsx (1 hunks)
- app/client/src/widgets/wds/WDSSelectWidget/config/propertyPaneConfig/contentConfig.ts (2 hunks)
- app/client/src/widgets/wds/WDSSelectWidget/config/propertyPaneConfig/validations/optionsCustomValidation.ts (2 hunks)
- app/client/src/widgets/wds/WDSSelectWidget/widget/index.tsx (1 hunks)
Additional comments not posted (1)
app/client/src/widgets/wds/WDSComboBoxWidget/widget/index.tsx (1)
155-159: Enhance robustness with added validation inoptionsToItems.The addition of a validation step in the
optionsToItemsmethod ensures that each item has a definedlabelandid, enhancing the robustness of the widget. This is a crucial improvement for maintaining data integrity. However, returning an empty array when any item fails the validation could have significant impacts on other parts of the application that depend on this method's output.Consider adding logging to help trace issues when invalid items are encountered, which would aid in debugging and maintaining the application.
| const isValidKeys = options.every((option) => { | ||
| return ( | ||
| _.isPlainObject(option) && | ||
| _.has(option, "label") && | ||
| _.has(option, "value") | ||
| ); | ||
| }); | ||
|
|
||
| if (!isValidKeys) { | ||
| return createErrorValidationResponse(options, { | ||
| name: "ValidationError", | ||
| message: | ||
| 'This value does not evaluate to type Array<{ "label": "string", "value": "string" | number }>', | ||
| }); | ||
| } |
There was a problem hiding this comment.
Enhanced Validation Logic in optionsCustomValidation
The addition of the isValidKeys check at lines 80-86 ensures that each option in the array is a valid object with the required properties (label and value). This is a crucial step for maintaining data integrity.
However, there is a potential improvement to consider:
- Refactor Opportunity: The validation logic is duplicated in the function. It would be more efficient to extract this into a separate utility function that can be reused, reducing code duplication and improving maintainability.
+ function validateOption(option) {
+ return _.isPlainObject(option) && _.has(option, "label") && _.has(option, "value");
+ }
- const isValidKeys = options.every((option) => {
- return (
- _.isPlainObject(option) &&
- _.has(option, "label") &&
- _.has(option, "value")
- );
- });
+ const isValidKeys = options.every(validateOption);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 isValidKeys = options.every((option) => { | |
| return ( | |
| _.isPlainObject(option) && | |
| _.has(option, "label") && | |
| _.has(option, "value") | |
| ); | |
| }); | |
| if (!isValidKeys) { | |
| return createErrorValidationResponse(options, { | |
| name: "ValidationError", | |
| message: | |
| 'This value does not evaluate to type Array<{ "label": "string", "value": "string" | number }>', | |
| }); | |
| } | |
| function validateOption(option) { | |
| return _.isPlainObject(option) && _.has(option, "label") && _.has(option, "value"); | |
| } | |
| const isValidKeys = options.every(validateOption); | |
| if (!isValidKeys) { | |
| return createErrorValidationResponse(options, { | |
| name: "ValidationError", | |
| message: | |
| 'This value does not evaluate to type Array<{ "label": "string", "value": "string" | number }>', | |
| }); | |
| } |
| const isValidKeys = options.every((option) => { | ||
| return ( | ||
| _.isPlainObject(option) && | ||
| _.has(option, "label") && | ||
| _.has(option, "value") | ||
| ); | ||
| }); | ||
|
|
||
| if (!isValidKeys) { | ||
| return createErrorValidationResponse(options, { | ||
| name: "ValidationError", | ||
| message: | ||
| 'This value does not evaluate to type Array<{ "label": "string", "value": "string" | number }>', | ||
| }); | ||
| } |
There was a problem hiding this comment.
Enhanced Validation Logic in optionsCustomValidation
The addition of the isValidKeys check at lines 42-48 ensures that each option in the array is a valid object with the required properties (label and value). This is a crucial step for maintaining data integrity.
However, there is a potential improvement to consider:
- Refactor Opportunity: The validation logic is duplicated in the function. It would be more efficient to extract this into a separate utility function that can be reused, reducing code duplication and improving maintainability.
+ function validateOption(option) {
+ return _.isPlainObject(option) && _.has(option, "label") && _.has(option, "value");
+ }
- const isValidKeys = options.every((option) => {
- return (
- _.isPlainObject(option) &&
- _.has(option, "label") &&
- _.has(option, "value")
- );
- });
+ const isValidKeys = options.every(validateOption);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 isValidKeys = options.every((option) => { | |
| return ( | |
| _.isPlainObject(option) && | |
| _.has(option, "label") && | |
| _.has(option, "value") | |
| ); | |
| }); | |
| if (!isValidKeys) { | |
| return createErrorValidationResponse(options, { | |
| name: "ValidationError", | |
| message: | |
| 'This value does not evaluate to type Array<{ "label": "string", "value": "string" | number }>', | |
| }); | |
| } | |
| function validateOption(option) { | |
| return _.isPlainObject(option) && _.has(option, "label") && _.has(option, "value"); | |
| } | |
| const isValidKeys = options.every(validateOption); | |
| if (!isValidKeys) { | |
| return createErrorValidationResponse(options, { | |
| name: "ValidationError", | |
| message: | |
| 'This value does not evaluate to type Array<{ "label": "string", "value": "string" | number }>', | |
| }); | |
| } |
| const items = options.map((option) => ({ | ||
| label: option[this.props.optionLabel || "label"] as string, | ||
| id: option[this.props.optionValue || "value"] as string, | ||
| })); | ||
|
|
||
| const isValidItems = items.every( | ||
| (item) => item.label !== undefined && item.id !== undefined, | ||
| ); | ||
|
|
||
| return isValidItems ? items : []; |
There was a problem hiding this comment.
Improved Validation in optionsToSelectItems Method
The changes at lines 140-149 introduce a validation step that ensures each item in the items array has both a defined label and id. This is a crucial step for maintaining data integrity and ensuring that only valid items are returned.
However, there is a potential improvement to consider:
- Error Handling: Currently, if the validation fails, an empty array is returned. It might be beneficial to also log an error or provide feedback to the user when this happens, to aid in debugging and user experience.
+ import { logger } from "utilities/logger";
const isValidItems = items.every(
(item) => item.label !== undefined && item.id !== undefined,
);
- return isValidItems ? items : [];
+ if (isValidItems) {
+ return items;
+ } else {
+ logger.error("Invalid items detected", items);
+ return [];
+ }Committable suggestion was skipped due to low confidence.
| import { ValidationTypes } from "constants/WidgetValidation"; | ||
| import { EvaluationSubstitutionType } from "entities/DataTree/dataTreeFactory"; | ||
| import { EVAL_VALUE_PATH } from "../../../../../utils/DynamicBindingUtils"; | ||
| import type { PropertyUpdates } from "../../../../../WidgetProvider/constants"; | ||
| import type { WidgetProps } from "../../../../BaseWidget"; | ||
| import { AutocompleteDataType } from "utils/autocomplete/AutocompleteDataType"; | ||
| import type { WDSSelectWidgetProps } from "../../widget/types"; | ||
| import { | ||
| defaultOptionValidation, | ||
| optionsCustomValidation, | ||
| } from "./validations"; | ||
| import type { WidgetProps } from "../../../../BaseWidget"; | ||
| import type { PropertyUpdates } from "../../../../../WidgetProvider/constants"; |
There was a problem hiding this comment.
Significant simplification in configuration.
The removal of several validation functions and property configurations from the WDSSelectWidget represents a significant simplification. This could make the widget easier to configure and maintain but might also reduce its flexibility and robustness in handling diverse data scenarios.
It's important to document the rationale behind these changes to ensure that the simplification aligns with the overall design goals and does not inadvertently introduce limitations or bugs.
Consider adding comments or documentation within the code to explain why these changes were made, especially if they relate to discussions or decisions that are not visible within the codebase itself.
Also applies to: 9-10
|
Deploy-Preview-URL: https://ce-36158.dp.appsmith.com |
znamenskii-ilia
force-pushed
the
fix/select-default-value-3
branch
from
ad0b1ba to
b162d6e
Compare
|
/build-deploy-preview skip-tests=true |
|
Deploying Your Preview: https://github.com/appsmithorg/appsmith/actions/runs/10737485628. |
There was a problem hiding this comment.
Actionable comments posted: 0
Outside diff range, codebase verification and nitpick comments (2)
app/client/src/widgets/wds/WDSSelectWidget/config/propertyPaneConfig/validations/defaultOptionValidation.ts (1)
51-51: Ensure robustness by validatingoptionobjects.The assumption that all
optionobjects have a "value" property is generally valid for UI components, but not universally across all usages in the codebase. To prevent potential runtime errors, consider adding a validation check within thedefaultOptionValidationfunction to ensure that eachoptionobject indeed contains a "value" property before accessing it.
- Consider adding a check like
if (option && option.hasOwnProperty("value"))before returningoption["value"].This will help maintain robustness and prevent errors if unexpected
optionsobjects are passed to the function.Analysis chain
Consider verifying the consistency of
optionobjects.The modification to directly access the "value" property simplifies the code but assumes that all
optionobjects will consistently have this property. This assumption might not hold true in all cases, especially if the structure ofoptionsis modified elsewhere in the application.It's important to ensure that this assumption is valid across all usages of the
optionsobject to prevent runtime errors. Consider adding a check to ensure that eachoptionobject indeed contains a "value" property before accessing it, or verify that all producers of theoptionsarray conform to this structure.
Let's correct the file type and search for
optionsdefinitions in JavaScript/TypeScript files to verify that eachoptionobject includes a "value" property.Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify that all `option` objects in the application have a "value" property. # Test: Search for all occurrences where `options` are defined or modified. Expect: Each `option` object should have a "value" property. rg --type python -A 5 $'options ='Length of output: 66
Script:
#!/bin/bash # Description: Verify that all `option` objects in the application have a "value" property. # Test: Search for all occurrences where `options` are defined or modified in JavaScript/TypeScript files. rg --type js --type ts -A 5 'options ='Length of output: 87357
app/client/src/widgets/wds/WDSSelectWidget/config/propertyPaneConfig/contentConfig.ts (1)
1-10: Document the rationale behind significant configuration changes.The removal of several validation functions and property configurations represents a significant simplification. This could make the widget easier to configure and maintain but might also reduce its flexibility and robustness in handling diverse data scenarios.
It's important to document the rationale behind these changes to ensure that the simplification aligns with the overall design goals and does not inadvertently introduce limitations or bugs.
Consider adding comments or documentation within the code to explain why these changes were made, especially if they relate to discussions or decisions that are not visible within the codebase itself.
Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Files selected for processing (6)
- app/client/src/widgets/wds/WDSComboBoxWidget/config/propertyPaneConfig/validations/optionsCustomValidation.ts (1 hunks)
- app/client/src/widgets/wds/WDSComboBoxWidget/widget/index.tsx (1 hunks)
- app/client/src/widgets/wds/WDSSelectWidget/config/propertyPaneConfig/contentConfig.ts (2 hunks)
- app/client/src/widgets/wds/WDSSelectWidget/config/propertyPaneConfig/validations/defaultOptionValidation.ts (1 hunks)
- app/client/src/widgets/wds/WDSSelectWidget/config/propertyPaneConfig/validations/optionsCustomValidation.ts (2 hunks)
- app/client/src/widgets/wds/WDSSelectWidget/widget/index.tsx (1 hunks)
Files skipped from review as they are similar to previous changes (4)
- app/client/src/widgets/wds/WDSComboBoxWidget/config/propertyPaneConfig/validations/optionsCustomValidation.ts
- app/client/src/widgets/wds/WDSComboBoxWidget/widget/index.tsx
- app/client/src/widgets/wds/WDSSelectWidget/config/propertyPaneConfig/validations/optionsCustomValidation.ts
- app/client/src/widgets/wds/WDSSelectWidget/widget/index.tsx
|
Deploy-Preview-URL: https://ce-36158.dp.appsmith.com |
znamenskii-ilia
added
Anvil team
...dgets/wds/WDSComboBoxWidget/config/propertyPaneConfig/validations/optionsCustomValidation.ts
Show resolved
Hide resolved
znamenskii-ilia
force-pushed
the
fix/select-default-value-3
branch
from
b162d6e to
3afce8d
Compare
znamenskii-ilia
added
ok-to-test
There was a problem hiding this comment.
Actionable comments posted: 0
Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Files selected for processing (6)
- app/client/src/widgets/wds/WDSComboBoxWidget/config/propertyPaneConfig/validations/optionsCustomValidation.ts (1 hunks)
- app/client/src/widgets/wds/WDSComboBoxWidget/widget/index.tsx (1 hunks)
- app/client/src/widgets/wds/WDSSelectWidget/config/propertyPaneConfig/contentConfig.ts (2 hunks)
- app/client/src/widgets/wds/WDSSelectWidget/config/propertyPaneConfig/validations/defaultOptionValidation.ts (1 hunks)
- app/client/src/widgets/wds/WDSSelectWidget/config/propertyPaneConfig/validations/optionsCustomValidation.ts (2 hunks)
- app/client/src/widgets/wds/WDSSelectWidget/widget/index.tsx (1 hunks)
Files skipped from review as they are similar to previous changes (5)
- app/client/src/widgets/wds/WDSComboBoxWidget/config/propertyPaneConfig/validations/optionsCustomValidation.ts
- app/client/src/widgets/wds/WDSComboBoxWidget/widget/index.tsx
- app/client/src/widgets/wds/WDSSelectWidget/config/propertyPaneConfig/validations/defaultOptionValidation.ts
- app/client/src/widgets/wds/WDSSelectWidget/config/propertyPaneConfig/validations/optionsCustomValidation.ts
- app/client/src/widgets/wds/WDSSelectWidget/widget/index.tsx
Additional comments not posted (6)
app/client/src/widgets/wds/WDSSelectWidget/config/propertyPaneConfig/contentConfig.ts (6)
1-10: Simplification of Configuration:The removal of several validation functions and property configurations from the
WDSSelectWidgetrepresents a significant simplification. This could make the widget easier to configure and maintain but might also reduce its flexibility and robustness in handling diverse data scenarios.It's important to document the rationale behind these changes to ensure that the simplification aligns with the overall design goals and does not inadvertently introduce limitations or bugs.
Consider adding comments or documentation within the code to explain why these changes were made, especially if they relate to discussions or decisions that are not visible within the codebase itself.
10-10: Ensure Comprehensive Documentation:Given the significant changes in the widget's configuration, it is crucial to ensure that all modifications are well-documented. This includes updating any related documentation or help texts within the application to reflect the new configuration options and behaviors.
Documentation is key to helping both current and future developers understand the context and reasoning behind these changes, especially when simplifying complex features.
10-10: Review Dependencies and Validation Logic:The removal of specific property configurations such as
optionLabelandoptionValueand their associated validations suggests a shift towards a more streamlined approach. However, this raises concerns about the robustness of the new validation logic, particularlyoptionsCustomValidation.Ensure that the new validation logic comprehensively covers all scenarios previously handled by the removed validations. It might be beneficial to include unit tests or examples within the code to demonstrate the effectiveness and coverage of the new validations.
10-10: Potential Impact on Widget Behavior:The changes in validation logic and property configurations could potentially alter how the widget behaves in certain scenarios, such as handling edge cases or specific data formats. It is important to conduct thorough testing, particularly integration and end-to-end tests, to ensure that the widget continues to function as expected in all supported use cases.
Consider adding automated tests that specifically target the new configurations and validations to ensure they perform correctly across a variety of scenarios.
10-10: Address Test Failures Before Merging:The placeholder for Cypress test results indicates that some tests have failed. It is crucial to address these failures and ensure that all tests pass before proceeding with merging this PR. Failing tests can be indicative of issues that might affect the stability or functionality of the application.
Review the failed tests to understand their cause and make necessary adjustments to the code or tests to resolve these issues. Ensuring a green test suite is essential for maintaining the quality and reliability of the application.
10-10: Consultation with Stakeholders:Given the significant changes made to the widget configuration, it is advisable to consult with key stakeholders, including the DevRel and Marketing teams, to determine if these changes should be communicated to the users. Significant changes in functionality or configuration can impact user experience and may require adjustments in user documentation or training materials.
Engage with the relevant teams to assess the need for communication and ensure that any necessary information is accurately and timely conveyed to the users.
znamenskii-ilia
dismissed
sagar-qa007’s stale review
The reason was explained in Slack DM
## Description https://theappsmith.slack.com/archives/C025SE88KNE/p1725434811592439 Fixes #`Issue Number` _or_ Fixes `Issue URL` > [!WARNING] > _If no issue exists, please create an issue first, and check with the maintainers if the issue is valid._ ## Automation /ok-to-test tags="@tag.All" ### 🔍 Cypress test results <!-- This is an auto-generated comment: Cypress test results --> > [!TIP] > 🟢 🟢 🟢 All cypress tests have passed! 🎉 🎉 🎉 > Workflow run: <https://github.com/appsmithorg/appsmith/actions/runs/10772310432> > Commit: 3afce8d > <a href="https://internal.appsmith.com/app/cypress-dashboard/rundetails-65890b3c81d7400d08fa9ee5?branch=master&workflowId=10772310432&attempt=1" target="_blank">Cypress dashboard</a>. > Tags: `@tag.All` > Spec: > <hr>Mon, 09 Sep 2024 12:43:13 UTC <!-- end of auto-generated comment: Cypress test results --> ## Communication Should the DevRel and Marketing teams inform users about this change? - [ ] Yes - [ ] No <!-- This is an auto-generated comment: release notes by coderabbit.ai --> ## Summary by CodeRabbit ## Summary by CodeRabbit - **New Features** - Introduced enhanced validation for options in the WDSComboBoxWidget, ensuring all entries are valid objects with `label` and `value` properties. - Updated WDSSelectWidget to include validation for the options array, ensuring only valid items are processed. - **Bug Fixes** - Removed outdated validation methods and configurations in WDSSelectWidget for a more streamlined approach. - **Refactor** - Simplified the configuration and validation logic within the WDSSelectWidget, improving maintainability and clarity. <!-- end of auto-generated comment: release notes by coderabbit.ai -->
Description
https://theappsmith.slack.com/archives/C025SE88KNE/p1725434811592439
Fixes #
Issue Numberor
Fixes
Issue URLWarning
If no issue exists, please create an issue first, and check with the maintainers if the issue is valid.
Automation
/ok-to-test tags="@tag.All"
🔍 Cypress test results
Tip
🟢 🟢 🟢 All cypress tests have passed! 🎉 🎉 🎉
Workflow run: https://github.com/appsmithorg/appsmith/actions/runs/10772310432
Commit: 3afce8d
Cypress dashboard.
Tags:
@tag.AllSpec:
Mon, 09 Sep 2024 12:43:13 UTC
Communication
Should the DevRel and Marketing teams inform users about this change?
Summary by CodeRabbit
Summary by CodeRabbit
New Features
labelandvalueproperties.Bug Fixes
Refactor