fix: Show error message when label and value are both empty in radio group widget's properties #34245
Conversation
prasad-madine
requested review from
a team and
riodeuno
and removed request for
a team
WalkthroughRecent updates to the Changes
Sequence DiagramssequenceDiagram
participant User
participant KeyValueComponent
participant ErrorMessageBox
User->>KeyValueComponent: Enter Name and Value pairs
KeyValueComponent->>KeyValueComponent: validatePairs(pairs)
KeyValueComponent->>ErrorMessageBox: Display error messages if validation fails
ErrorMessageBox->>User: Show error messages alongside input fields
Poem
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (invoked as PR comments)
Additionally, you can add CodeRabbit Configuration File (
|
![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 and nitpick comments (5)
app/client/src/components/propertyControls/KeyValueComponent.tsx (2)
41-45: Consider adding a descriptive comment for theFlexBoxstyled component.Adding a comment explaining the purpose of
FlexBoxcan improve code readability, especially for new contributors or during future maintenance.
[!TIP]
Codebase VerificationThe validation logic in the
validatePairsfunction currently only checks if both thelabelandvaluefields are empty. It does not handle cases where only one of the fields is empty.
- Location to address:
app/client/src/components/propertyControls/KeyValueComponent.tsx, within thevalidatePairsfunction.Consider extending the validation logic to handle cases where either the
labelorvaluefield is empty.Analysis chain
Line range hint
81-144: Ensure error handling covers all necessary cases.The current implementation only checks for both fields being empty. Consider extending this to handle cases where one of the fields is empty but not both, which might also require validation.
Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify handling of cases where only one of the fields is empty. # Test: Search for the usage of validatePairs function and check if there are any cases not covered by current validation. rg --type typescript "validatePairs"Length of output: 71
Script:
#!/bin/bash # Correcting the search for the usage of validatePairs function in the codebase. rg "validatePairs"Length of output: 662
Script:
#!/bin/bash # Extracting the implementation of the validatePairs function to analyze its logic. ast-grep --lang ts --pattern 'function validatePairs($_) { $$$ }'Length of output: 67
Script:
#!/bin/bash # Locate the definition of the validatePairs function within KeyValueComponent.tsx to analyze its implementation. rg "function validatePairs" app/client/src/components/propertyControls/KeyValueComponent.tsxLength of output: 161
Script:
#!/bin/bash # Extract the full implementation of the validatePairs function from KeyValueComponent.tsx for analysis. rg -A 10 "function validatePairs" app/client/src/components/propertyControls/KeyValueComponent.tsxLength of output: 458
app/client/src/widgets/RadioGroupWidget/widget/index.tsx (3)
Line range hint
62-62: ReplacehasOwnPropertywithObject.hasOwn.Using
Object.hasOwninstead ofhasOwnPropertyavoids issues related to the object prototype chain and is recommended for checking properties.- if (!uniqueLabels.hasOwnProperty(value)) { + if (!Object.hasOwn(uniqueLabels, value)) {
Line range hint
112-112: Correct the invalidtypeofcomparison.The
typeofcomparison should be against a valid string literal to ensure proper type checking.- if (typeof value !== valueType) { + if (typeof value !== "string" && typeof value !== "number") {
Line range hint
158-160: Remove unnecessaryelseclause.The
elseclause can be omitted as the previous branches containbreakstatements, making it redundant.- else { + // Removed unnecessary else clause
Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Files selected for processing (2)
- app/client/src/components/propertyControls/KeyValueComponent.tsx (9 hunks)
- app/client/src/widgets/RadioGroupWidget/widget/index.tsx (1 hunks)
Additional context used
Biome
app/client/src/components/propertyControls/KeyValueComponent.tsx
[error] 205-205: Missing key property for this element in iterable. (lint/correctness/useJsxKeyInIterable)
The order of the items may change, and having a key can help React identify which item was moved.
Check the React documentation.app/client/src/widgets/RadioGroupWidget/widget/index.tsx
[error] 62-62: Do not access Object.prototype method 'hasOwnProperty' from target object. (lint/suspicious/noPrototypeBuiltins)
It's recommended using Object.hasOwn() instead of using Object.hasOwnProperty().
See MDN web docs for more details.
[error] 112-112: Invalid
typeofcomparison value: this expression is not a string literal (lint/suspicious/useValidTypeof)not a string literal
[error] 158-160: This else clause can be omitted because previous branches break early. (lint/style/noUselessElse)
Unsafe fix: Omit the else clause.
app/client/src/components/propertyControls/KeyValueComponent.tsx
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
Actionable comments posted: 0
Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Files selected for processing (1)
- app/client/src/components/propertyControls/KeyValueComponent.tsx (8 hunks)
Files not reviewed due to errors (1)
- app/client/src/components/propertyControls/KeyValueComponent.tsx (Error: Server error. Please try again later.)
riodeuno
changed the title
|
/build-deploy-preview skip-tests=true |
|
Deploying Your Preview: https://github.com/appsmithorg/appsmith/actions/runs/9596052403. |
|
Deploy-Preview-URL: https://ce-34245.dp.appsmith.com |
|
@PrasadMadine The error message doesn't look appropriate as the size and positioning of text don't match the context.
Here is a screenshot of the expected design. |
There was a problem hiding this comment.
Actionable comments posted: 0
Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Files ignored due to path filters (1)
app/client/src/assets/icons/alert/warning-error.svgis excluded by!**/*.svg
Files selected for processing (1)
- app/client/src/components/propertyControls/KeyValueComponent.tsx (9 hunks)
Files not reviewed due to errors (1)
- app/client/src/components/propertyControls/KeyValueComponent.tsx (no review received)
|
Hello @riodeuno , Made the suggested UI changes, please review this PR. Thankyou |
|
This PR has not seen activitiy for a while. It will be closed in 7 days unless further activity is detected. |
|
Hello @riodeuno Thank you! |
|
We need to show errors as per the conventions.
|
|
Hello @riodeuno Thank you! |
…to fix/radio-button-widget-to-show-error-message-#4653
prasad-madine
requested review from
ApekshaBhosale and
sagar-qa007
as code owners
There was a problem hiding this comment.
Actionable comments posted: 0
Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Files selected for processing (1)
- app/client/cypress/e2e/Regression/ClientSide/IDE/Canvas_Context_Bug_Fixes.js (1 hunks)
Files skipped from review due to trivial changes (1)
- app/client/cypress/e2e/Regression/ClientSide/IDE/Canvas_Context_Bug_Fixes.js
app/client/cypress/e2e/Regression/ClientSide/IDE/Canvas_Context_Bug_Fixes.js
Show resolved
Hide resolved
There was a problem hiding this comment.
Actionable comments posted: 0
Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Files selected for processing (1)
- app/client/cypress/e2e/Regression/ClientSide/IDE/Canvas_Context_Bug_Fixes.js (1 hunks)
Files skipped from review due to trivial changes (1)
- app/client/cypress/e2e/Regression/ClientSide/IDE/Canvas_Context_Bug_Fixes.js
|
@PrasadMadine Please see the PR #35800. There are some typescript issues in the code that fails the build. I've discussed your PR with the team and we'll be merging your PR once all the tests pass. |
| cy.get(`[data-guided-tour-id="explorer-entity-Image1"]`).should("exist"); | ||
| }); | ||
| }); | ||
| }); |
There was a problem hiding this comment.
Please remove this changes as it is not part of functionality.
4 participants
User description
Description:
In Radio button widget->In property pane->if the options section(label or value) is kept empty the required error message should be displayed.
Bug: In Radio button widget->In property pane->if the options section(label or value) is kept empty the required error message should be displayed
Before:
After resolving bug:
Summary by CodeRabbit
New Features
Bug Fixes
Tests
PR Type
Bug fix, Enhancement
Description
KeyValueComponent.ErrorMessageBoxstyled component for displaying error messages.validatePairsfunction to validate key-value pairs and set appropriate error messages.Changes walkthrough 📝
KeyValueComponent.tsx
Add validation and error messages for empty key-value pairsapp/client/src/components/propertyControls/KeyValueComponent.tsx
ErrorMessageBoxstyled component for error display.validatePairsfunction to validate key-value pairs.fields.