[Nu-1882] change parameter parameter list item labels #7171
Conversation
WalkthroughThe pull request introduces modifications to the Cypress end-to-end tests for a fragment feature, enhancing validation and error handling for input parameters. It updates error messages and expands test cases to cover various input modes. Additionally, new props for customizable labels are added to components related to user-defined lists, improving user interface clarity. The changes maintain existing logic while enhancing the robustness and clarity of the tests and components involved. Changes
Possibly related PRs
Suggested labels
Suggested reviewers
📜 Recent review detailsConfiguration used: CodeRabbit UI 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous 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
CodeRabbit Configuration File (
|
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Outside diff range and nitpick comments (6)
designer/client/src/components/graph/node-modal/fragment-input-definition/settings/variants/fields/FixedValuesSetting.tsx (1)
19-19: Consider making userDefinedListInputLabel optionalMaking this a required prop might be a breaking change for existing implementations. Consider marking it as optional with
?unless it's absolutely necessary to have a label.- userDefinedListInputLabel: string; + userDefinedListInputLabel?: string;designer/client/src/components/graph/node-modal/fragment-input-definition/settings/variants/StringBooleanVariants/FixedListVariant.tsx (1)
Line range hint
14-21: AdduserDefinedListInputLabelto Props interfaceThe
userDefinedListInputLabelprop is being passed toFixedValuesSettingbut is not defined in thePropsinterface. This should be added to maintain type safety.Apply this diff to fix the interface:
interface Props { item: FixedListParameterVariant; onChange: (path: string, value: onChangeType) => void; path: string; readOnly: boolean; variableTypes: VariableTypes; errors: NodeValidationError[]; + userDefinedListInputLabel?: string; }designer/client/src/components/graph/node-modal/fragment-input-definition/settings/variants/StringBooleanVariants/AnyValueWithSuggestionVariant.tsx (1)
Line range hint
31-75: Consider component optimization opportunities.While the component is well-structured, consider these improvements:
- Split the component into smaller sub-components for better maintainability (e.g., separate components for fixed values, initial value, hint text sections)
- Use
React.memooruseMemofor performance optimization if the component re-renders frequently with the same propsExample refactor for splitting components:
// HintTextSection.tsx const HintTextSection: React.FC<{ hintText: string; onChange: (path: string, value: string) => void; path: string; readOnly: boolean; }> = ({ hintText, onChange, path, readOnly }) => { const { t } = useTranslation(); return ( <FormControl> <SettingLabelStyled>{t("fragment.hintText", "Hint text:")}</SettingLabelStyled> <TextAreaNode value={hintText} onChange={(e) => onChange(`${path}.hintText`, e.currentTarget.value)} style={{ width: "70%" }} disabled={readOnly} className={nodeInput} /> </FormControl> ); };designer/client/src/components/graph/node-modal/fragment-input-definition/settings/variants/fields/UserDefinedListInput.tsx (2)
171-171: Consider making the default label more genericThe current fallback uses "Add list item:" which might be too specific for a generic input label component. Consider using a more generic translation key and default text that better reflects the component's reusable nature.
-<SettingLabelStyled>{inputLabel || t("fragment.addListItem", "Add list item:")}</SettingLabelStyled> +<SettingLabelStyled>{inputLabel || t("common.input.list.label", "List input:")}</SettingLabelStyled>
Line range hint
1-204: Consider enhancing TypeScript type safetyThe component could benefit from stricter TypeScript type definitions:
- Consider using strict type checking for the validation errors
- Add proper typing for the AceEditor ref callback
- Define explicit return types for callbacks
Example improvements:
// Add explicit typing for validation errors type ValidationError = { errorType: string; fieldName: FieldName; typ: string; description: string; message: string; }; // Add proper typing for the AceEditor ref const editorRef = (ref: AceEditor | null): void => { if (ref?.editor) { ref.editor.commands.addCommand(aceEditorEnterCommand); } }; // Add return type for callbacks const handleAddNewListItem = (): void => { // ... existing implementation };designer/client/cypress/e2e/fragment.cy.ts (1)
Line range hint
1-507: Consider improving test organization and maintainabilityWhile the tests are comprehensive and well-written, consider these improvements to enhance maintainability:
- Break down the large test case into smaller, focused test cases using
describeblocks- Extract common setup steps into
beforeEachhooks- Consider using custom commands for repetitive UI interactions
- Add explicit wait conditions instead of force-clicking elements
Example refactor for better test organization:
describe("Fragment", () => { + describe("Input Parameters", () => { + beforeEach(() => { + cy.visitNewFragment(seed, "fragment"); + cy.get("[model-id=input]").dblclick(); + }); + + it("should handle String Any Value input mode", () => { + // Test specific to String Any Value + }); + + it("should handle String Any with Suggestion Value input mode", () => { + // Test specific to String Any with Suggestion + }); + }); + + describe("Fragment Properties", () => { + // Group property-related tests + }); });
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (5)
designer/client/cypress/e2e/fragment.cy.ts(1 hunks)designer/client/src/components/graph/node-modal/fragment-input-definition/settings/variants/StringBooleanVariants/AnyValueWithSuggestionVariant.tsx(1 hunks)designer/client/src/components/graph/node-modal/fragment-input-definition/settings/variants/StringBooleanVariants/FixedListVariant.tsx(1 hunks)designer/client/src/components/graph/node-modal/fragment-input-definition/settings/variants/fields/FixedValuesSetting.tsx(3 hunks)designer/client/src/components/graph/node-modal/fragment-input-definition/settings/variants/fields/UserDefinedListInput.tsx(3 hunks)
🔇 Additional comments (7)
designer/client/src/components/graph/node-modal/fragment-input-definition/settings/variants/fields/FixedValuesSetting.tsx (2)
34-34: LGTM!
The new parameter is correctly destructured and follows the existing code style.
52-52: Verify prop name consistency with UserDefinedListInput component
The prop name inputLabel differs from the parameter name userDefinedListInputLabel. Let's verify that the UserDefinedListInput component correctly accepts this prop.
✅ Verification successful
Prop name is correctly defined and used in UserDefinedListInput component
The inputLabel prop is properly defined as an optional prop in the Props interface and correctly used within the component. The code shows:
- Props interface includes
inputLabel?: string - Component destructures and uses the prop in the JSX
- The prop is used as a fallback with the translation:
{inputLabel || t("fragment.addListItem", "Add list item:")}
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for UserDefinedListInput Props interface definition
ast-grep --pattern 'interface Props {
$$$
inputLabel$_
$$$
}'
# Alternatively, search for the component definition
ast-grep --pattern 'function UserDefinedListInput($_) {
$$$
}'
Length of output: 136
Script:
#!/bin/bash
# Let's try to find the UserDefinedListInput component file first
fd UserDefinedListInput
# Then search for prop definitions or usages with ripgrep for broader coverage
rg "UserDefinedListInput.*Props" -A 5
rg "interface.*Props.*UserDefinedListInput" -A 5
Length of output: 240
Script:
#!/bin/bash
# Now that we found the file, let's examine its contents
cat designer/client/src/components/graph/node-modal/fragment-input-definition/settings/variants/fields/UserDefinedListInput.tsx
Length of output: 8754
designer/client/src/components/graph/node-modal/fragment-input-definition/settings/variants/StringBooleanVariants/FixedListVariant.tsx (1)
46-46: LGTM! Well-structured internationalization implementation.
The translation implementation follows best practices:
- Uses a descriptive hierarchical key:
fragment.userDefinedList.label.possibleValues - Provides a clear fallback text
- Aligns with the PR objective of fixing parameter list item labels
designer/client/src/components/graph/node-modal/fragment-input-definition/settings/variants/StringBooleanVariants/AnyValueWithSuggestionVariant.tsx (1)
47-47: LGTM! Verify translation key existence.
The addition of the internationalized label prop follows React best practices and improves UI clarity.
Let's verify the translation key exists and is consistent across the application:
designer/client/src/components/graph/node-modal/fragment-input-definition/settings/variants/fields/UserDefinedListInput.tsx (2)
30-30: LGTM! Props interface change is well-structured
The addition of the optional inputLabel prop follows TypeScript best practices and maintains backward compatibility.
43-43: LGTM! Parameter destructuring is consistent
The addition of inputLabel to the destructured parameters maintains code style consistency and aligns with the Props interface.
designer/client/cypress/e2e/fragment.cy.ts (1)
Line range hint 69-74: LGTM: Error message update maintains test coverage
The change from "Add list item" to "Suggested values" in the error message validation is consistent with the PR objectives to fix parameter list item labels. The test structure properly validates both the presence and content of the error message.
Let's verify the consistency of this error message across the codebase:
✅ Verification successful
The test update correctly reflects the UI label change
The change from "Add list item" to "Suggested values" in the test is consistent with the actual implementation. The new label is properly defined in the translation system and used in AnyValueWithSuggestionVariant component with the key fragment.userDefinedList.label.suggestedValues. The test correctly validates this UI element's visibility and content.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for other occurrences of the old and new error messages
# to ensure consistent messaging across the application
echo "Checking for old 'Add list item' message..."
rg -i "add list item" --type ts
echo "Checking for new 'Suggested values' message..."
rg -i "suggested values" --type ts
Length of output: 1306
Script:
#!/bin/bash
# Let's check the actual implementation to understand the context better
# and verify if the message change is intentional and consistent
# Check the implementation of AnyValueWithSuggestionVariant
ast-grep --pattern 'const AnyValueWithSuggestionVariant = $_'
# Check translation keys usage
rg -l "fragment\.userDefinedList\.label\.suggestedValues" --type ts
# Check if there are any other similar list-related components
fd UserDefinedList --type f --extension tsx --extension ts
Length of output: 19544
Describe your changes
Checklist before merge
Summary by CodeRabbit
New Features
Bug Fixes
Documentation