1.18 ports7 #7221
1.18 ports7 #7221
Conversation
Revert to shorter tooltip for labels
… enter until validation finish (#7215) * NU-1888 change typing text to spinner and provide delayed adding on enter --------- Co-authored-by: github-actions[bot] <41898282+github-actions[bot]@users.noreply.github.com> Co-authored-by: Dzuming <[email protected]>
📝 WalkthroughWalkthroughThe pull request introduces several modifications across various components in the Nussknacker application, primarily focusing on enhancing type flexibility and improving user interaction. The Additionally, the Overall, these changes reflect a concerted effort to improve the handling of validation information, user feedback during input, and the overall structure and type definitions within the components. Possibly related PRs
Suggested labels
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: 1
🧹 Outside diff range and nitpick comments (10)
designer/client/src/components/toolbars/scenarioDetails/useDelayedEnterAction.tsx (2)
3-7: Add JSDoc documentation to the Props interface.Adding documentation would improve code maintainability and help other developers understand the purpose of each prop.
+/** + * Props for the useDelayedEnterAction hook + */ interface Props { + /** Indicates if the user is currently typing */ inputTyping: boolean; + /** Number of validation errors */ errorsLength: number; + /** Callback function to execute when conditions are met */ action: () => void; }
9-9: Consider renaming the hook for better clarity.The current name
useDelayedEnterActionsuggests it specifically handles Enter key actions, but it's actually a generic conditional action hook. Consider renaming it to better reflect its purpose, such asuseConditionalActionoruseValidatedAction.designer/client/src/components/graph/node-modal/editors/EditableEditor.tsx (1)
Line range hint
31-49: Consider memoizing Editor component selectionThe Editor component selection logic could benefit from memoization to prevent unnecessary recalculations.
Consider this optimization:
-const editorType = useMemo(() => (isEmpty(param) ? EditorType.RAW_PARAMETER_EDITOR : param.editor.type), [param]); - -const Editor: SimpleEditor | ExtendedEditor = useMemo(() => editors[editorType], [editorType]); +const Editor: SimpleEditor | ExtendedEditor = useMemo(() => { + const type = isEmpty(param) ? EditorType.RAW_PARAMETER_EDITOR : param.editor.type; + return editors[type]; +}, [param]);designer/client/src/components/graph/node-modal/fragment-input-definition/settings/variants/fields/UserDefinedListInput.tsx (2)
Line range hint
104-143: Consider extracting validation logic.The
handleAddNewListItemfunction handles multiple responsibilities: enter key state, validation checks, and list updates. Consider extracting the validation logic into a separate function for better maintainability.+const validateNewListItem = (temporaryListItem: string, fixedValuesList: FixedValuesOption[]): NodeValidationError[] => { + const errors: NodeValidationError[] = []; + if (!mandatoryValueValidator.isValid(temporaryListItem)) { + errors.push({ + errorType: "SaveAllowed", + fieldName: temporaryItemName, + typ: temporaryListItemTyp.refClazzName, + description: mandatoryValueValidator.description(), + message: mandatoryValueValidator.message(), + }); + } + + const isUniqueValueValidator = uniqueValueValidator(fixedValuesList.map(item => item.label)); + if (!isUniqueValueValidator.isValid(temporaryListItem)) { + errors.push({ + errorType: "SaveAllowed", + fieldName: temporaryItemName, + typ: temporaryListItemTyp.refClazzName, + description: isUniqueValueValidator.description(), + message: isUniqueValueValidator.message(), + }); + } + return errors; +}; const handleAddNewListItem = () => { if (temporaryValuesTyping) { setIsEnterPressed(true); return; } - const isUniqueValueValidator = uniqueValueValidator(fixedValuesList.map((fixedValuesList) => fixedValuesList.label)); - - if (!mandatoryValueValidator.isValid(temporaryListItem)) { - setTemporaryValueErrors((prevState) => [ - ...prevState, - { - errorType: "SaveAllowed", - fieldName: temporaryItemName, - typ: temporaryListItemTyp.refClazzName, - description: mandatoryValueValidator.description(), - message: mandatoryValueValidator.message(), - }, - ]); - return; - } - - if (!isUniqueValueValidator.isValid(temporaryListItem)) { - setTemporaryValueErrors((prevState) => [ - ...prevState, - { - errorType: "SaveAllowed", - fieldName: temporaryItemName, - typ: temporaryListItemTyp.refClazzName, - description: isUniqueValueValidator.description(), - message: isUniqueValueValidator.message(), - }, - ]); - return; - } + const validationErrors = validateNewListItem(temporaryListItem, fixedValuesList); + if (validationErrors.length > 0) { + setTemporaryValueErrors(validationErrors); + return; + } if (temporaryValueErrors.length === 0) { const updatedList = [...fixedValuesList, { expression: temporaryListItem, label: temporaryListItem }]; handleChangeFixedValuesList(updatedList); setTemporaryListItem(""); } };
191-205: Consider adding error handling for editor initialization.The editor setup looks good, but consider adding error handling for cases where the editor initialization fails.
ref={(ref: AceEditor | null) => { + try { if (ref?.editor) { editorRef.current = ref.editor; ref.editor.commands.addCommand(aceEditorEnterCommand); } + } catch (error) { + console.error('Failed to initialize editor:', error); + } }}designer/client/src/components/toolbars/scenarioDetails/ScenarioLabels.tsx (1)
225-238: Consider simplifying keyboard event simulationThe current implementation creates and dispatches a synthetic keyboard event, which is a complex approach that might be fragile across different browsers.
Consider simplifying this by directly calling the Autocomplete's internal methods or using MUI's API:
- const { setIsEnterPressed } = useDelayedEnterAction({ - action: () => { - const enterEvent = new KeyboardEvent("keydown", { - key: "Enter", - keyCode: 13, - code: "Enter", - bubbles: true, - cancelable: true, - }); - autocompleteRef.current.dispatchEvent(enterEvent); - }, - errorsLength: inputErrors.length, - inputTyping, - }); + const { setIsEnterPressed } = useDelayedEnterAction({ + action: () => { + // Use MUI's Autocomplete ref methods directly + if (autocompleteRef.current) { + const api = autocompleteRef.current; + api.blur(); // or other appropriate method + } + }, + errorsLength: inputErrors.length, + inputTyping, + });docs/scenarios_authoring/Spel.md (1)
Line range hint
271-307: Consider adding error handling examples.While the conversion documentation is comprehensive, it would be helpful to include examples of how errors are handled in each case, especially for the
toprefix which propagates exceptions.Consider adding examples like:
| `'abc'.toDoubleOrNull` | null | Double | +| `'abc'.toDouble` | NumberFormatException | - | +| `'123.45'.to('Integer')` | NumberFormatException | - | | `'123'.canBe('Double')` | true | Boolean |docs/MigrationGuide.md (3)
13-13: Consider adding release date for version 1.19.0For better tracking and clarity, consider adding the expected release date or marking it as "Upcoming" for version 1.19.0.
Line range hint
13-19: Breaking change warning needed for Kafka state restorationThe breaking change regarding Kafka source/sink components and state restoration should be highlighted more prominently, perhaps with a warning box or bold text, as this could significantly impact production systems.
### ⚠️ Breaking Changes * We lost support for old ConsumerRecord constructor supported by Flink 1.14 / 1.15 * If you used Kafka source/sink components in your scenarios then state of these scenarios won't be restored
Line range hint
1-11: Add table of contents for better navigationFor a long migration guide like this, consider adding a table of contents at the beginning to help users quickly find relevant sections.
# Migration guide ## Table of Contents - [Version 1.19.0](#in-version-1190-not-released-yet) - [Version 1.18.0](#in-version-1180) - [Version 1.17.0](#in-version-1170) ... To see the biggest differences please consult the [changelog](Changelog.md).
📜 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 #1.pngis excluded by!**/*.png
📒 Files selected for processing (9)
designer/client/src/components/graph/node-modal/editors/EditableEditor.tsx(2 hunks)designer/client/src/components/graph/node-modal/editors/expression/Editor.ts(2 hunks)designer/client/src/components/graph/node-modal/fragment-input-definition/settings/variants/fields/UserDefinedListInput.tsx(5 hunks)designer/client/src/components/toolbars/scenarioDetails/ScenarioLabels.tsx(9 hunks)designer/client/src/components/toolbars/scenarioDetails/useDelayedEnterAction.tsx(1 hunks)designer/submodules/packages/components/src/common/labelChip.tsx(1 hunks)docs/Changelog.md(2 hunks)docs/MigrationGuide.md(1 hunks)docs/scenarios_authoring/Spel.md(3 hunks)
🔇 Additional comments (19)
designer/client/src/components/toolbars/scenarioDetails/useDelayedEnterAction.tsx (1)
12-17: Verify the action execution timing.
The current implementation might execute the action multiple times if the dependencies change rapidly. Consider implementing debouncing to prevent this.
designer/submodules/packages/components/src/common/labelChip.tsx (2)
31-31: Consider UX impact of simplified tooltip
The change from a dynamic tooltip that showed the full label value to a static "Label" text might reduce user experience, especially for:
- Users relying on tooltips for additional context
- Screen reader users who benefit from more descriptive tooltips
- Cases where the label text is truncated and tooltip provided the full text
Let's check if this change is consistent with other similar components:
Consider one of these alternatives:
- title={t("scenariosList.tooltip.label", "Label")}
+ title={t("scenariosList.tooltip.label", "Scenario label: {{label}}", { label: value })}or at minimum:
- title={t("scenariosList.tooltip.label", "Label")}
+ title={value}Line range hint 31-38: Verify accessibility compliance
The component uses good accessibility practices with:
- Proper tabIndex
- Click handling
- Visual feedback through color changes
However, the simplified tooltip might not provide sufficient context for screen readers.
Let's check other accessibility attributes in similar components:
Consider adding explicit aria attributes:
title={t("scenariosList.tooltip.label", "Label")}
key={id}
color={isSelected ? "primary" : "default"}
size="small"
variant={"outlined"}
label={value}
+ aria-label={`${t("scenariosList.tooltip.label", "Label")}: ${value}`}
tabIndex={0}
onClick={onClick}designer/client/src/components/graph/node-modal/editors/EditableEditor.tsx (2)
2-2: LGTM: Import change aligns with the type update.
The addition of ReactNode to the React import is necessary to support the Props interface change.
27-27:
Document this breaking change in MigrationGuide.md
Changing validationLabelInfo from string to ReactNode is a breaking change that affects the component's public API. While this change enables more flexible validation label rendering (e.g., progress indicators, rich text), it requires documentation to help users migrate.
Let's verify if this breaking change is documented:
Please ensure you:
- Document this change in MigrationGuide.md
- Update any string literals being passed to validationLabelInfo
- Update the PR checklist to confirm these steps are completed
designer/client/src/components/graph/node-modal/editors/expression/Editor.ts (2)
7-7: LGTM!
The import statement follows TypeScript best practices by explicitly importing the required type.
30-30: Breaking change detected: Document migration steps
Changing expressionInfo from string to ReactNode is a breaking change that allows for richer content rendering. This change requires documentation in the migration guide.
Let's verify the impact:
Please ensure you:
- Add an entry to
MigrationGuide.mdexplaining how to migrate string-basedexpressionInfotoReactNode - Update relevant documentation
- Review and update unit tests to cover ReactNode scenarios
designer/client/src/components/graph/node-modal/fragment-input-definition/settings/variants/fields/UserDefinedListInput.tsx (3)
6-6: LGTM: Import changes and constant definition are well-organized.
The new imports and constant definition are properly structured and necessary for the added functionality.
Also applies to: 18-21, 22-22
188-190: LGTM: Well-implemented loading indicator.
The loading indicator provides good user feedback during typing and follows Material-UI's theming conventions.
49-49: Verify editor command registration.
The delayed enter action executes the ENTER_VALUE_COMMAND on the editor. Ensure that this command is properly registered before it's executed.
Also applies to: 91-97
✅ Verification successful
Let me check the command registration implementation to ensure it's properly set up.
Editor command is properly registered and used
The command is well-implemented:
- Command constant
ENTER_VALUE_COMMANDis defined and consistently used - Command registration is done with proper binding to Enter key
- Command execution is correctly set up to handle new list items
- The same command name is used in both registration and execution
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for all places where editor commands are registered
rg -A 2 "commands\.addCommand"
Length of output: 937
Script:
#!/bin/bash
# Search for the command definition and its usage
ast-grep --pattern 'const aceEditorEnterCommand = {
$$$
}'
# Also check for any ENTER_VALUE_COMMAND constant definition
rg "ENTER_VALUE_COMMAND"
Length of output: 1850
designer/client/src/components/toolbars/scenarioDetails/ScenarioLabels.tsx (3)
8-8: LGTM: Import additions enhance component capabilities
The new imports support improved loading states and keyboard interactions:
- CircularProgress provides better visual feedback
- useRef enables DOM manipulation for keyboard events
- useDelayedEnterAction custom hook manages Enter key interactions
Also applies to: 19-19, 26-26
291-291: LGTM: Enhanced user feedback and i18n support
Good improvements:
- Loading indicator provides better visual feedback
- Label tooltip properly uses translation key
Also applies to: 377-377
321-321: Ensure comprehensive test coverage for error handling
The error handling logic is complex, covering multiple scenarios:
- Input validation states
- Duplicate label detection
- Loading states
- Keyboard interaction during errors
Let's verify the test coverage:
Would you like me to help generate additional test cases for these error handling scenarios?
Also applies to: 353-353
docs/scenarios_authoring/Spel.md (3)
271-275: LGTM! Clear introduction to type conversions.
The changes provide a clear and structured introduction to type conversions, properly distinguishing between implicit and explicit conversions. The addition of utility classes alongside built-in functions enhances completeness.
290-293: LGTM! Well-structured explanation of conversion functions.
The explanation of the different conversion function prefixes (canBe, to, toOrNull) is clear and precise. Each prefix's behavior is well-documented, helping users understand when to use each variant.
307-307: LGTM! Good example of LocalDateTime conversion.
The added example demonstrates LocalDateTime string parsing, which is a common use case. It complements the existing examples well.
docs/Changelog.md (2)
37-37: LGTM: Version release date added.
The version date has been properly updated to indicate the official release date.
127-127: LGTM: New changelog entry for UI improvement.
The new entry properly documents a UI enhancement that improves user feedback during validation by replacing static text with a spinner and handling delayed additions until validation completes.
docs/MigrationGuide.md (1)
Line range hint 21-186: LGTM: Version 1.18.0 section is well documented
The version 1.18.0 section is comprehensive and well-structured, covering:
- Configuration changes
- Code API changes
- REST API changes
- Other changes
The migration steps are clear and include code examples where needed.
| export const useDelayedEnterAction = ({ inputTyping, errorsLength, action }: Props) => { | ||
| const [isEnterPressed, setIsEnterPressed] = useState(false); | ||
|
|
||
| useEffect(() => { | ||
| if (isEnterPressed && !inputTyping && errorsLength === 0) { | ||
| action(); | ||
| setIsEnterPressed(false); | ||
| } | ||
| }, [errorsLength, inputTyping, isEnterPressed, action]); | ||
|
|
||
| return { setIsEnterPressed }; | ||
| }; |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Consider adding error handling and cleanup.
The current implementation could be improved in several ways:
- Add error handling around the action execution
- Include a cleanup function in useEffect
- Consider debouncing to prevent rapid repeated executions
Here's a suggested implementation:
export const useDelayedEnterAction = ({ inputTyping, errorsLength, action }: Props) => {
const [isEnterPressed, setIsEnterPressed] = useState(false);
useEffect(() => {
+ let mounted = true;
if (isEnterPressed && !inputTyping && errorsLength === 0) {
- action();
- setIsEnterPressed(false);
+ try {
+ Promise.resolve(action()).then(() => {
+ if (mounted) {
+ setIsEnterPressed(false);
+ }
+ });
+ } catch (error) {
+ console.error('Error executing delayed action:', error);
+ if (mounted) {
+ setIsEnterPressed(false);
+ }
+ }
}
+ return () => {
+ mounted = false;
+ };
}, [errorsLength, inputTyping, isEnterPressed, action]);
return { setIsEnterPressed };
};Committable suggestion skipped: line range outside the PR's diff.
|
created: #7222 |
Describe your changes
Checklist before merge
Summary by CodeRabbit
Release Notes
New Features
Improvements
ScenarioLabelsandUserDefinedListInputcomponents.Documentation