[Nu-7231] Add possibility for generic types in fragment input definition #7242
Conversation
DeamonDev
force-pushed
the
nu-7231-fragment-input-definitions-second-phase
branch
from
e0e7664 to
2127b3d
Compare
|
created: #7266 |
DeamonDev
changed the title
DeamonDev
force-pushed
the
nu-7231-fragment-input-definitions-second-phase
branch
from
0e77a13 to
d0308b5
Compare
📝 Walkthrough📝 WalkthroughWalkthroughThe pull request introduces several changes primarily focused on enhancing the functionality and testing of the "Fragment" feature within the application. A new test case has been added to the Cypress end-to-end testing suite to handle a new input parameter type, specifically "generic_type" with the format "List[String]". This test simulates user interactions to ensure the input is processed correctly and includes additional assertions for visibility and correctness. In the A new function, Possibly related PRs
Suggested labels
Suggested reviewers
📜 Recent review detailsConfiguration used: CodeRabbit UI ⛔ Files ignored due to path filters (1)
📒 Files selected for processing (7)
🚧 Files skipped from review as they are similar to previous changes (7)
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 (
|
![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 (8)
scenario-compiler/src/main/scala/pl/touk/nussknacker/engine/definition/fragment/FragmentParameterTypingParser.scala (1)
Line range hint
8-24: Consider implementing a proper parser for better generic type handling.While the current regex-based approach is functional and accepted (as per previous discussions), given that this PR aims to enhance generic type support, it might be worth considering implementing the TODO suggestion for a proper AST-based parser. This would enable:
- Handling of nested generic types (e.g.,
Map[String, List[Int]])- More robust parsing of complex type expressions
- Better error reporting for malformed type expressions
Some suggestions for future implementation:
- Consider using parser combinators (e.g., Scala's
scala-parser-combinatorslibrary)- Build a proper AST for type expressions
- Implement a visitor pattern for type resolution
Would you like me to create a GitHub issue to track this technical debt?
designer/client/src/components/graph/node-modal/fragment-input-definition/TypeSelect.tsx (3)
Line range hint
71-71: Fix value handling in onChange handlerThe current implementation discards custom input by returning an empty string when the option is a string type. This prevents users from entering custom generic types.
-onChange={(option) => onChange(typeof option === "string" ? "" : option.value)} +onChange={(option) => onChange(typeof option === "string" ? option : option.value)}
Line range hint
64-104: Add validation for generic type inputsConsider adding validation to ensure custom type inputs follow the correct format (e.g.,
List[String],Map[String, Int]). This will prevent invalid type definitions from being created.<CreatableSelect {...props} + isValidNewOption={(inputValue) => { + const genericTypeRegex = /^[A-Z][a-zA-Z0-9]*(\[[A-Z][a-zA-Z0-9]*(\s*,\s*[A-Z][a-zA-Z0-9]*)?\])?$/; + return genericTypeRegex.test(inputValue); + }} + formatCreateLabel={(inputValue) => `Use custom type: ${inputValue}`} {...rest} />
Line range hint
38-38: Add documentation for generic types supportPlease add JSDoc comments to document:
- The format expected for generic type inputs
- Examples of valid generic types
- Any limitations or restrictions
+/** + * TypeSelect component that supports both predefined and custom generic types. + * Custom types should follow the format: SimpleType or GenericType[TypeParam] + * Examples: + * - String + * - List[String] + * - Map[String, Int] + */ interface RowSelectProps extends Omit<HTMLProps<HTMLSelectElement>, "value" | "options" | "onBlur" | "onChange"> {designer/client/src/components/graph/node-modal/fragment-input-definition/item/Item.tsx (2)
29-33: Consider handling edge cases in resolveSimpleClassNameWhile the function works for the happy path, it should handle edge cases to be more robust.
Consider this improved implementation:
function resolveSimpleClassName(className: string): string { + if (!className) return ""; const parts = className.split("."); - return parts[parts.length - 1]; + return parts[parts.length - 1] || className; }
Line range hint
16-28: Consider enhancing TypeScript types for generic parametersSince we're adding support for generic types in fragment input definitions, consider updating the TypeScript interfaces to properly type these generic parameters.
Some suggestions:
- Add JSDoc comments explaining generic type support in the
FragmentInputParameterinterface- Consider creating a dedicated type for generic parameter definitions
- Add type guards or validation utilities for generic type syntax
designer/client/cypress/e2e/fragment.cy.ts (1)
161-165: Consider enhancing generic type test coverageThe test case for generic type input is well-integrated with the existing test suite. However, consider enhancing the test coverage:
- Add validation checks for the generic type format
- Verify the persistence of the generic type after saving
- Include negative test cases for invalid generic type formats
- Test different generic type variations (e.g.,
Map[String, Int],Option[T])Example addition:
cy.get("[data-testid='fieldsRow:8']").find("[placeholder='Field name']").type("generic_type"); cy.get("[data-testid='fieldsRow:8']").contains("String").click().type("List[String]{enter}"); + +// Verify generic type persistence +cy.contains(/^apply$/i).click(); +cy.contains(/^save\*$/i).click(); +cy.contains(/^ok$/i).click(); +cy.get("[model-id=input]").trigger("dblclick"); +cy.get("[data-testid='fieldsRow:8']").should("contain", "List[String]"); + +// Test invalid generic type format +cy.get("@window").contains("+").click(); +cy.get("[data-testid='fieldsRow:9']").find("[placeholder='Field name']").type("invalid_generic"); +cy.get("[data-testid='fieldsRow:9']").contains("String").click().type("List<String>{enter}"); +cy.get("[data-testid='fieldsRow:9']").should("contain", "Invalid generic type format");scenario-compiler/src/test/scala/pl/touk/nussknacker/engine/compile/NodeDataValidatorSpec.scala (1)
1163-1192: LGTM! Consider adding more test coverage.The test case correctly validates the error handling for invalid generic types. Consider adding more test cases to cover:
- Nested generic types (e.g.,
List[Map[String, Int]])- Multiple type parameters (e.g.,
Either[String, Int])- Bounded type parameters (e.g.,
List[T <: Number])
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (6)
designer/client/cypress/e2e/fragment.cy.ts(1 hunks)designer/client/src/components/graph/node-modal/fragment-input-definition/TypeSelect.tsx(2 hunks)designer/client/src/components/graph/node-modal/fragment-input-definition/item/Item.tsx(3 hunks)designer/client/src/components/graph/node-modal/fragment-input-definition/item/types.ts(1 hunks)scenario-compiler/src/main/scala/pl/touk/nussknacker/engine/definition/fragment/FragmentParameterTypingParser.scala(1 hunks)scenario-compiler/src/test/scala/pl/touk/nussknacker/engine/compile/NodeDataValidatorSpec.scala(1 hunks)
🧰 Additional context used
📓 Learnings (1)
scenario-compiler/src/main/scala/pl/touk/nussknacker/engine/definition/fragment/FragmentParameterTypingParser.scala (1)
Learnt from: DeamonDev
PR: TouK/nussknacker#7167
File: scenario-compiler/src/main/scala/pl/touk/nussknacker/engine/definition/fragment/FragmentParameterTypingParser.scala:26-28
Timestamp: 2024-11-20T12:20:09.673Z
Learning: In the `FragmentParameterTypingParser`, it's acceptable to use shallow parsing with regex patterns, even if it doesn't handle nested generic types, as the team is currently okay with this approach.
🔇 Additional comments (7)
scenario-compiler/src/main/scala/pl/touk/nussknacker/engine/definition/fragment/FragmentParameterTypingParser.scala (1)
26-26: LGTM! The whitespace handling improvement is a good addition.
The updated regex pattern now properly handles optional whitespace after the comma in Map type definitions, making it more flexible with different formatting styles while maintaining the existing functionality.
designer/client/src/components/graph/node-modal/fragment-input-definition/item/types.ts (1)
84-84:
Simplify type checking logic and add generic type support
The current implementation has several issues:
- It's unnecessarily verbose and less performant as it creates an array of boolean results and performs an additional
includescheck - It doesn't support generic types (e.g., "List[String]") mentioned in the PR objectives
Consider this simpler and more extensible implementation:
- return [item.typ.refClazzName === "String", item.typ.refClazzName === "Boolean", item.typ.refClazzName === "Long"].includes(true);
+ const { refClazzName } = item.typ;
+ // Direct primitive type check
+ if (["String", "Boolean", "Long"].includes(refClazzName)) {
+ return true;
+ }
+ // Generic type check (e.g., "List[String]")
+ const genericMatch = refClazzName.match(/^(\w+)\[(String|Boolean|Long)\]$/);
+ return !!genericMatch;This implementation:
- Simplifies the primitive type check using a single
includes - Adds support for generic types like "List[String]"
- Is more maintainable and easier to extend with additional type patterns
Let's verify the current usage of generic types in the codebase:
designer/client/src/components/graph/node-modal/fragment-input-definition/TypeSelect.tsx (2)
2-2: LGTM: Appropriate component selection for generic types support
The switch to CreatableSelect is a good choice as it enables users to input custom generic types while maintaining the ability to select from predefined options.
Line range hint 1-104: Verify test coverage for generic types functionality
Please ensure adequate test coverage for:
- Custom type input validation
- Generic type format handling
- Edge cases with complex generic types
designer/client/src/components/graph/node-modal/fragment-input-definition/item/Item.tsx (2)
45-45: LGTM! Clean implementation of simple class name resolution
The change correctly implements simple class name display for both label and value, maintaining consistency with the backward compatibility requirements.
79-79: Verify error path structure with backend validation
The error path has been updated to be more specific to each parameter. Let's verify that this matches the backend validation structure.
✅ Verification successful
The search results show that the error path pattern $param.{paramName}.$typ is consistently used in the backend codebase, particularly in validation-related files. The test files (UIProcessValidatorSpec.scala and BaseFlowTest.scala) and the API endpoint file (MigrationApiEndpoints.scala) all demonstrate the same pattern being used for parameter type validation errors.
Error path structure matches backend validation pattern
The updated error path $param.${item.name}.$typ in the frontend code aligns perfectly with the backend's validation structure, as evidenced by multiple occurrences of the same pattern in the backend codebase.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for validation error path patterns in backend code
rg -A 2 '\$param\.\$typ|\$param\.[^.]+\.\$typ'
Length of output: 1258
scenario-compiler/src/test/scala/pl/touk/nussknacker/engine/compile/NodeDataValidatorSpec.scala (1)
1134-1161: LGTM! Well-structured test for valid generic type.
The test case properly validates that a Map[String, Double] is accepted as a valid generic type in FragmentInputDefinition parameter.
DeamonDev
force-pushed
the
nu-7231-fragment-input-definitions-second-phase
branch
from
d0308b5 to
1924ecd
Compare
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Outside diff range and nitpick comments (4)
designer/client/src/components/graph/node-modal/fragment-input-definition/TypeSelect.tsx (2)
Line range hint
71-72: Improve type safety in the onChange handler.The current implementation could be more type-safe and handle edge cases better.
Consider this improvement:
-onChange={(option) => onChange(typeof option === "string" ? "" : option.value)} +onChange={(option) => { + if (!option) return onChange(""); + if (typeof option === "string") return onChange(option); + return onChange(option.value); +}}
Line range hint
31-41: Update RowSelectProps interface for CreatableSelect.The interface should be adjusted to better match CreatableSelect's expectations.
Consider updating the interface:
interface RowSelectProps extends Omit<HTMLProps<HTMLSelectElement>, "value" | "options" | "onBlur" | "onChange"> { onChange: (value: string) => void; onBlur?: (value: string) => void; options: Option[]; readOnly?: boolean; isMarked?: boolean; - value: Option; + value: Option | null; placeholder?: string; fieldErrors?: FieldError[]; }designer/client/cypress/e2e/fragment.cy.ts (2)
161-164: Add assertions to verify generic type handlingWhile the test successfully adds a generic type parameter, it would be more robust with explicit assertions to verify:
- The type is correctly saved
- The UI properly displays the generic type
- The type selector accepts and validates the generic type format
Consider adding these assertions after line 164:
cy.get("[data-testid='fieldsRow:8']").contains("String").click().type("List[String]{enter}"); +// Verify the type is correctly saved +cy.get("[data-testid='fieldsRow:8']").find("[aria-label='type-select']").should("have.value", "List[String]"); +// Verify the UI displays the generic type +cy.get("[data-testid='fieldsRow:8']").contains("List[String]").should("be.visible");
161-164: Consider adding test cases for error handlingTo ensure robust generic type support, consider adding test cases for:
- Invalid generic type formats (e.g., "List[" or "List]")
- Nested generic types (e.g., "List[List[String]]")
- Error messages for invalid formats
Example test cases to add:
// Test invalid generic type format cy.get("@window").contains("+").click(); cy.get("[data-testid='fieldsRow:9']").find("[placeholder='Field name']").type("invalid_generic"); cy.get("[data-testid='fieldsRow:9']").contains("String").click().type("List[{enter}"); // Verify error message cy.get("[data-testid='fieldsRow:9']").find("[data-testid='form-helper-text']").should("contain", "Invalid generic type format"); // Test nested generic type cy.get("@window").contains("+").click(); cy.get("[data-testid='fieldsRow:10']").find("[placeholder='Field name']").type("nested_generic"); cy.get("[data-testid='fieldsRow:10']").contains("String").click().type("List[List[String]]{enter}"); // Verify nested type is accepted cy.get("[data-testid='fieldsRow:10']").find("[aria-label='type-select']").should("have.value", "List[List[String]]");
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (6)
designer/client/cypress/e2e/fragment.cy.ts(1 hunks)designer/client/src/components/graph/node-modal/fragment-input-definition/TypeSelect.tsx(2 hunks)designer/client/src/components/graph/node-modal/fragment-input-definition/item/Item.tsx(3 hunks)designer/client/src/components/graph/node-modal/fragment-input-definition/item/types.ts(1 hunks)scenario-compiler/src/main/scala/pl/touk/nussknacker/engine/definition/fragment/FragmentParameterTypingParser.scala(1 hunks)scenario-compiler/src/test/scala/pl/touk/nussknacker/engine/compile/NodeDataValidatorSpec.scala(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (3)
- designer/client/src/components/graph/node-modal/fragment-input-definition/item/Item.tsx
- designer/client/src/components/graph/node-modal/fragment-input-definition/item/types.ts
- scenario-compiler/src/main/scala/pl/touk/nussknacker/engine/definition/fragment/FragmentParameterTypingParser.scala
🔇 Additional comments (5)
designer/client/src/components/graph/node-modal/fragment-input-definition/TypeSelect.tsx (2)
Line range hint 64-74: LGTM! The CreatableSelect implementation aligns with the PR objective.
The switch to CreatableSelect successfully enables users to input custom generic types, which is the main goal of this PR.
Line range hint 70-70: Consider validating empty value handling.
The value={value || ""} expression might need validation to ensure proper handling of null/undefined cases.
Let's verify the value handling in the codebase:
✅ Verification successful
The null/undefined handling is already properly implemented
The TypeSelect component already has proper handling for null/undefined cases:
- The
valueprop is typed asOptionin the interface, making it a required prop that must be provided - The
value={value || ""}expression is used within the CreatableSelect component which is designed to handle empty string values correctly - The component includes proper type checking in the onChange handler:
typeof option === "string" ? "" : option.value - The component is used with a ValidationLabels component that can display field errors if validation fails
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for TypeSelect usage to understand value handling patterns
rg -A 5 "TypeSelect" --type typescript
Length of output: 73
Script:
#!/bin/bash
# Search for TypeSelect usage and implementation with ripgrep
rg -l "TypeSelect"
# Get the content of TypeSelect component
cat designer/client/src/components/graph/node-modal/fragment-input-definition/TypeSelect.tsx
Length of output: 5203
designer/client/cypress/e2e/fragment.cy.ts (1)
161-164: Implementation looks good!
The test case follows the established patterns and successfully verifies the basic functionality of generic type support in fragment input definition.
scenario-compiler/src/test/scala/pl/touk/nussknacker/engine/compile/NodeDataValidatorSpec.scala (2)
1134-1161: LGTM! Well-structured test for valid generic type.
The test case effectively verifies that valid generic types (e.g., Map[String, Double]) are accepted in FragmentInputDefinition parameters. The test structure and assertions are clear and follow the established patterns.
1163-1192: LGTM! Good negative test case for invalid generic type.
The test case properly verifies that invalid generic types (using non-existent types) are rejected with appropriate error messages. The test complements the positive test case and ensures robust type validation.
|
@DeamonDev is it a correct Jira number? I can't find it |
No, but it is correct issue number: #7231 |
designer/client/src/components/graph/node-modal/fragment-input-definition/item/Item.tsx
Outdated
Show resolved
Hide resolved
DeamonDev
force-pushed
the
nu-7231-fragment-input-definitions-second-phase
branch
from
126ced6 to
d561de9
Compare
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Outside diff range and nitpick comments (1)
designer/client/src/components/graph/node-modal/fragment-input-definition/item/utils.ts (1)
5-9: Add input validation and improve documentationWhile the function works for the basic case, it could benefit from better error handling and documentation.
Consider this improved implementation:
-//This projection is used for backward-compatibility reasons, since previously fragment input definition type options display part contained full class name +/** + * Extracts the simple class name from a fully qualified class name. + * Used for backward compatibility with legacy fragment input definition type options + * that displayed the full class name. + * + * @example + * resolveSimpleClassName("java.util.List") // returns "List" + * + * @param refClazzName - The fully qualified class name + * @returns The simple class name + * @throws If the input is empty or invalid + */ export function resolveSimpleClassName(refClazzName: string): string { + if (!refClazzName?.trim()) { + throw new Error("Class name cannot be empty"); + } const parts = refClazzName.split("."); - return parts[parts.length - 1]; + const simpleName = parts[parts.length - 1]; + if (!simpleName) { + throw new Error("Invalid class name format"); + } + return simpleName; }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (3)
designer/client/src/components/graph/node-modal/fragment-input-definition/item/Item.tsx(3 hunks)designer/client/src/components/graph/node-modal/fragment-input-definition/item/types.ts(2 hunks)designer/client/src/components/graph/node-modal/fragment-input-definition/item/utils.ts(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (2)
- designer/client/src/components/graph/node-modal/fragment-input-definition/item/Item.tsx
- designer/client/src/components/graph/node-modal/fragment-input-definition/item/types.ts
🔇 Additional comments (1)
designer/client/src/components/graph/node-modal/fragment-input-definition/item/utils.ts (1)
Line range hint 11-24: Verify the usage of resolveSimpleClassName
Let's verify that the new resolveSimpleClassName function is properly integrated with components consuming these utilities.
✅ Verification successful
Let me gather more specific information about the usage context.
No issues found with resolveSimpleClassName integration
The verification shows that resolveSimpleClassName is properly integrated in the codebase:
- It's used in
isPermittedTypeVariantfunction which validates parameter types getDefaultFieldsis correctly consumed byFragmentInputDefinition.tsxwhich handles the fragment input parameters- The type system ensures type safety through the
FragmentInputParameterinterface
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for usages of resolveSimpleClassName to ensure proper integration
rg -A 3 "resolveSimpleClassName" --type ts --type tsx
# Search for direct usage of refClazzName to ensure we haven't missed any places that should use resolveSimpleClassName
ast-grep --pattern 'refClazzName'
Length of output: 6445
Script:
#!/bin/bash
# Search for files that import and use getDefaultFields
rg -A 5 "getDefaultFields" --type ts
# Search for files that might be using the type information
rg -A 3 "FragmentInputParameter" --type ts
Length of output: 12178
DeamonDev
force-pushed
the
nu-7231-fragment-input-definitions-second-phase
branch
2 times, most recently
from
c42e24a to
0b7d6b5
Compare
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Outside diff range and nitpick comments (3)
designer/client/src/components/graph/node-modal/fragment-input-definition/TypeSelect.tsx (2)
Line range hint
64-102: Enhance type safety and error handlingThe implementation could benefit from more robust type handling and explicit fallbacks.
Consider these improvements:
- value={value || ""} - onChange={(option) => onChange(typeof option === "string" ? "" : option.value)} + value={value ?? { value: "", label: "" }} + onChange={(option) => { + if (!option) { + onChange(""); + return; + } + onChange(typeof option === "string" ? option : option.value); + }}This change:
- Provides a more explicit fallback for the value prop
- Handles null/undefined cases in onChange
- Preserves string input capability for custom values
Line range hint
102-103: Enhance error feedback accessibilityConsider adding ARIA attributes to improve screen reader feedback for validation errors.
- <ValidationLabels fieldErrors={fieldErrors} /> + <ValidationLabels + fieldErrors={fieldErrors} + aria-live="polite" + role="alert" + />scenario-compiler/src/test/scala/pl/touk/nussknacker/engine/compile/NodeDataValidatorSpec.scala (1)
1163-1192: LGTM! Consider adding a clarifying comment.The test case correctly verifies that invalid generic types are rejected. Consider adding a brief comment explaining that
Foois intentionally used as a non-existent type to test the error handling.test( "should not allow usage of generic type in FragmentInputDefinition parameter when occurring type is not on classpath" ) { + // Using 'Foo' as a non-existent type to verify that the validator correctly handles missing types in generic parameters val nodeId: String = "in" val paramName = "param1"
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
⛔ Files ignored due to path filters (1)
designer/client/cypress/e2e/__image_snapshots__/electron/Linux/Fragment should allow adding input parameters and display used fragment graph in modal #4.pngis excluded by!**/*.png
📒 Files selected for processing (7)
designer/client/cypress/e2e/fragment.cy.ts(2 hunks)designer/client/src/components/graph/node-modal/fragment-input-definition/TypeSelect.tsx(2 hunks)designer/client/src/components/graph/node-modal/fragment-input-definition/item/Item.tsx(3 hunks)designer/client/src/components/graph/node-modal/fragment-input-definition/item/types.ts(2 hunks)designer/client/src/components/graph/node-modal/fragment-input-definition/item/utils.ts(1 hunks)scenario-compiler/src/main/scala/pl/touk/nussknacker/engine/definition/fragment/FragmentParameterTypingParser.scala(1 hunks)scenario-compiler/src/test/scala/pl/touk/nussknacker/engine/compile/NodeDataValidatorSpec.scala(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (5)
- designer/client/cypress/e2e/fragment.cy.ts
- designer/client/src/components/graph/node-modal/fragment-input-definition/item/Item.tsx
- designer/client/src/components/graph/node-modal/fragment-input-definition/item/types.ts
- designer/client/src/components/graph/node-modal/fragment-input-definition/item/utils.ts
- scenario-compiler/src/main/scala/pl/touk/nussknacker/engine/definition/fragment/FragmentParameterTypingParser.scala
🔇 Additional comments (3)
designer/client/src/components/graph/node-modal/fragment-input-definition/TypeSelect.tsx (2)
2-2: LGTM: CreatableSelect import addition
The addition of CreatableSelect is appropriate for enabling user-defined generic types in fragment input definitions.
Line range hint 35-43: Verify all TypeSelect consumers are updated
The change of value prop type from string to Option in RowSelectProps interface is a breaking change that requires updates in all consuming components.
scenario-compiler/src/test/scala/pl/touk/nussknacker/engine/compile/NodeDataValidatorSpec.scala (1)
1134-1161: LGTM! Well-structured test for valid generic type.
The test case effectively verifies that valid generic types like Map[String, Double] are accepted in FragmentInputDefinition parameters.
DeamonDev
force-pushed
the
nu-7231-fragment-input-definitions-second-phase
branch
3 times, most recently
from
dedef4d to
1e0408d
Compare
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Outside diff range and nitpick comments (2)
designer/client/src/components/graph/node-modal/fragment-input-definition/TypeSelect.tsx (2)
Line range hint
64-102: Enhance error messaging for custom type validationThe current error handling is good but could be more specific for custom types. Consider adding descriptive error messages when users attempt to create invalid types.
<CreatableSelect ... + formatCreateLabel={(inputValue) => { + const typePattern = /^(List|Set)\[(String|Boolean|Int)\]$/; + if (!typePattern.test(inputValue)) { + return `Invalid format. Expected: List[Type] or Set[Type]`; + } + return `Create type "${inputValue}"`; + }} ... />
Line range hint
1-102: Consider implementing a type registry or validation serviceWhile the current implementation enables generic types, consider implementing a centralized type registry or validation service to:
- Maintain consistency in custom type definitions across the application
- Provide reusable type validation logic
- Enable type suggestions based on existing usage patterns
This could be implemented as a separate service that:
- Maintains a registry of valid custom types
- Provides validation rules and type suggestions
- Ensures consistency across different parts of the application
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
⛔ Files ignored due to path filters (1)
designer/client/cypress/e2e/__image_snapshots__/electron/Linux/Fragment should allow adding input parameters and display used fragment graph in modal #4.pngis excluded by!**/*.png
📒 Files selected for processing (7)
designer/client/cypress/e2e/fragment.cy.ts(2 hunks)designer/client/src/components/graph/node-modal/fragment-input-definition/TypeSelect.tsx(2 hunks)designer/client/src/components/graph/node-modal/fragment-input-definition/item/Item.tsx(3 hunks)designer/client/src/components/graph/node-modal/fragment-input-definition/item/types.ts(2 hunks)designer/client/src/components/graph/node-modal/fragment-input-definition/item/utils.ts(1 hunks)scenario-compiler/src/main/scala/pl/touk/nussknacker/engine/definition/fragment/FragmentParameterTypingParser.scala(1 hunks)scenario-compiler/src/test/scala/pl/touk/nussknacker/engine/compile/NodeDataValidatorSpec.scala(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (6)
- designer/client/cypress/e2e/fragment.cy.ts
- designer/client/src/components/graph/node-modal/fragment-input-definition/item/Item.tsx
- designer/client/src/components/graph/node-modal/fragment-input-definition/item/types.ts
- designer/client/src/components/graph/node-modal/fragment-input-definition/item/utils.ts
- scenario-compiler/src/main/scala/pl/touk/nussknacker/engine/definition/fragment/FragmentParameterTypingParser.scala
- scenario-compiler/src/test/scala/pl/touk/nussknacker/engine/compile/NodeDataValidatorSpec.scala
🔇 Additional comments (2)
designer/client/src/components/graph/node-modal/fragment-input-definition/TypeSelect.tsx (2)
2-2: LGTM: Import change aligns with feature requirements
The switch to CreatableSelect is appropriate for enabling user-defined generic types in fragment input definitions.
Line range hint 64-102: Verify type creation behavior and add input validation
While the implementation is generally good, consider these improvements:
- The onChange handler returns an empty string when option is a string type. Verify if this is the intended behavior for custom type creation.
- Consider adding validation for user-created types to ensure they follow the expected format (e.g., "List[String]").
- Add a placeholder or helper text to guide users on the expected format for custom types.
Consider adding type validation:
<CreatableSelect
...
+ formatCreateLabel={(inputValue) => `Create type "${inputValue}"`}
+ isValidNewOption={(inputValue) => {
+ const typePattern = /^(List|Set)\[(String|Boolean|Int)\]$/;
+ return typePattern.test(inputValue);
+ }}
+ placeholder="Select or enter type (e.g., List[String])"
...
/>
DeamonDev
force-pushed
the
nu-7231-fragment-input-definitions-second-phase
branch
from
1e0408d to
e109cb3
Compare
DeamonDev
deleted the
nu-7231-fragment-input-definitions-second-phase
branch
Co-authored-by: Piotr Rudnicki <[email protected]>
Co-authored-by: Piotr Rudnicki <[email protected]>
Co-authored-by: Piotr Rudnicki <[email protected]>
Co-authored-by: Piotr Rudnicki <[email protected]>
Co-authored-by: Piotr Rudnicki <[email protected]>
Describe your changes
I changed
<Select>component inTypeSelectisnideFragmentInputDefinitionto<CreatableSelect>which allows to type own type:Checklist before merge
Summary by CodeRabbit
New Features
TypeSelectcomponent to allow users to create new options in addition to selecting existing ones.Bug Fixes
TypeSelectcomponent to provide more specific error messages.Tests