[Nu-1889] provide an unique validation message to the scenario labels #7182
Conversation
📝 Walkthrough📝 WalkthroughWalkthroughThis pull request introduces enhancements to the label input validation in the Cypress end-to-end tests and the Changes
Possibly related PRs
Suggested reviewers
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
CodeRabbit Configuration File (
|
Dzuming
changed the title
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Outside diff range and nitpick comments (4)
designer/client/cypress/e2e/labels.cy.ts (3)
36-42: LGTM! Consider adding a wait after clearing inputThe test correctly validates the duplicate label error scenario. However, for improved test reliability, consider adding a small wait after clearing the input field to ensure the UI has fully updated before proceeding with the next action.
cy.get("@labelInput").find("input").clear(); +cy.wait(100); // Add a small wait to ensure UI update
Line range hint
1-1: Consider adding tests for additional validation scenariosThe test suite could be enhanced by adding the following test cases:
- Maximum allowed label length validation
- Special characters in labels validation
- Empty label validation
This would provide more comprehensive coverage of the validation logic.
40-40: Consider extracting error messages as constantsFor better maintainability and consistency, consider extracting the error message strings into constants at the top of the file:
const ERROR_MESSAGES = { DUPLICATE_LABEL: "This label already exists. Please enter a unique value.", INVALID_LABEL: "Incorrect value 'very long tag'" };designer/client/src/components/toolbars/scenarioDetails/ScenarioLabels.tsx (1)
29-30: Consider using i18n for the error messageThe error message should be internationalized using i18next to maintain consistency with other messages in the codebase.
-const labelUniqueValidation = (label: string) => ({ label, messages: ["This label already exists. Please enter a unique value."] }); +const labelUniqueValidation = (label: string) => ({ + label, + messages: [i18next.t("panels.scenarioDetails.labels.duplicateLabel", "This label already exists. Please enter a unique value.")] +});
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (3)
designer/client/cypress/e2e/labels.cy.ts(1 hunks)designer/client/src/components/toolbars/scenarioDetails/ScenarioLabels.tsx(3 hunks)docs/Changelog.md(1 hunks)
🔇 Additional comments (4)
designer/client/src/components/toolbars/scenarioDetails/ScenarioLabels.tsx (2)
130-135: LGTM! Good performance optimization
The useCallback implementation is correct and will prevent unnecessary re-renders by memoizing the function. The dependency array is properly configured with scenarioLabelOptions.
159-162: Consider handling potential race condition in validation
There's a potential race condition between the HTTP validation and uniqueness check. The uniqueness validation could overwrite error states from the HTTP validation that arrives later due to the async nature of the calls.
Consider this approach to preserve both validation results:
-if (isInputInSelectedOptions(newInput)) {
- setInputErrors((prevState) => [...prevState, labelUniqueValidation(newInput)]);
-}
+if (isInputInSelectedOptions(newInput)) {
+ setInputErrors((prevState) => {
+ const uniqueError = labelUniqueValidation(newInput);
+ return prevState.some(error => error.label === uniqueError.label)
+ ? prevState
+ : [...prevState, uniqueError];
+ });
+}docs/Changelog.md (2)
103-103: LGTM! The changelog entry is well-formatted and clear.
The entry follows the established changelog conventions and accurately describes the PR's purpose of providing unique validation messages for scenario labels.
103-103: Entry is consistent with PR objectives.
The changelog entry accurately reflects the PR's goal of implementing unique validation messages for scenario labels, and is placed in the correct version section.
designer/client/src/components/toolbars/scenarioDetails/ScenarioLabels.tsx
Outdated
Show resolved
Hide resolved
…-validation-message-to-the-scenario-label # Conflicts: # docs/Changelog.md
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Outside diff range and nitpick comments (1)
designer/client/src/components/toolbars/scenarioDetails/ScenarioLabels.tsx (1)
29-34: LGTM! Consider adding type safety.The validation message implementation looks good and correctly uses i18next for translations as requested.
Consider adding type safety by explicitly declaring the return type:
-const labelUniqueValidation = (label: string) => ({ +const labelUniqueValidation = (label: string): ScenarioLabelValidationError => ({
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (2)
designer/client/src/components/toolbars/scenarioDetails/ScenarioLabels.tsx(3 hunks)docs/Changelog.md(1 hunks)
🔇 Additional comments (3)
designer/client/src/components/toolbars/scenarioDetails/ScenarioLabels.tsx (2)
135-140: LGTM! Good performance optimization.
The refactoring to useCallback is appropriate here since this function is used in other hooks' dependencies. The implementation correctly checks for duplicate labels.
164-167: 🛠️ Refactor suggestion
Consider handling potential race condition in validation.
While the duplicate check implementation is correct, there's a potential race condition between the HTTP validation response and the duplicate check. The duplicate validation might overwrite validation errors from the HTTP response if they arrive in a different order.
Consider this sequence:
- User types "label1"
- HTTP validation starts
- Duplicate check runs and sets errors
- HTTP validation completes and overwrites errors
To verify this behavior, we can check for similar patterns in the codebase:
Consider combining all validations:
return debounce(async (newInput: string) => {
if (newInput !== "") {
const response = await HttpService.validateScenarioLabels([newInput]);
+ const validationErrors = [...response.data.validationErrors];
+
+ if (isInputInSelectedOptions(newInput)) {
+ validationErrors.push(labelUniqueValidation(newInput));
+ }
- if (response.status === 200) {
- setInputErrors(response.data.validationErrors);
- }
+ setInputErrors(validationErrors);
}
-
- if (isInputInSelectedOptions(newInput)) {
- setInputErrors((prevState) => [...prevState, labelUniqueValidation(newInput)]);
- }
setInputTyping(false);
}, 500);docs/Changelog.md (1)
103-103: LGTM! Changelog entry is properly formatted and consistent with PR objectives.
The changelog entry accurately describes the PR's purpose of providing unique validation messages for scenario labels, and is correctly placed in the unreleased 1.18 section.
Dzuming
deleted the
NU-1889-provide-an-unique-validation-message-to-the-scenario-label
branch
Describe your changes
Checklist before merge
Summary by CodeRabbit
Release Notes
New Features
Bug Fixes
Documentation