Sticky notes #7181
Sticky notes #7181
Conversation
📝 Walkthrough📝 WalkthroughWalkthroughThe pull request introduces extensive modifications to the Nussknacker application, primarily focusing on the implementation and management of sticky notes. Key changes include the addition of a new dependency, Several new functions are introduced in the Database schema migrations are implemented to create a Possibly related PRs
Suggested labels
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 (
|
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 16
🧹 Outside diff range and nitpick comments (72)
designer/client/src/types/stickyNote.ts (1)
1-1: Consider using a const assertion for better type safetyThe constant looks good, but you could make it more type-safe by using a const assertion:
-export const StickyNoteType = "StickyNote"; +export const StickyNoteType = "StickyNote" as const;designer/client/src/common/StickyNote.ts (1)
3-3: Add JSDoc documentation for the Dimensions type.Consider adding documentation to explain the purpose and usage of this type, including any constraints on width and height values.
+/** Represents the dimensions of a sticky note in pixels */ export type Dimensions = { width: number; height: number };designer/server/src/main/scala/pl/touk/nussknacker/ui/api/description/stickynotes/StickyNoteEvent.scala (1)
5-13: Add ScalaDoc documentation for better maintainability.The implementation looks good, but consider adding documentation to explain:
- The purpose of this enumeration
- When each event type is triggered
- How these events integrate with the sticky notes feature
+/** + * Represents events that can occur during sticky note lifecycle. + * Used for tracking state changes and triggering appropriate actions. + */ object StickyNoteEvent extends Enumeration { implicit val typeEncoder: Encoder[StickyNoteEvent.Value] = Encoder.encodeEnumeration(StickyNoteEvent) implicit val typeDecoder: Decoder[StickyNoteEvent.Value] = Decoder.decodeEnumeration(StickyNoteEvent) type StickyNoteEvent = Value + /** Emitted when a new sticky note is created */ val StickyNoteCreated: Value = Value("CREATED") + /** Emitted when an existing sticky note is modified */ val StickyNoteUpdated: Value = Value("UPDATED") + /** Emitted when a sticky note is removed */ val StickyNoteDeleted: Value = Value("DELETED")designer/client/src/reducers/graph/types.ts (1)
17-17: Consider adding JSDoc documentationThe addition of
stickyNotestoGraphStatelooks good. Consider adding JSDoc documentation to describe the purpose and usage of this state property, especially since it's an optional field.+ /** Collection of sticky notes associated with the current graph/scenario */ stickyNotes?: StickyNote[];designer/client/src/components/graph/fragmentGraph.tsx (2)
7-24: LGTM! Consider adding prop documentation.The implementation correctly integrates sticky notes functionality while maintaining type safety. Consider adding JSDoc comments to document the props, especially the new
stickyNotesprop, to improve maintainability./** * Renders a preview of a fragment graph with sticky notes support. * @param props.processCounts - Process execution counts * @param props.scenario - Scenario configuration * @param props.stickyNotes - Collection of sticky notes for the graph * @param props.nodeIdPrefixForFragmentTests - Prefix for node IDs in fragment tests */ export const FragmentGraphPreview = forwardRef<...
9-9: Consider making stickyNotes explicitly optional.Since this is integrating a new feature, consider marking
stickyNotesas optional in the type definition to ensure backward compatibility and gradual adoption.- Pick<GraphProps, "processCounts" | "scenario" | "stickyNotes" | "nodeIdPrefixForFragmentTests"> + Pick<GraphProps, "processCounts" | "scenario" | "nodeIdPrefixForFragmentTests"> & { stickyNotes?: GraphProps["stickyNotes"] }designer/client/src/containers/theme/helpers.ts (2)
19-26: Add type safety and documentation improvementsThe function implementation is solid, but could benefit from some TypeScript best practices:
+ /** + * Returns an augmented color for sticky note background based on theme + * @param theme - Material-UI theme object + * @param color - CSS color string + * @returns Augmented color object from theme palette + */ - export function getStickyNoteBackgroundColor(theme: Theme, color: string) { + export function getStickyNoteBackgroundColor(theme: Theme, color: string): ReturnType<Theme['palette']['augmentColor']> {
20-20: Consider caching color validation resultsThe
CSS.supportscall could be expensive if called frequently. Consider caching results for commonly used colors if this function is called often in your application.designer/client/src/actions/notificationActions.tsx (2)
15-15: Remove or convert TODO into documentationThe TODO comment should be resolved before merging. If these changes are intentional and correct, consider converting this into proper documentation explaining the rationale behind the parameters and logic.
16-19: Improve error message handling logicThe current ternary operation could be made more explicit and safer. Consider restructuring the logic to handle edge cases more clearly.
- children: <Notification type={"error"} icon={<InfoOutlinedIcon />} message={showErrorText && error ? error : message} />, + children: <Notification + type={"error"} + icon={<InfoOutlinedIcon />} + message={showErrorText && error !== undefined ? error : message} + />,Also, consider adding JSDoc documentation to clarify the purpose of each parameter:
/** * Displays an error notification * @param message - The default message to display * @param error - Optional technical error details * @param showErrorText - When true and error is provided, shows technical error instead of message */ export function error(message: string, error?: string, showErrorText?: boolean): Actiondesigner/client/src/actions/actionTypes.ts (1)
15-16: Consider additional improvements for completeness.A few suggestions to enhance the implementation:
- Consider adding a
STICKY_NOTE_CREATEDaction type if sticky notes can be created separately from updates- Consider grouping related action types together (e.g., moving all sticky note actions adjacent to each other) for better maintainability
designer/client/src/components/graph/GraphPartialsInTS/cellUtils.ts (2)
12-14: Consider extracting the magic string to a constant.The function is well-implemented with proper type guarding. However, the string
"stickyNote.StickyNoteElement"could be extracted to a named constant for better maintainability.+const STICKY_NOTE_ELEMENT_TYPE = "stickyNote.StickyNoteElement"; + export function isStickyNoteElement(el: dia.Cell): el is shapes.devs.Model { - return isElement(el) && el.get("type") === `stickyNote.StickyNoteElement`; + return isElement(el) && el.get("type") === STICKY_NOTE_ELEMENT_TYPE; }
16-21: Consider using strict equality for noteId comparison.The function is well-implemented with proper type safety checks and immutability through deep cloning. However, consider using strict equality (
===) instead of loose equality (==) for the noteId comparison to prevent potential type coercion issues.- const stickyNote = stickyNotes.find((note) => note.noteId == noteId); + const stickyNote = stickyNotes.find((note) => note.noteId === noteId);designer/client/src/components/stickyNotes/StickyNotesPanel.tsx (5)
11-14: Consider adding prop type validationWhile TypeScript provides type safety through
ToolbarPanelProps, consider adding runtime prop validation usingprop-typesfor additional safety, especially for required props.
14-14: Consider extracting the note model configurationThe hardcoded note model could be moved to a constants file or configuration object for better maintainability and reusability.
+// In a separate constants file +export const DEFAULT_STICKY_NOTE = { + id: "StickyNoteToAdd", + type: StickyNoteType, + isDisabled: false, +}; -const noteModel = { id: "StickyNoteToAdd", type: StickyNoteType, isDisabled: false }; +const noteModel = DEFAULT_STICKY_NOTE;
21-26: Consider enhancing the user experience with a tooltipThe disabled state when
pristineis false should provide feedback to users about why the tool is unavailable.<Tool nodeModel={noteModel} label={t("stickyNotes.tool.label", "sticky note")} key={StickyNoteType + "_" + pristine} disabled={!pristine} + tooltip={!pristine ? t("stickyNotes.tool.disabled.tooltip", "Save your changes first to add sticky notes") : undefined} />
24-24: Improve key construction for better maintainabilityThe key construction using string concatenation could be made more maintainable.
-key={StickyNoteType + "_" + pristine} +key={`${StickyNoteType}_${pristine}`}
31-32: Consider direct return instead of fragmentSince there's only one child element being returned, the fragment wrapper is unnecessary.
-return <>{stickyNoteTool}</>; +return stickyNoteTool;designer/client/src/types/node.ts (1)
5-5: Consider adding JSDoc documentation for the Type union.The type modification correctly extends the union type to support sticky notes. However, since this is a core type that other components will rely on, consider adding JSDoc documentation to explain the purpose and valid values of this type.
+/** + * Represents the type of a node in the designer. + * - "FragmentInput": Represents a fragment input node + * - StickyNoteType: Represents a sticky note node + * - string: Allows for other custom node types + */ type Type = "FragmentInput" | typeof StickyNoteType | string;designer/client/src/components/graph/StickyNoteElement.ts (1)
6-15: Add JSDoc documentation for interfaces.While the interfaces are well-structured, adding JSDoc documentation would improve code maintainability and help other developers understand the purpose and usage of these interfaces.
+/** + * Default properties for configuring a sticky note element. + */ interface StickyNoteDefaults { + /** Position coordinates for the sticky note */ position?: { x: number; y: number }; + /** Dimensions of the sticky note */ size?: { width: number; height: number }; + /** Additional attributes for styling and behavior */ attrs?: Record<string, unknown>; } +/** + * Properties for the sticky note prototype including markup and additional configurations. + */ interface StickyNoteProtoProps { + /** Array of markup nodes defining the sticky note's structure */ markup: (dia.MarkupNodeJSON | MarkupNodeJSON)[]; + /** Additional prototype properties */ [key: string]: unknown; }designer/client/src/components/toolbars/creator/ComponentIcon.tsx (2)
26-27: Consider adding path validation and documentationWhile the implementation follows the existing pattern, consider:
- Adding runtime validation to ensure the SVG assets exist for all
StickyNoteTypevalues- Adding JSDoc comments to document the purpose and usage of these exported constants
+/** Path to the SVG asset for sticky note icons */ export const stickyNoteIconSrc = `/assets/components/${StickyNoteType}.svg`; +/** Preloaded sticky note icon reference, null if loading fails */ export const stickyNoteIcon: string | null = preloadBeImage(stickyNoteIconSrc);
26-27: Consider centralizing asset path managementSince the application handles various icon types (components, sticky notes), consider creating a centralized asset path management system. This could:
- Provide type-safe path construction
- Centralize path validation
- Make it easier to change asset organization in the future
Example approach:
// assets/paths.ts export const AssetPaths = { stickyNote: (type: StickyNoteType) => `/assets/components/${type}.svg`, component: (type: string) => `/assets/components/${type}.svg`, } as const;designer/client/src/components/graph/node-modal/node/FragmentContent.tsx (2)
17-17: Consider adding type annotation for better type safety.While the code works, adding a type annotation for
stickyNoteswould improve type safety and code documentation.- const stickyNotes = useSelector(getStickyNotes); + const stickyNotes: StickyNote[] = useSelector(getStickyNotes);
Line range hint
1-49: Well-structured integration of sticky notes feature.The implementation follows good practices by:
- Maintaining separation of concerns
- Following established Redux patterns
- Integrating cleanly with existing component structure
designer/server/src/test/scala/pl/touk/nussknacker/test/base/it/WithSimplifiedConfigScenarioHelper.scala (1)
44-50: Consider adding ScalaDoc for the new methods.While the methods are straightforward, adding ScalaDoc comments would help other developers understand:
- The expected behavior of these methods in test scenarios
- Any specific requirements for the input parameters
- The significance of the return types
Example documentation:
/** Updates the scenario with a new version and returns the update result. * @param scenarioName name of the scenario to update * @param newVersion new version of the scenario * @return update result containing version and modification details */ def updateScenario(scenarioName: ProcessName, newVersion: CanonicalProcess): ProcessUpdated /** Adds a sticky note to the specified scenario. * @param scenarioName name of the scenario to add the sticky note to * @param request details of the sticky note to add * @return correlation ID for tracking the sticky note */ def addStickyNote(scenarioName: ProcessName, request: StickyNoteAddRequest): StickyNoteCorrelationIddesigner/client/src/components/StickyNotePreview.tsx (2)
9-11: Consider extracting props interface.For better reusability and maintainability, consider extracting the props interface.
+interface StickyNotePreviewProps { + isActive?: boolean; + isOver?: boolean; +} + -export function StickyNotePreview({ isActive, isOver }: { isActive?: boolean; isOver?: boolean }): JSX.Element { +export function StickyNotePreview({ isActive, isOver }: StickyNotePreviewProps): JSX.Element {
19-53: Extract magic numbers in styles to constants.The styling is well-structured, but there are some magic numbers that should be constants for better maintainability.
+const SHADOW_OPACITY = 0.5; +const BORDER_WIDTH = 0.5; +const TRANSFORM_ORIGIN_X = "80%"; +const TRANSFORM_ORIGIN_Y = "50%"; +const STYLE_OPACITY = 0.5; + const nodeStyles = css({ // ... - filter: `drop-shadow(0 4px 8px ${alpha(theme.palette.common.black, 0.5)})`, - borderWidth: 0.5, + filter: `drop-shadow(0 4px 8px ${alpha(theme.palette.common.black, SHADOW_OPACITY)})`, + borderWidth: BORDER_WIDTH, borderStyle: "solid", - transformOrigin: "80% 50%", + transformOrigin: `${TRANSFORM_ORIGIN_X} ${TRANSFORM_ORIGIN_Y}`, // ... }); const colors = css({ - opacity: 0.5, + opacity: STYLE_OPACITY, // ... });designer/client/src/components/graph/GraphPartialsInTS/applyCellChanges.ts (2)
23-24: Consider using a more descriptive variable nameThe variable name 'a' in the map function could be more descriptive. Consider using 'modelWithTool' or 'stickyNoteWithTool' instead.
- const stickyNotesModels = stickyNotesModelsWithTools.map((a) => a.model); + const stickyNotesModels = stickyNotesModelsWithTools.map((modelWithTool) => modelWithTool.model);
46-57: Consider enhancing error handlingWhile the current error handling is functional, consider these improvements:
- Replace console logging with proper error tracking/monitoring
- Add more specific error messages including sticky note details
- Consider implementing an error recovery mechanism
newStickyNotesModelsWithTools.forEach((m) => { try { const view = m.model.findView(paper); if (!view) { - console.warn(`View not found for stickyNote model: ${m.model.id}`); + const details = `id: ${m.model.id}, position: ${JSON.stringify(m.model.position)}`; + logger.warn(`Failed to find view for sticky note: ${details}`); return; } view.addTools(m.tools); } catch (error) { - console.error(`Failed to add tools to stickyNote view:`, error); + logger.error(`Failed to add tools to sticky note view`, { + stickyNoteId: m.model.id, + error: error instanceof Error ? error.message : String(error) + }); + // Consider implementing recovery mechanism here } });designer/client/src/components/graph/types.ts (1)
49-49: LGTM: Consider documenting readonly behaviorThe addition of
stickyNotestoFragmentGraphPropsis consistent with the type structure. SinceFragmentGraphPropsis readonly, consider documenting whether sticky notes should be immutable in fragment contexts.designer/server/src/test/scala/pl/touk/nussknacker/ui/api/StickyNotesApiHttpServiceBusinessSpec.scala (2)
42-43: Document the helper method and consider parameterizing coordinates.Consider adding documentation to explain the purpose of the helper method and the significance of the default values used.
+ /** + * Creates a sticky note request with default values for testing. + * @param versionId The version ID of the scenario + * @return A sticky note request with predefined layout and styling + */ private def stickyNoteToAdd(versionId: VersionId): StickyNoteAddRequest = StickyNoteAddRequest(versionId, "", LayoutData(0, 1), "#aabbcc", Dimensions(300, 200), None)
69-82: Consider adding assertion messages for better test clarity.While the test logic is correct, adding descriptive messages to assertions would improve test failure debugging.
.when() .basicAuthAllPermUser() .get(s"$nuDesignerHttpAddress/api/processes/$exampleScenarioName/stickyNotes?scenarioVersionId=1") .Then() - .statusCode(200) + .statusCode(200, "Should return success status for valid version request") - .equalsJsonBody("[]") + .equalsJsonBody("[]", "Should return empty array for notes from different version")designer/client/src/components/ComponentPreview.tsx (2)
80-82: Add explicit type checking for better robustness.While optional chaining prevents crashes, consider adding explicit type checking for better error handling:
- return node?.type === StickyNoteType ? ( + if (!node) { + return null; // or an appropriate fallback UI + } + return node.type === StickyNoteType ? (
80-82: Consider extracting default rendering logic into a separate component.The component handles two distinct rendering paths. Consider extracting the default rendering logic into a separate component (e.g.,
DefaultComponentPreview) to improve maintainability and readability:function DefaultComponentPreview({ node, isActive, isOver, colors, nodeStyles, ...props }) { return ( <div className={cx(colors, nodeStyles)}> {/* existing default rendering logic */} </div> ); }Then simplify the main component:
return node?.type === StickyNoteType ? <StickyNotePreview isActive={isActive} isOver={isOver} /> : <DefaultComponentPreview node={node} isActive={isActive} isOver={isOver} colors={colors} nodeStyles={nodeStyles} />;designer/client/src/components/graph/ProcessGraph.tsx (1)
49-54: Consider extracting drop handlers for better maintainability.While the logic is correct, consider extracting the handlers into separate functions to improve readability and maintainability.
- if (item?.type === StickyNoteType) { - graph.current.addStickyNote(scenario.name, scenario.processVersionId, mapValues(nodeInputRelOffset, Math.round)); - } else { - graph.current.addNode(monitor.getItem(), mapValues(nodeInputRelOffset, Math.round)); - setLinksHovered(graph.current.graph); - } + const position = mapValues(nodeInputRelOffset, Math.round); + handleItemDrop(item, position); + + function handleItemDrop(item: NodeType, position: any) { + if (item?.type === StickyNoteType) { + graph.current.addStickyNote(scenario.name, scenario.processVersionId, position); + return; + } + graph.current.addNode(item, position); + setLinksHovered(graph.current.graph); + }designer/client/src/components/graph/utils/graphUtils.ts (1)
130-134: LGTM! Consider adding JSDoc documentation.The addition of the
preventDefaultparameter with a default value is a good approach to make the event handling more flexible. This change maintains backward compatibility while allowing fine-grained control over event prevention.Consider adding JSDoc documentation to explain the purpose of the
preventDefaultparameter:/** * Creates an event handler for graph interactions * @param touchEvent Handler for touch events * @param mouseEvent Handler for mouse events * @param preventDefault Function to determine if default event behavior should be prevented * @returns Combined event handler function */designer/server/src/main/scala/pl/touk/nussknacker/ui/config/FeatureTogglesConfig.scala (1)
61-62: Consider adding configuration validationThe configuration parsing looks good, using the idiomatic Scala approach with
getAsandgetOrElse. However, consider adding validation for the sticky notes settings to ensure all required fields are properly configured.Example validation approach:
val stickyNotesSettings = config.getAs[StickyNotesSettings]("stickyNotesSettings") match { case Some(settings) => validateStickyNotesSettings(settings) case None => StickyNotesSettings.default }designer/client/src/actions/nk/process.ts (1)
63-105: Consider adding loading statesThe sticky note operations would benefit from loading states to improve user experience during API calls.
Consider dispatching loading actions before API calls and clearing them after completion:
// Example for fetchStickyNotesForScenario dispatch({ type: "STICKY_NOTES_LOADING", loading: true }); // API call // ... then in finally block: dispatch({ type: "STICKY_NOTES_LOADING", loading: false });designer/client/src/components/graph/NodeDescriptionPopover.tsx (2)
123-123: Enhance code clarity with better documentationWhile the logic is correct, the comment could be more descriptive and professional.
Consider this improvement:
- if (isStickyNoteElement(view.model)) return; //Dont use it for stickyNotes + // Skip description popover for sticky notes as they handle their own content display + if (isStickyNoteElement(view.model)) { + return; + }
Line range hint
123-128: Consider extracting sticky note behavior into a separate componentThe current approach of excluding sticky notes from the description popover is correct. However, as the sticky notes feature grows, consider creating a dedicated
StickyNotePopovercomponent to handle sticky note-specific interactions. This would:
- Separate concerns more clearly
- Make the code more maintainable
- Allow for easier extension of sticky note-specific features
designer/client/src/components/graph/graphStyledWrapper.ts (1)
186-211: Consider UX improvements for the sticky note editor.The current implementation could benefit from the following enhancements:
- Add hover state styles for better interactivity
- Include smooth transitions for state changes
- Consider adding a subtle box-shadow for depth
Here's a suggested implementation:
.sticky-note-markdown-editor: { + transition: "all 0.2s ease-in-out", + boxShadow: `0 1px 3px ${alpha(theme.palette.common.black, 0.1)}`, + "&:hover": { + backgroundColor: alpha(theme.palette.common.white, 0.4), + boxShadow: `0 2px 4px ${alpha(theme.palette.common.black, 0.15)}`, + }, // ... existing styles }designer/client/src/reducers/graph/utils.ts (2)
85-96: Consider defensive programming and performance optimizationsThe function could benefit from the following improvements:
- Add null checks for state.stickyNotes to handle undefined cases
- Optimize to avoid multiple iterations over the arrays
export function removeStickyNoteFromLayout(state: GraphState, stickyNoteId: number): { layout: NodePosition[]; stickyNotes: StickyNote[] } { - const { layout } = state; + const { layout, stickyNotes = [] } = state; const stickyNoteLayoutId = createStickyNoteId(stickyNoteId); - const updatedStickyNotes = state.stickyNotes.filter((n) => n.noteId !== stickyNoteId); - const updatedLayout = updatedStickyNotes.map((stickyNote) => { - return { id: createStickyNoteId(stickyNote.noteId), position: stickyNote.layoutData }; - }); + const updatedStickyNotes = stickyNotes.filter((n) => n.noteId !== stickyNoteId); + const filteredLayout = layout.filter((l) => l.id !== stickyNoteLayoutId); + const newLayoutEntries = updatedStickyNotes.map((stickyNote) => ({ + id: createStickyNoteId(stickyNote.noteId), + position: stickyNote.layoutData, + })); return { - stickyNotes: [...updatedStickyNotes], - layout: [...layout.filter((l) => l.id !== stickyNoteLayoutId), ...updatedLayout], + stickyNotes: updatedStickyNotes, + layout: [...filteredLayout, ...newLayoutEntries], }; }
Line range hint
85-135: Consider implementing a StickyNoteManager classThe current implementation spreads sticky note management across multiple functions. Consider encapsulating this functionality in a dedicated
StickyNoteManagerclass to:
- Centralize validation logic
- Manage state consistency
- Handle ID generation and uniqueness
- Provide a cleaner API for sticky note operations
This would improve maintainability and make the code more testable.
designer/server/src/test/scala/pl/touk/nussknacker/test/utils/domain/ScenarioHelper.scala (2)
85-97: Consider adding documentation for test utility methods.The methods are well-structured, but since they're part of a test utility class, consider adding ScalaDoc comments to explain their testing purpose and usage examples.
Example documentation:
/** Updates a test scenario and waits for completion. * @param scenarioName name of the scenario to update * @param newScenario new scenario content * @return ProcessUpdated containing update results */ def updateScenario(... /** Adds a sticky note to a test scenario and waits for completion. * @param scenarioName name of the scenario to add the note to * @param request sticky note details * @return correlation ID of the created note */ def addStickyNote(...
222-258: Consider improving error handling for missing scenarios.Both helper methods use
.map(_.get)on the optional scenario ID, which could throw if the scenario doesn't exist. Consider using more explicit error handling.Example improvement:
- scenarioId <- futureFetchingScenarioRepository.fetchProcessId(scenarioName).map(_.get) + scenarioId <- futureFetchingScenarioRepository.fetchProcessId(scenarioName).flatMap { + case Some(id) => Future.successful(id) + case None => Future.failed(new NoSuchElementException(s"Scenario not found: $scenarioName")) + }designer/server/src/main/scala/pl/touk/nussknacker/ui/server/AkkaHttpBasedRouteProvider.scala (1)
72-72: Well-designed integration of sticky notes functionality.The implementation follows a clean architecture pattern:
- Repository layer for data access
- Service layer for business logic
- API layer for HTTP endpoints
- Proper integration into the existing route structure
This modular approach ensures good separation of concerns and maintainability.
Also applies to: 171-171, 402-409, 603-603
designer/client/src/http/HttpService.ts (3)
695-707: Consider making the default sticky note color configurable.The default color is hardcoded. Consider moving it to a configuration object for better maintainability and customization.
- color: STICKY_NOTE_DEFAULT_COLOR, //TODO add config for default sticky note color? For now this is default. + color: config.stickyNotes.defaultColor ?? STICKY_NOTE_DEFAULT_COLOR,
713-713: Use i18next interpolation instead of template literal.For consistency with other error messages, use i18next interpolation for the stickyNoteId.
- i18next.t("notification.error.failedToDeleteStickyNote", `Failed to delete sticky note with id: ${stickyNoteId}`), + i18next.t("notification.error.failedToDeleteStickyNote", "Failed to delete sticky note with id: {{stickyNoteId}}", { stickyNoteId }),
695-746: Add input validation and JSDoc comments.Consider these improvements:
- Add input validation for required fields
- Add JSDoc comments to document parameters, return types, and possible errors
Example JSDoc and validation:
/** * Adds a sticky note to a scenario * @param scenarioName - The name of the scenario * @param scenarioVersionId - The version ID of the scenario * @param position - The position of the sticky note * @param dimensions - The dimensions of the sticky note * @returns Promise<AxiosResponse<StickyNote>> * @throws {Error} When required parameters are missing */ addStickyNote(scenarioName: string, scenarioVersionId: number, position: Position, dimensions: Dimensions) { if (!scenarioName || !scenarioVersionId || !position || !dimensions) { throw new Error("Missing required parameters"); } // ... rest of the method }docs/configuration/DesignerConfiguration.md (1)
593-593: Documentation enhancement needed for sticky notes panel.The addition of the sticky-notes-panel configuration is well-placed and follows the existing pattern. However, consider enhancing the documentation by adding:
- A description of the sticky notes feature and its purpose
- Available configuration options specific to sticky notes
- Any limitations or constraints users should be aware of
docs-internal/api/nu-designer-openapi.yaml (2)
5501-5513: Well-designed schema definitions for sticky notes functionality.The schema definitions are well-structured with:
- Clear separation between core entity (StickyNote) and request DTOs
- Proper required field annotations
- Consistent use of types
- Comprehensive property definitions
A few suggestions to enhance the implementation:
- Consider adding validation constraints for:
- Color format (hex color code)
- Dimensions (minimum/maximum values)
- Content length limits
Dimensions: title: Dimensions type: object required: - width - height properties: width: type: integer format: int64 + minimum: 50 + maximum: 1000 height: type: integer format: int64 + minimum: 50 + maximum: 1000 StickyNote: title: StickyNote type: object required: - noteId - content - layoutData - color - dimensions - editedBy - editedAt properties: color: type: string + pattern: '^#[0-9A-Fa-f]{6}$' content: type: string + maxLength: 5000Also applies to: 7191-7276
Line range hint
2530-7276: Consider adding rate limiting and pagination for sticky notes endpoints.For better API scalability and performance:
- Add rate limiting headers to prevent abuse
- Implement pagination for GET endpoint to handle large numbers of sticky notes
- Consider adding bulk operations endpoints for efficiency
designer/server/src/main/scala/pl/touk/nussknacker/ui/process/repository/stickynotes/StickyNotesRepository.scala (2)
21-49: Add Scaladoc comments to public methods in the traitConsider adding Scaladoc comments to each of the public methods in the
StickyNotesRepositorytrait to improve code documentation and help other developers understand their purpose and usage.
21-49: Ensure consistent parameter ordering across methodsTo improve readability and maintainability, consider ordering the parameters consistently across all methods. For example, placing
scenarioIdandscenarioVersionIdparameters before other parameters in each method.designer/server/src/main/scala/db/migration/V1_060__CreateStickyNotesDefinition.scala (4)
66-69: Consider removing the dependency on Shapeless for tuple mappingThe use of
shapeless.syntax.std.tuple._andproductElementsintroduces an additional dependency on Shapeless, which may be unnecessary. You can achieve the same result using standard Scala features, simplifying the code and reducing dependencies.You can modify the mapping as follows:
-override def * = - (id :: tupleWithoutAutoIncId.productElements).tupled <> ( - StickyNoteEventEntityData.apply _ tupled, StickyNoteEventEntityData.unapply - ) +override def * = + ( + id, + noteCorrelationId, + content, + layoutData, + color, + dimensions, + targetEdge, + eventCreator, + eventDate, + eventType, + scenarioId, + scenarioVersionId + ) <> ( + StickyNoteEventEntityData.tupled, + StickyNoteEventEntityData.unapply + )This change eliminates the need for
tupleWithoutAutoIncIdand the Shapeless import.
25-25: Define foreign key constraints using Slick's table definitionsInstead of executing raw SQL to add foreign key constraints, you can define the foreign key relationship directly in the Slick table definition. This approach enhances type safety and maintainability.
Modify the
StickyNotesEntityclass to include the foreign key constraint:def scenario = foreignKey( "sticky_notes_scenario_version_fk", (scenarioId, scenarioVersionId), processVersionsTable )( r => (r.processId, r.id), onDelete = ForeignKeyAction.Cascade )Ensure you have
processVersionsTabledefined as aTableQueryof the corresponding table, and import or define any necessary references.
39-50: Consider adding indexes to improve query performanceAdding indexes to columns that are frequently queried can significantly enhance database performance. Columns like
noteCorrelationId,scenarioId, andscenarioVersionIdare good candidates for indexing.Include index definitions in the
StickyNotesEntityclass:// After column definitions def idxNoteCorrelationId = index("idx_sticky_notes_note_correlation_id", noteCorrelationId) def idxScenarioId = index("idx_sticky_notes_scenario_id", scenarioId) def idxScenarioVersionId = index("idx_sticky_notes_scenario_version_id", scenarioVersionId)These indexes will help optimize queries that filter or join on these columns.
41-44: Use appropriate data types for structured dataFields such as
layoutData,color, anddimensionsappear to hold structured or complex data. Storing them as plain strings may limit querying capabilities and data validation.If these fields contain JSON data or other structured formats, consider using appropriate database column types:
- For JSON data, use a
JSONorJSONBcolumn type if your database supports it.- Define custom column types or Slick mappings for complex structures.
For example, if
dimensionsis a JSON object representing width and height:// Define a case class for dimensions case class Dimensions(width: Int, height: Int) // Use an implicit mapping for the custom type implicit val dimensionsColumnType = MappedColumnType.base[Dimensions, String]( dimensions => write(dimensions), // Convert Dimensions to JSON string jsonStr => read[Dimensions](jsonStr) // Convert JSON string to Dimensions ) // Then define the column def dimensions = column[Dimensions]("dimensions", NotNull)This approach ensures data integrity and eases future data manipulation.
designer/server/src/main/scala/pl/touk/nussknacker/ui/api/description/stickynotes/Dtos.scala (1)
38-41: Consider usingIntinstead ofLongfor dimension fieldsIn the
Dimensionscase class, thewidthandheightfields are defined asLong. Unless you anticipate extremely large values for dimensions, usingIntmight be more appropriate and efficient.designer/client/src/components/graph/EspNode/stickyNote.ts (2)
67-71: Enhance Error Handling for Markdown ParsingIn the catch block, consider providing more descriptive feedback to the user when markdown parsing fails. Instead of a generic message, you could display a user-friendly error indicating what went wrong and how to fix it. This improves the user experience by guiding users to correct their input.
83-83: Consider Externalizing the SVG Path for ReusabilityThe
stickyNotePathconstant defines an SVG path used for the sticky note shape. If this path might be reused in other components or shapes, consider moving it to a shared constants file to promote reusability and maintainability.designer/client/src/components/graph/EspNode/stickyNoteElements.ts (1)
25-46: Optimize by reusing the sticky note background colorThe function
getStickyNoteBackgroundColor(theme, stickyNote.color)is called multiple times within theattrsobject. To improve performance and enhance readability, consider computing the background color once and reusing it in the attributes.Apply this refactor:
+ const stickyNoteBackgroundColor = getStickyNoteBackgroundColor(theme, stickyNote.color); attrs: { size: { width: stickyNote.dimensions.width, height: stickyNote.dimensions.height, }, body: { - fill: getStickyNoteBackgroundColor(theme, stickyNote.color).main, + fill: stickyNoteBackgroundColor.main, opacity: 1, }, foreignObject: { width: stickyNote.dimensions.width, height: stickyNote.dimensions.height - ICON_SIZE - CONTENT_PADDING * 4, - color: theme.palette.getContrastText(getStickyNoteBackgroundColor(theme, stickyNote.color).main), + color: theme.palette.getContrastText(stickyNoteBackgroundColor.main), }, icon: { xlinkHref: stickyNoteIcon, opacity: 1, - color: theme.palette.getContrastText(getStickyNoteBackgroundColor(theme, stickyNote.color).main), + color: theme.palette.getContrastText(stickyNoteBackgroundColor.main), }, border: { - stroke: getStickyNoteBackgroundColor(theme, stickyNote.color).dark, + stroke: stickyNoteBackgroundColor.dark, strokeWidth: 1, }, },designer/server/src/main/scala/pl/touk/nussknacker/ui/process/repository/stickynotes/DbStickyNotesRepository.scala (2)
88-94: Improve exception messages for clarityThe exception messages in lines 88 and 92 use phrases like "This is odd," which may not provide sufficient detail for debugging. Consider making the messages more descriptive.
Consider applying this diff:
case 0 => DBIOAction.failed(new IllegalStateException( - s"This is odd, no sticky note was added" + s"Failed to add sticky note: no records were inserted" )) case n => DBIOAction.failed(new IllegalStateException( - s"This is odd, more than one sticky note were added (added $n records)." + s"Failed to add sticky note: unexpected number of records inserted ($n)" ))
34-48: Add comments to explain complex query logic infindStickyNotesThe query in the
findStickyNotesmethod is complex and could benefit from inline comments explaining the logic at each step, improving maintainability and readability.designer/server/src/main/scala/pl/touk/nussknacker/ui/api/description/StickyNotesApiEndpoints.scala (3)
60-60: Typo in endpoint summary: 'nodes' should be 'notes'The summary currently reads "Returns sticky nodes for given scenario with version" but it should be "Returns sticky notes for given scenario with version".
146-146: Consistent terminology in endpoint summaryIn the summary, "Deletes stickyNote by given noteId" should be "Deletes sticky note by given noteId" for consistency and readability.
171-171: UsejsonBodyinstead ofplainBodyfor error responsesIn the error responses at lines 171, 183, 195, and 207,
plainBodyis used to define the error outputs. For better consistency and to provide structured error information, consider usingjsonBodyinstead ofplainBody. This will return the error responses in JSON format, which is more suitable for API clients expecting JSON responses.Also applies to: 183-183, 195-195, 207-207
designer/server/src/main/scala/pl/touk/nussknacker/ui/api/StickyNotesApiHttpService.scala (1)
46-104: Refactor duplicate logic in endpoint handlersThe logic for fetching
scenarioIdand checking authorization is repeated across the endpoint handlers for getting, adding, updating, and deleting sticky notes. Consider abstracting this common code into a helper method or middleware to reduce duplication and enhance maintainability.designer/client/src/components/graph/Graph.tsx (2)
470-489: Consider externalizing sticky note size constraints to configurationThe maximum and minimum dimensions for sticky notes are hardcoded within the
CELL_RESIZEDevent handler. Moving these constraints to a configuration file or constants module will enhance maintainability and make future adjustments easier.Would you like assistance in refactoring this code to externalize the size constraints?
495-495: Use strict equality for string comparisonTo prevent potential type coercion issues, it's advisable to use strict equality
===when comparing strings in TypeScript.Apply this diff to update the comparison:
-if (updatedStickyNote.content == content) return; +if (updatedStickyNote.content === content) return;designer/client/src/reducers/graph/reducer.ts (2)
249-254: Simplify redundant object spread in return statementThe return statement spreads the result of
addStickyNotesWithLayoutinto a new object, which is unnecessary sinceaddStickyNotesWithLayoutalready returns a new state object. You can simplify the return statement by returning the result directly.Apply this diff to simplify the return statement:
- return { - ...addStickyNotesWithLayout(state, { stickyNotes, layout }), - }; + return addStickyNotesWithLayout(state, { stickyNotes, layout });
255-260: Simplify redundant object spread in return statementSimilar to the previous case, the return statement here unnecessarily spreads the result of
addStickyNotesWithLayoutinto a new object. Simplify the return statement by returning the result directly.Apply this diff:
- return { - ...addStickyNotesWithLayout(state, { stickyNotes, layout }), - }; + return addStickyNotesWithLayout(state, { stickyNotes, layout });
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
⛔ Files ignored due to path filters (2)
designer/client/package-lock.jsonis excluded by!**/package-lock.jsondesigner/server/src/main/resources/web/static/assets/components/StickyNote.svgis excluded by!**/*.svg
📒 Files selected for processing (56)
designer/client/package.json(1 hunks)designer/client/src/actions/actionTypes.ts(1 hunks)designer/client/src/actions/nk/process.ts(3 hunks)designer/client/src/actions/notificationActions.tsx(1 hunks)designer/client/src/assets/json/nodeAttributes.json(1 hunks)designer/client/src/common/StickyNote.ts(1 hunks)designer/client/src/components/ComponentPreview.tsx(2 hunks)designer/client/src/components/StickyNotePreview.tsx(1 hunks)designer/client/src/components/graph/EspNode/stickyNote.ts(1 hunks)designer/client/src/components/graph/EspNode/stickyNoteElements.ts(1 hunks)designer/client/src/components/graph/Graph.tsx(14 hunks)designer/client/src/components/graph/GraphPartialsInTS/applyCellChanges.ts(2 hunks)designer/client/src/components/graph/GraphPartialsInTS/cellUtils.ts(2 hunks)designer/client/src/components/graph/NodeDescriptionPopover.tsx(2 hunks)designer/client/src/components/graph/ProcessGraph.tsx(4 hunks)designer/client/src/components/graph/StickyNoteElement.ts(1 hunks)designer/client/src/components/graph/fragmentGraph.tsx(1 hunks)designer/client/src/components/graph/graphStyledWrapper.ts(2 hunks)designer/client/src/components/graph/node-modal/node/FragmentContent.tsx(3 hunks)designer/client/src/components/graph/types.ts(3 hunks)designer/client/src/components/graph/utils/graphUtils.ts(1 hunks)designer/client/src/components/stickyNotes/StickyNotesPanel.tsx(1 hunks)designer/client/src/components/toolbarSettings/TOOLBAR_COMPONENTS_MAP.ts(2 hunks)designer/client/src/components/toolbarSettings/defaultToolbarsConfig.ts(1 hunks)designer/client/src/components/toolbars/creator/ComponentIcon.tsx(2 hunks)designer/client/src/components/toolbars/creator/ToolBox.tsx(1 hunks)designer/client/src/containers/theme/helpers.ts(2 hunks)designer/client/src/http/HttpService.ts(3 hunks)designer/client/src/reducers/graph/reducer.ts(2 hunks)designer/client/src/reducers/graph/types.ts(2 hunks)designer/client/src/reducers/graph/utils.ts(3 hunks)designer/client/src/reducers/selectors/graph.ts(2 hunks)designer/client/src/types/node.ts(1 hunks)designer/client/src/types/stickyNote.ts(1 hunks)designer/server/src/main/resources/defaultDesignerConfig.conf(2 hunks)designer/server/src/main/scala/db/migration/V1_060__CreateStickyNotesDefinition.scala(1 hunks)designer/server/src/main/scala/db/migration/hsql/V1_060__CreateStickyNotes.scala(1 hunks)designer/server/src/main/scala/db/migration/postgres/V1_060__CreateStickyNotes.scala(1 hunks)designer/server/src/main/scala/pl/touk/nussknacker/ui/api/StickyNotesApiHttpService.scala(1 hunks)designer/server/src/main/scala/pl/touk/nussknacker/ui/api/description/StickyNotesApiEndpoints.scala(1 hunks)designer/server/src/main/scala/pl/touk/nussknacker/ui/api/description/stickynotes/Dtos.scala(1 hunks)designer/server/src/main/scala/pl/touk/nussknacker/ui/api/description/stickynotes/StickyNoteEvent.scala(1 hunks)designer/server/src/main/scala/pl/touk/nussknacker/ui/config/FeatureTogglesConfig.scala(4 hunks)designer/server/src/main/scala/pl/touk/nussknacker/ui/config/scenariotoolbar/ToolbarPanelConfig.scala(1 hunks)designer/server/src/main/scala/pl/touk/nussknacker/ui/db/NuTables.scala(1 hunks)designer/server/src/main/scala/pl/touk/nussknacker/ui/db/entity/StickyNotesEntityFactory.scala(1 hunks)designer/server/src/main/scala/pl/touk/nussknacker/ui/process/repository/stickynotes/DbStickyNotesRepository.scala(1 hunks)designer/server/src/main/scala/pl/touk/nussknacker/ui/process/repository/stickynotes/StickyNotesRepository.scala(1 hunks)designer/server/src/main/scala/pl/touk/nussknacker/ui/server/AkkaHttpBasedRouteProvider.scala(4 hunks)designer/server/src/test/resources/config/common-designer.conf(2 hunks)designer/server/src/test/scala/pl/touk/nussknacker/test/base/it/WithSimplifiedConfigScenarioHelper.scala(2 hunks)designer/server/src/test/scala/pl/touk/nussknacker/test/utils/domain/ScenarioHelper.scala(4 hunks)designer/server/src/test/scala/pl/touk/nussknacker/ui/api/ProcessesResourcesSpec.scala(2 hunks)designer/server/src/test/scala/pl/touk/nussknacker/ui/api/StickyNotesApiHttpServiceBusinessSpec.scala(1 hunks)docs-internal/api/nu-designer-openapi.yaml(3 hunks)docs/configuration/DesignerConfiguration.md(1 hunks)
🧰 Additional context used
📓 Learnings (1)
designer/client/src/components/graph/Graph.tsx (1)
Learnt from: philemone
PR: TouK/nussknacker#7149
File: designer/client/src/components/graph/Graph.tsx:530-536
Timestamp: 2024-11-20T10:37:32.855Z
Learning: There is backend validation for sticky note content length, so frontend validation in the `updateStickyNote` method in `Graph.tsx` is unnecessary.
🔇 Additional comments (82)
designer/server/src/main/scala/db/migration/hsql/V1_060__CreateStickyNotes.scala (2)
1-5: LGTM! Clean organization of imports and package structure.
The imports are minimal and properly scoped, including only the necessary components for the HSQL-specific migration.
6-8: LGTM! Clean implementation with proper lazy loading.
The implementation follows best practices:
- Extends the base definition trait for sticky notes
- Uses lazy loading for the JDBC profile
- Properly overrides the profile for HSQLDB specifically
Please ensure corresponding migration files exist for other supported database types (e.g., PostgreSQL) to maintain database compatibility across different environments.
designer/server/src/main/scala/db/migration/postgres/V1_060__CreateStickyNotes.scala (2)
1-5: LGTM! Clean and well-organized imports
The package declaration and imports are appropriate for a PostgreSQL-specific migration implementation.
6-8: LGTM! Well-structured migration class
The implementation follows best practices:
- Proper extension of the base definition
- Correct use of lazy initialization for the profile
- Clean separation of PostgreSQL-specific configuration
Please ensure that:
- The base definition in
V1_060__CreateStickyNotesDefinitionincludes appropriate indexes for foreign key columns - The migration has been tested with actual PostgreSQL database to verify the schema changes
designer/client/src/common/StickyNote.ts (1)
6-8: Consider consolidating ID fields.
Having both id (optional string) and noteId (required number) could lead to confusion. Consider using a single ID field or documenting the specific use case for each.
designer/server/src/main/scala/pl/touk/nussknacker/ui/db/NuTables.scala (2)
14-15: LGTM! Clean integration of sticky notes functionality.
The addition of StickyNotesEntityFactory follows the established pattern of trait composition and maintains separation of concerns in the database layer.
14-15: Verify database migration ordering.
Please ensure that the database migrations (V1_060__CreateStickyNotes) are applied before this change is deployed to prevent runtime issues.
designer/server/src/main/scala/pl/touk/nussknacker/ui/api/description/stickynotes/StickyNoteEvent.scala (1)
9-12: Verify the completeness of event types.
The current set of events covers Create, Update, and Delete operations. Please verify if any additional events are needed for the complete sticky notes feature, such as:
- Events for bulk operations
- Events for state changes (e.g., visibility, pinning)
- Events for user interactions
designer/client/src/reducers/graph/types.ts (1)
4-4: Verify the StickyNote type definition
The import looks good, but please ensure that the StickyNote type in ../../common/StickyNote is properly defined with all necessary properties for the sticky notes feature.
designer/client/src/assets/json/nodeAttributes.json (1)
41-43: LGTM! The StickyNote entry follows consistent patterns.
The new entry maintains the established structure and naming conventions of the configuration file.
Please ensure that all related changes (UI components, actions, and state management) properly reference this node type configuration.
designer/client/src/containers/theme/helpers.ts (1)
4-4: LGTM!
The import statement is correctly formatted and necessary for the new functionality.
designer/client/src/actions/actionTypes.ts (1)
15-16: LGTM! The new action types are well-defined.
The added action types follow the existing naming conventions and represent clear, atomic operations for sticky note management.
designer/client/src/components/toolbarSettings/TOOLBAR_COMPONENTS_MAP.ts (2)
11-11: LGTM! Clean import statement.
The import follows the established pattern and maintains consistency with the project structure.
21-21: LGTM! Please verify toolbar configuration.
The sticky notes panel integration looks good. Since this is a draft PR, please ensure that:
- The corresponding toolbar configuration includes this new panel identifier
- The panel's position in the toolbar layout has been tested
designer/client/src/components/graph/GraphPartialsInTS/cellUtils.ts (1)
2-3: LGTM! Imports are appropriate for the new functionality.
The addition of StickyNote type and cloneDeep from lodash are well-suited for handling sticky note operations.
designer/client/src/components/stickyNotes/StickyNotesPanel.tsx (2)
1-10: LGTM: Imports are well-organized and necessary
All imports are being used and serve clear purposes:
- React core functionality
- State management (Redux)
- Internationalization
- Type definitions and UI components
16-30: Verify the necessity of useMemo for this component
The useMemo usage here might be premature optimization:
- The component's render tree seems relatively simple
- The dependencies (
pristine,props,t) might change frequently enough to negate the benefits
Consider benchmarking with and without useMemo to ensure it provides meaningful performance benefits.
designer/client/src/types/node.ts (1)
3-3: LGTM! Clean import statement.
The import follows TypeScript best practices and maintains good module organization.
designer/server/src/test/resources/config/common-designer.conf (2)
37-37: LGTM! Panel placement is appropriate.
The sticky-notes-panel is correctly placed in the topLeft toolbar section alongside other tool panels.
58-61: Verify sticky notes limits and consider adding validation messages.
The limits seem reasonable, but please verify:
- Is 5000 characters sufficient for your use case? Consider documenting the rationale.
- Is the limit of 5 notes per process adequate?
Consider adding validation messages to be displayed when these limits are exceeded, similar to how scenarioLabelSettings includes validationMessage.
designer/client/src/components/graph/StickyNoteElement.ts (2)
1-5: LGTM! Imports and type definitions are well-organized.
The imports are appropriate for the sticky note functionality, and the type alias helps maintain type compatibility.
17-18: LGTM! Element definition follows JointJS patterns.
The implementation is concise and properly typed, following JointJS best practices for custom element definition.
designer/client/src/components/toolbars/creator/ComponentIcon.tsx (1)
10-10: LGTM! Import statement is properly placed
The import for StickyNoteType is correctly organized with other type imports and follows the project's import structure.
designer/client/src/components/graph/node-modal/node/FragmentContent.tsx (2)
4-4: LGTM! Import changes are well-organized.
The getStickyNotes selector import follows the established pattern and is correctly placed with other selector imports.
44-44: Verify FragmentGraphPreview prop types and documentation.
Ensure that:
- The
stickyNotesprop is properly typed in theFragmentGraphPreviewcomponent. - The component's documentation is updated to reflect this new prop.
designer/server/src/test/scala/pl/touk/nussknacker/test/base/it/WithSimplifiedConfigScenarioHelper.scala (3)
9-10: LGTM! Clean and well-scoped imports.
The new imports are properly organized and explicitly specify the required types.
44-46: LGTM! Consider adding test coverage verification.
The method follows the established pattern of delegating to rawScenarioHelper. Since this is a test helper method, ensure there are test cases that verify both successful and failed scenario updates using this method.
48-50: LGTM! Consider comprehensive test scenarios.
The method is well-structured and consistent with the trait's design. Since this is introducing sticky note functionality, ensure test coverage includes:
- Adding sticky notes to different types of scenarios (saved, deployed, archived)
- Handling of invalid requests
- Correlation ID usage in subsequent operations
designer/client/src/components/StickyNotePreview.tsx (2)
1-8: LGTM! Imports are well-organized and necessary.
All imports are being used appropriately in the component.
55-62: LGTM! Clean and efficient render structure.
The component's render method is well-structured and makes good use of className composition.
designer/client/src/components/graph/GraphPartialsInTS/applyCellChanges.ts (3)
10-11: LGTM: Clean import additions
The new imports are properly typed and necessary for the sticky notes functionality.
13-19: LGTM: Well-typed function signature update
The addition of the stickyNotes parameter is properly typed and maintains the function's type safety.
Line range hint 30-44: LGTM: Robust cell management implementation
The cell management logic properly handles the addition, removal, and updates of cells while maintaining clean separation between different cell types.
designer/client/src/components/graph/types.ts (3)
10-12: LGTM: Imports are well-organized
The new imports for sticky note actions and types are properly structured and align with the feature requirements.
Also applies to: 17-17
25-27: LGTM: Props structure is consistent
The new sticky note properties follow the established pattern for action handlers and state management.
Please verify that the StickyNote type definition in ../../common/StickyNote includes all necessary properties for the graph operations.
Also applies to: 31-31
77-79: Consider adding creation event for completeness
The new cell events cover modification operations well. Consider adding a CELL_CREATED = "cellCustom:created" event to maintain symmetry with the full lifecycle of sticky notes.
Also, verify that these events align with all the sticky note operations in the UI implementation.
designer/server/src/main/scala/pl/touk/nussknacker/ui/config/scenariotoolbar/ToolbarPanelConfig.scala (2)
34-34: LGTM! Clean addition of the sticky notes panel type.
The new StickyNotesPanel value follows the established pattern and is correctly integrated into the enumeration. The implementation maintains type safety and will work with existing JSON serialization.
34-34: Verify frontend compatibility.
Please ensure that the panel type string "sticky-notes-panel" matches the identifier used in the frontend components, particularly in the defaultToolbarsConfig.ts and TOOLBAR_COMPONENTS_MAP mentioned in the summary.
designer/client/src/components/toolbarSettings/defaultToolbarsConfig.ts (2)
74-80: LGTM! The sticky notes panel integration looks good.
The addition of sticky-notes-panel follows the existing pattern and maintains consistency with other panel definitions in the TopLeft toolbar section.
77-77: Verify sticky-notes-panel registration
Please ensure that the sticky-notes-panel ID is properly registered in the panel system and that the corresponding component is correctly imported and available.
designer/server/src/test/scala/pl/touk/nussknacker/ui/api/StickyNotesApiHttpServiceBusinessSpec.scala (3)
1-21: LGTM! Well-organized imports.
The imports are clearly organized and properly scoped for the test requirements.
24-33: LGTM! Well-structured test specification class.
The class appropriately combines testing traits for HTTP testing, configuration, and assertions.
84-84: Expand test coverage for comprehensive validation.
The TODO indicates more tests are needed. Consider adding test cases for:
- Successful sticky note creation and retrieval
- Invalid version ID handling
- Permission checks
- Content validation
- Multiple notes handling
Would you like me to help generate additional test cases?
designer/client/src/components/ComponentPreview.tsx (1)
10-11: LGTM! Import statements are properly organized.
The new imports for sticky note functionality are correctly placed and follow project conventions.
designer/client/src/components/toolbars/creator/ToolBox.tsx (1)
15-15: LGTM! Good practice making the styled component reusable.
Making StyledToolbox exportable promotes code reuse and maintains styling consistency across components. This change aligns well with the sticky notes feature implementation.
designer/client/src/components/graph/ProcessGraph.tsx (4)
6-6: LGTM! Import changes are well-organized.
The new imports for sticky notes functionality are logically grouped and include all necessary dependencies.
Also applies to: 13-24, 28-28
36-36: LGTM! State management is properly implemented.
The sticky notes state is correctly retrieved using the selector and integrated into the component.
87-89: LGTM! Action creators are properly bound.
The sticky note action creators are correctly integrated into the bindActionCreators call.
104-104: LGTM! Props are properly passed down.
The stickyNotes prop is correctly passed to the GraphWrapped component. Please verify that the GraphWrapped component is updated to handle this new prop appropriately.
designer/server/src/main/scala/pl/touk/nussknacker/ui/config/FeatureTogglesConfig.scala (3)
10-10: LGTM: Import statement is correctly placed
The import for StickyNotesSettings follows the project's import organization pattern.
32-33: Verify field ordering in case class
The new stickyNotesSettings field is added at the end of the case class, which is good for backward compatibility. However, consider grouping it with related settings fields for better organization.
83-83: LGTM: Constructor parameter correctly added
The stickyNotesSettings parameter is properly added to the constructor call, maintaining consistency with the case class definition.
designer/client/src/actions/nk/process.ts (2)
9-11: LGTM: Imports are appropriate for the new functionality
The new imports are well-organized and necessary for the sticky notes feature implementation.
23-23: LGTM: Appropriate placement of sticky notes fetch
The sticky notes fetch is correctly placed after fetching process details and before displaying the process.
designer/client/src/reducers/selectors/graph.ts (2)
11-11: LGTM! Clean import addition.
The import follows the established pattern and provides proper typing for the sticky notes feature.
74-74: LGTM! Well-implemented selector.
The getStickyNotes selector is properly implemented with:
- Memoization using createSelector
- Appropriate fallback to empty array
- Correct typing with StickyNote[]
designer/client/src/components/graph/NodeDescriptionPopover.tsx (1)
8-8: LGTM: Clean import addition
The import statement for isStickyNoteElement is well-placed and follows proper conventions.
designer/server/src/main/resources/defaultDesignerConfig.conf (2)
124-124: LGTM! Toolbar panel placement is well-structured.
The sticky notes panel is logically placed in the topLeft section, between search/tips and creator panels. The configuration follows the existing pattern and the hidden property correctly handles archived scenarios.
214-217: Verify the sticky notes limits.
Please confirm if these limits are appropriate for your use case:
maxContentLength: 5000characters per notemaxNotesCount: 5notes total
Consider:
- Are these limits sufficient for typical user scenarios?
- Have these values been tested with real-world usage patterns?
- Should these limits be configurable via environment variables like other settings?
designer/client/src/components/graph/graphStyledWrapper.ts (1)
2-2: LGTM: Import changes are appropriate.
The new imports for alpha and stickyNotePath are correctly added and necessary for the sticky notes feature implementation.
Also applies to: 5-5
designer/client/src/reducers/graph/utils.ts (1)
8-9: LGTM: Imports are appropriate
The new imports are specific and necessary for the sticky notes functionality.
designer/server/src/test/scala/pl/touk/nussknacker/test/utils/domain/ScenarioHelper.scala (2)
13-13: LGTM! Imports are properly organized.
The new imports are necessary for the sticky notes functionality and scenario management.
Also applies to: 19-23, 26-26
48-48: LGTM! Repository field follows established patterns.
The stickyNotesRepository is properly initialized and follows the same patterns as other repository fields in the class.
designer/client/package.json (2)
24-24: Good choice of DOMPurify for XSS prevention!
Adding DOMPurify is a crucial security measure for sanitizing user-generated HTML content in sticky notes, protecting against XSS attacks.
24-24: Verify DOMPurify version for updates
Please verify if version 3.2.0 is the latest stable version and check for any security advisories or patches.
designer/server/src/main/scala/pl/touk/nussknacker/ui/server/AkkaHttpBasedRouteProvider.scala (1)
402-409: LGTM! Well-structured service initialization.
The StickyNotesApiHttpService is properly initialized with all necessary dependencies and follows the established pattern of other services in the codebase.
designer/client/src/http/HttpService.ts (2)
34-34: LGTM!
The import statement is well-organized and correctly imports the necessary types for sticky notes functionality.
740-746: LGTM!
The getStickyNotes method is well-implemented with proper typing, error handling, and query parameter usage.
designer/server/src/test/scala/pl/touk/nussknacker/ui/api/ProcessesResourcesSpec.scala (2)
55-55: LGTM!
The import statement for StickyNotesPanel is correctly placed and follows the existing import organization pattern.
1229-1230: LGTM!
The test case correctly verifies the toolbar configuration with the addition of CreatorPanel and StickyNotesPanel, maintaining proper test coverage for the toolbar functionality.
docs-internal/api/nu-designer-openapi.yaml (1)
2530-2929: Well-structured REST API endpoints for sticky notes management!
The endpoints follow REST best practices with proper HTTP methods:
- GET for retrieval
- POST for creation
- PUT for updates
- DELETE for removal
The error handling is comprehensive, covering:
- 400 for invalid requests
- 401 for authentication issues
- 403 for authorization issues
- 404 for not found scenarios
- 501 for unsupported features
designer/server/src/main/scala/pl/touk/nussknacker/ui/process/repository/stickynotes/StickyNotesRepository.scala (1)
43-44:
Remove unnecessary trailing comma in method parameters
There is a trailing comma after scenarioVersionId: VersionId in the parameter list of the updateStickyNote method, which could cause a syntax error.
Apply this diff to fix the issue:
def updateStickyNote(
noteId: StickyNoteId,
content: String,
layoutData: LayoutData,
color: String,
dimensions: Dimensions,
targetEdge: Option[String],
- scenarioVersionId: VersionId,
+ scenarioVersionId: VersionId
)(implicit user: LoggedUser): DB[Int]Likely invalid or redundant comment.
designer/server/src/main/scala/pl/touk/nussknacker/ui/db/entity/StickyNotesEntityFactory.scala (2)
27-64: Database table and entity mapping are correctly defined
The StickyNotesEntity class accurately defines the database table schema for sticky notes, with all columns appropriately typed and the foreign key constraint correctly established with processVersionsTable.
96-112: The entity data class and conversion method are properly implemented
The StickyNoteEventEntityData case class correctly represents the entity data, and the toStickyNote method accurately converts it into a StickyNote object, ensuring consistency between the database layer and the application layer.
designer/client/src/components/graph/EspNode/stickyNote.ts (2)
75-75: Review the Use of a Disabled Textarea
The textarea element is rendered but disabled:
<textarea @selector="markdown-editor" class="sticky-note-markdown-editor" name="markdown-editor" autocomplete="off" disabled="disabled"></textarea>If user input is not intended here, consider removing the textarea to simplify the DOM and avoid confusion. If it will be enabled in future implementations, you might want to add a comment explaining its current purpose.
55-62:
Verify Security of Custom Renderer Functions with DOMPurify
While overriding the link and image methods in the marked renderer, ensure that DOMPurify effectively sanitizes all user-provided content, especially the href attribute. This is crucial to prevent XSS attacks through malicious links or image sources. Consider specifying additional DOMPurify configurations if necessary.
designer/client/src/components/graph/EspNode/stickyNoteElements.ts (4)
94-95: Verify the event mapping for 'touchcancel'
In the documentEvents, the 'touchcancel' event is mapped to onPointerUp, while 'touchend' is mapped to onPointerUpCustom. Please verify if this is intentional. For consistent behavior across touch devices, consider mapping 'touchcancel' to onPointerUpCustom if appropriate.
60-62: Ensure proper handling of the 'CELL_DELETED' event
The removeButtonTool triggers the Events.CELL_DELETED when the sticky note is removed. Confirm that this event is correctly handled elsewhere in the application to manage cleanup processes related to the sticky note deletion.
71-71: Confirm the SVG path data for the resize handle
The d attribute for the resize handle path is set to "M 4 0 L 4 4 L 0 4 L 0 5 L 5 5 L 5 0 L 4 0". Ensure that this SVG path renders the desired icon consistently across different browsers and devices.
133-133: Validate the setting of sticky note content
The line stickyNoteModel.attr(\${MARKDOWN_EDITOR_NAME}/props/value`, stickyNote.content);sets the content of the sticky note. Verify thatMARKDOWN_EDITOR_NAME` correctly references the intended attribute path and that the content renders properly within the sticky note.
designer/server/src/main/scala/pl/touk/nussknacker/ui/process/repository/stickynotes/DbStickyNotesRepository.scala (2)
156-156:
Remove unnecessary trailing comma in parameter list
There's a trailing comma at the end of the parameter list in line 156, which may cause a syntax error.
Apply this diff to fix the syntax error:
- implicit executionContext: ExecutionContext,
+ implicit executionContext: ExecutionContext
): StickyNotesRepository = new DbStickyNotesRepository(dbRef, clock)Likely invalid or redundant comment.
122-122:
Remove unnecessary trailing comma in parameter list
There's a trailing comma at the end of the parameter list in line 122, which may cause a syntax error.
Apply this diff to fix the syntax error:
- scenarioVersionId: VersionId,
+ scenarioVersionId: VersionId
)(Likely invalid or redundant comment.
designer/server/src/main/scala/pl/touk/nussknacker/ui/db/entity/StickyNotesEntityFactory.scala
Show resolved
Hide resolved
designer/server/src/main/scala/pl/touk/nussknacker/ui/db/entity/StickyNotesEntityFactory.scala
Show resolved
Hide resolved
designer/server/src/main/scala/pl/touk/nussknacker/ui/api/StickyNotesApiHttpService.scala
Show resolved
Hide resolved
designer/server/src/main/scala/pl/touk/nussknacker/ui/api/StickyNotesApiHttpService.scala
Show resolved
Hide resolved
designer/server/src/main/scala/pl/touk/nussknacker/ui/api/StickyNotesApiHttpService.scala
Outdated
Show resolved
Hide resolved
|
created: #7276 |
philemone
force-pushed
the
stickyNotes
branch
2 times, most recently
from
b991b11 to
99de706
Compare
There was a problem hiding this comment.
Actionable comments posted: 20
🧹 Outside diff range and nitpick comments (35)
designer/client/src/types/component.ts (1)
8-8: Consider adding JSDoc documentation.Since this property is part of a larger sticky notes feature and is used across multiple components, adding documentation would help other developers understand its purpose and usage.
+ /** + * Function that determines if the component should be disabled. + * Used primarily for controlling interactive states of sticky note components. + * @returns {boolean} True if the component should be disabled, false otherwise + */ disabled?: () => boolean;designer/client/src/components/toolbars/creator/StickyNoteComponent.tsx (1)
4-4: Add type annotation and documentation for the note model.The
noteModelconstant would benefit from:
- An explicit type annotation
- JSDoc documentation explaining its purpose and usage
- Consider using a more unique ID generation mechanism
Consider applying these improvements:
+/** Model representing a sticky note template for the toolbox. + * Used as a prototype for creating new sticky notes. + */ +const noteModel: { + id: string; + type: typeof StickyNoteType; + isDisabled: boolean; +} = { id: "StickyNoteToAdd", type: StickyNoteType, isDisabled: false };designer/client/src/containers/theme/helpers.ts (2)
4-4: Consider relocating shared constants to a dedicated constants fileThe
STICKY_NOTE_DEFAULT_COLORconstant is imported from a component file but used in theme helpers. This creates a dependency from the theme layer on the component layer, which could make the codebase harder to maintain.Consider moving shared constants to a dedicated constants file (e.g.,
src/constants/theme.ts) to maintain better separation of concerns.
19-26: Add parameter validation and documentationThe function would benefit from proper JSDoc documentation and input validation.
Consider adding documentation and validation:
+/** + * Generates a background color for sticky notes based on the provided color or falls back to default + * @param theme - The MUI theme object + * @param color - The desired sticky note color (must be a valid CSS color) + * @returns An augmented color object from the theme palette + */ export function getStickyNoteBackgroundColor(theme: Theme, color: string) { + if (!theme || !color) { + throw new Error('Theme and color parameters are required'); + } const isValidColor = CSS.supports("color", color); return theme.palette.augmentColor({ color: { main: isValidColor ? color : STICKY_NOTE_DEFAULT_COLOR, }, }); }designer/client/src/components/graph/GraphPartialsInTS/cellUtils.ts (2)
2-2: Consider using absolute imports for better maintainabilityThe relative import with multiple parent directory traversals (
../../../) can make the code harder to maintain. Consider configuring TypeScript path aliases to use absolute imports instead.Example configuration in
tsconfig.json:{ "compilerOptions": { "baseUrl": ".", "paths": { "@common/*": ["src/common/*"] } } }Then the import could be simplified to:
import { StickyNote } from "@common/StickyNote";
12-14: Extract string literal into a named constantThe string literal
"stickyNote.StickyNoteElement"should be extracted into a named constant for better maintainability and reusability.+const STICKY_NOTE_ELEMENT_TYPE = "stickyNote.StickyNoteElement"; export function isStickyNoteElement(el: dia.Cell): el is shapes.devs.Model { - return isElement(el) && el.get("type") === `stickyNote.StickyNoteElement`; + return isElement(el) && el.get("type") === STICKY_NOTE_ELEMENT_TYPE; }designer/server/src/main/scala/pl/touk/nussknacker/ui/process/repository/stickynotes/StickyNotesRepository.scala (3)
17-20: Add documentation for the trait and clock usage.Consider adding ScalaDoc to:
- Document the trait's purpose and responsibility
- Explain how the
clockfield is used (e.g., for audit trails, timestamps)- Specify any threading or synchronization requirements for implementations
+/** + * Repository trait for managing sticky notes in scenarios. + * Provides CRUD operations for sticky notes with proper audit trailing. + * + * @note The clock field is used for tracking creation and modification timestamps + * and should be consistent across the application. + */ trait StickyNotesRepository {
21-30: Consider adding pagination support for findStickyNotes.For scenarios with many sticky notes, fetching all notes at once could impact performance. Consider:
- Adding pagination parameters (offset/limit or cursor-based)
- Returning a paginated response type
def findStickyNotes( scenarioId: ProcessId, - scenarioVersionId: VersionId + scenarioVersionId: VersionId, + offset: Int, + limit: Int ): DB[Seq[StickyNote]]
51-54: Consider implementing soft delete.Instead of hard deleting sticky notes, consider implementing soft delete to:
- Maintain audit history
- Enable potential recovery
- Track deletion patterns
+ /** + * Marks a sticky note as deleted without removing it from the database. + * @return Number of affected rows (should be 1 if successful) + */ + def softDeleteStickyNote( + noteId: StickyNoteId, + deletedAt: java.time.Instant = clock.instant() + )(implicit user: LoggedUser): DB[Int]designer/client/src/reducers/selectors/settings.ts (1)
17-17: Consider adding explicit return type for better type safety.The implementation follows the established pattern and is consistent with other feature settings selectors. However, consider adding an explicit return type for better maintainability and type safety.
-export const getStickyNotesSettings = createSelector(getFeatureSettings, (s) => s?.stickyNotesSettings); +export const getStickyNotesSettings = createSelector(getFeatureSettings, (s): StickyNotesSettings | undefined => s?.stickyNotesSettings);designer/client/src/components/toolbars/creator/ToolboxComponentGroup.tsx (2)
72-78: Consider optimizing the disabled state calculationThe
disabledprop calculation could be optimized to prevent unnecessary function calls on each render.Consider memoizing the disabled state:
const elements = useMemo( () => componentGroup.components.map((component) => { + const isDisabled = useMemo( + () => component.disabled?.() ?? false, + [component.disabled] + ); return ( <Tool nodeModel={component.node} label={component.label} key={component.componentId} highlights={highlights} - disabled={component.disabled ? component.disabled() : false} + disabled={isDisabled} /> ); }), [highlights, componentGroup.components], );
72-78: Document the disabled state logicPlease add documentation explaining when and why a tool would be disabled, as this seems to be part of the sticky notes feature implementation.
Add a comment block above the Tool component:
/** * Renders a Tool component with optional disabled state. * The disabled state is determined by the component's disabled function, * which is typically used to prevent certain tools (e.g., sticky notes) * from being used under specific conditions. */designer/server/src/main/scala/pl/touk/nussknacker/ui/db/entity/StickyNotesEntityFactory.scala (1)
96-112: Consider adding input validation and documenting field mappingThe
toStickyNoteconversion drops several fields (noteCorrelationId, eventType, scenarioId, scenarioVersionId). While this might be intentional, it should be documented. Also, consider adding input validation for critical fields.Example validation implementation:
final case class StickyNoteEventEntityData( id: StickyNoteId, noteCorrelationId: StickyNoteCorrelationId, content: String, layoutData: LayoutData, color: String, dimensions: Dimensions, targetEdge: Option[String], eventCreator: String, eventDate: Timestamp, eventType: StickyNoteEvent, scenarioId: ProcessId, scenarioVersionId: VersionId ) { require(content.nonEmpty, "Content cannot be empty") require(content.length <= 1000, "Content exceeds maximum length") require(color.matches("^#[0-9A-Fa-f]{6}$") || color.matches("^[a-zA-Z]+$"), "Invalid color format") /** Converts to StickyNote DTO, excluding audit and correlation fields */ def toStickyNote: StickyNote = StickyNote(id, content, layoutData, color, dimensions, targetEdge, eventCreator, eventDate.toInstant) }designer/server/src/main/scala/pl/touk/nussknacker/ui/api/description/stickynotes/Dtos.scala (3)
44-54: Consider adding validation for color format.The
colorfield inStickyNoteshould validate the color format (e.g., hex color code).Consider adding a color validator:
object StickyNote { private def validateColor(color: String): Boolean = color.matches("^#([A-Fa-f0-9]{6}|[A-Fa-f0-9]{3})$") def apply(noteId: StickyNoteId, content: String, layoutData: LayoutData, color: String, dimensions: Dimensions, targetEdge: Option[String], editedBy: String, editedAt: Instant): StickyNote = { require(validateColor(color), s"Invalid color format: $color") new StickyNote(noteId, content, layoutData, color, dimensions, targetEdge, editedBy, editedAt) } }
91-117: Add documentation and fix formatting inconsistencies.The error handling is comprehensive but would benefit from documentation and consistent formatting.
Consider these improvements:
object StickyNotesError { + /** Thrown when the specified scenario doesn't exist */ final case class NoScenario(scenarioName: ProcessName) extends StickyNotesError + + /** Thrown when user doesn't have required permissions */ final case object NoPermission extends StickyNotesError with CustomAuthorizationError + + /** Thrown when note content exceeds maximum length */ final case class StickyNoteContentTooLong(count: Int, max: Int) extends StickyNotesError - final case class StickyNoteCountLimitReached(max: Int) extends StickyNotesError - final case class NoStickyNote(noteId: StickyNoteId) extends StickyNotesError + + /** Thrown when scenario reaches maximum allowed notes */ + final case class StickyNoteCountLimitReached(max: Int) extends StickyNotesError + + /** Thrown when specified sticky note doesn't exist */ + final case class NoStickyNote(noteId: StickyNoteId) extends StickyNotesError
1-119: Overall architecture looks good with room for improvements.The DTOs and error handling are well-structured. Consider these architectural improvements:
- Add a validation layer to enforce business rules (dimensions, colors, etc.)
- Consider using refined types for validated values
- Add comprehensive ScalaDoc documentation
- Consider adding property-based tests for JSON serialization/deserialization
designer/client/src/components/graph/EspNode/stickyNote.ts (3)
10-17: Consider reducing maximum dimensionsThe maximum dimensions (3000x3000) seem excessive and could potentially impact performance or be used for DOM-based attacks. Consider reducing these to more reasonable values (e.g., 800x600) to prevent abuse while maintaining usability.
export const STICKY_NOTE_CONSTRAINTS = { MIN_WIDTH: 100, - MAX_WIDTH: 3000, + MAX_WIDTH: 800, DEFAULT_WIDTH: 300, MIN_HEIGHT: 100, - MAX_HEIGHT: 3000, + MAX_HEIGHT: 600, DEFAULT_HEIGHT: 250, } as const;
64-81: Improve error handling and clarify disabled stateTwo suggestions for improvement:
- The error message could be more user-friendly and include the actual error details
- The disabled textarea might be confusing - consider adding a comment explaining why it's disabled
try { parsed = DOMPurify.sanitize(marked.parse(stickyNote.content, { renderer })); } catch (error) { console.error("Failed to parse markdown:", error); - parsed = "Error: Could not parse content. See error logs in console"; + parsed = `Error: Could not parse markdown content. ${error.message}`; } const singleMarkupNode = util.svg/* xml */ ` <foreignObject @selector="foreignObject"> <div @selector="sticky-note-content" class="sticky-note-content"> + <!-- Textarea is disabled by default. It becomes enabled when the note is in edit mode --> <textarea @selector="${MARKDOWN_EDITOR_NAME}" class="sticky-note-markdown-editor" name="${MARKDOWN_EDITOR_NAME}" autocomplete="off" disabled="disabled"></textarea>
93-97: Use STICKY_NOTE_DEFAULT_COLOR constantFor consistency, use the
STICKY_NOTE_DEFAULT_COLORconstant instead of hardcoding the color value.body: { refD: stickyNotePath, strokeWidth: 2, - fill: "#eae672", + fill: STICKY_NOTE_DEFAULT_COLOR, filter: {designer/client/src/components/toolbars/creator/ToolBox.tsx (1)
118-122: Optimize useMemo dependenciesThe
stickyNoteToolGroupuseMemo has unnecessary dependencies:
propsis included but not used in the computationt(translation function) is used but not included in the dependenciesConsider updating the dependency array:
-const stickyNoteToolGroup = useMemo(() => stickyNoteComponentGroup(pristine), [pristine, props, t]); +const stickyNoteToolGroup = useMemo(() => stickyNoteComponentGroup(pristine), [pristine, t]);designer/client/src/components/graph/EspNode/stickyNoteElements.ts (2)
65-118: Consider optimizing resize calculations and adding error handling.The resize tool implementation could benefit from the following improvements:
- Debounce the resize calculations to improve performance during drag operations
- Add error handling for resize operations
Consider applying these improvements:
onPointerUpCustom: function (evt: dia.Event) { this.onPointerUp(evt); - stickyNoteModel.trigger(Events.CELL_RESIZED, stickyNoteModel); + try { + stickyNoteModel.trigger(Events.CELL_RESIZED, stickyNoteModel); + } catch (error) { + console.error('Failed to resize sticky note:', error); + // Optionally: Revert to previous size + } },
16-136: Consider implementing the Observer pattern for state changes.The current implementation directly triggers events for state changes. Consider implementing the Observer pattern to better manage state updates and make the code more maintainable. This would also make it easier to add features like undo/redo functionality in the future.
Key benefits:
- Better separation of concerns
- Easier testing
- More maintainable state management
- Simplified addition of new features like undo/redo
designer/server/src/test/scala/pl/touk/nussknacker/test/utils/domain/ScenarioHelper.scala (2)
85-97: Add ScalaDoc comments for the new test utility methodsConsider adding ScalaDoc comments to document the purpose and usage of these test utility methods. This would help other developers understand how to use them in their tests.
Example documentation:
+ /** + * Updates an existing scenario with new content and returns the ProcessUpdated result. + * Useful for testing scenario update operations. + * + * @param scenarioName the name of the scenario to update + * @param newScenario the new scenario content + * @return ProcessUpdated containing the update result + */ def updateScenario( scenarioName: ProcessName, newScenario: CanonicalProcess ): ProcessUpdated = { updateAndGetScenarioVersions(scenarioName, newScenario).futureValue } + /** + * Adds a sticky note to an existing scenario. + * Useful for testing sticky notes functionality. + * + * @param scenarioName the name of the scenario to add the sticky note to + * @param request the sticky note details + * @return the correlation ID of the created sticky note + */ def addStickyNote( scenarioName: ProcessName, request: StickyNoteAddRequest ): StickyNoteCorrelationId = { addStickyNoteForScenario(scenarioName, request).futureValue }
222-238: Consider making version increment configurableThe
increaseVersionWhenJsonNotChangedis hardcoded totrue, which forces a version increment even when the JSON content hasn't changed. Consider making this configurable through the method parameters to support different test scenarios.def updateAndGetScenarioVersions( scenarioName: ProcessName, - newScenario: CanonicalProcess + newScenario: CanonicalProcess, + increaseVersionWhenJsonNotChanged: Boolean = true ): Future[ProcessUpdated] = { for { scenarioId <- futureFetchingScenarioRepository.fetchProcessId(scenarioName).map(_.get) action = UpdateProcessAction( scenarioId, newScenario, comment = None, labels = List.empty, - increaseVersionWhenJsonNotChanged = true, + increaseVersionWhenJsonNotChanged = increaseVersionWhenJsonNotChanged, forwardedUserName = None ) processUpdated <- dbioRunner.runInTransaction(writeScenarioRepository.updateProcess(action)) } yield processUpdated }designer/client/src/reducers/graph/reducer.ts (1)
249-260: Consider handling selection state for sticky notesThe node-related actions (
NODE_ADDED,NODES_WITH_EDGES_ADDED) update the selection state, but sticky note actions don't. Consider whether sticky notes should also update the selection state for consistency in user experience.case "STICKY_NOTES_UPDATED": { const { stickyNotes, layout } = prepareNewStickyNotesWithLayout(state, action.stickyNotes); return { ...addStickyNotesWithLayout(state, { stickyNotes, layout }), + selectionState: stickyNotes.map(note => note.id) }; }designer/client/src/components/graph/Graph.tsx (3)
372-379: Consider extracting event handlers to a separate methodThe event handlers for sticky notes could be organized better by extracting them into a dedicated method like
bindStickyNoteEvents(), similar to howbindPaperEvents()is structured.
470-489: Address the TODO comment about configurationThe sticky note constraints should be moved to a configuration file for better maintainability and reusability.
Would you like me to help create a configuration file for these constraints?
Line range hint
1-853: Consider implementing sticky notes as a pluginThe sticky note functionality could be implemented as a separate plugin, similar to how PanZoomPlugin and RangeSelectPlugin are implemented. This would:
- Improve code organization
- Make the feature more modular and reusable
- Reduce the complexity of the main Graph component
- Make testing easier
This refactoring could be considered for a future improvement.
docs-internal/api/nu-designer-openapi.yaml (1)
2530-2929: Consider enhancing error responses and examples for sticky notes endpoints.The API endpoints are well-structured, but consider the following improvements:
Add specific error response examples for content validation:
- Maximum content length exceeded
- Invalid color format
- Invalid dimensions
- Invalid layout coordinates
Enhance success response examples to show:
- Notes with different colors
- Notes with various content formats
- Edge-attached notes
designer/server/src/main/scala/pl/touk/nussknacker/ui/api/description/StickyNotesApiEndpoints.scala (4)
52-85: Refine endpoint by includingscenarioVersionIdin the pathCurrently, the
scenarioVersionIdis passed as a query parameter in thestickyNotesGetEndpoint. For improved clarity and consistency with RESTful conventions, consider includingscenarioVersionIdas part of the path parameters. This adjustment aligns the endpoint structure with typical RESTful practices.Apply this diff to adjust the endpoint:
.get - .in("processes" / path[ProcessName]("scenarioName") / "stickyNotes" / query[VersionId]("scenarioVersionId")) + .in("processes" / path[ProcessName]("scenarioName") / "versions" / path[VersionId]("scenarioVersionId") / "stickyNotes")
37-37: Remove unusedencodervariable to clean up the codeThe
encodervariable is declared but not utilized within this class. Removing it will tidy up the codebase and prevent potential confusion.Apply this diff to remove the unused variable:
-lazy val encoder: Encoder[TypingResult] = TypingResult.encoder
6-6: Eliminate unused imports to improve code clarityThe import statement for
pl.touk.nussknacker.engine.api.typed.typing._is not used in this file. Removing unused imports enhances readability and maintains a clean codebase.Apply this diff to remove the unused import:
-import pl.touk.nussknacker.engine.api.typed.typing._
184-189: Ensure consistent error message formatting in examplesIn the
noStickyNoteExample, the summary states "No sticky note with id: 3 was found." For consistency and clarity across the API, consider aligning the error message format with other examples and ensuring that all error messages follow a uniform structure.designer/client/src/http/HttpService.ts (2)
700-700: Address the TODO and parameterize the default sticky note colorThere is a TODO comment to add configuration for the default sticky note color. To enhance flexibility and eliminate TODO comments in the codebase, consider parameterizing the color with a default value.
Suggested change:
addStickyNote( scenarioName: string, scenarioVersionId: number, position: Position, dimensions: Dimensions, + color: string = STICKY_NOTE_DEFAULT_COLOR ) { const promise = api.post(`/processes/${encodeURIComponent(scenarioName)}/stickyNotes`, { scenarioVersionId, content: "", layoutData: position, - color: STICKY_NOTE_DEFAULT_COLOR, //TODO add config for default sticky note color? For now this is default. + color: color, dimensions: dimensions, }); // rest of the code }Would you like assistance in implementing this change or creating a configuration option for the default sticky note color?
703-706: Refactor duplicated error handling codeThe error handling code in the
addStickyNote,deleteStickyNote,updateStickyNote, andgetStickyNotesmethods is duplicated. Consider refactoring this pattern into a helper method to improve maintainability and reduce code duplication.Suggested approach:
Create a helper method:
private handleStickyNoteError( errorKey: string, defaultMessage: string, error: AxiosError, interpolationVars?: Record<string, unknown>, ) { this.#addError( i18next.t(errorKey, { defaultValue: defaultMessage, ...interpolationVars, }), error, true, ); }Update your methods as follows:
In
addStickyNote:promise.catch((error) => - this.#addError( - i18next.t("notification.error.failedToAddStickyNote", { - defaultValue: "Failed to add sticky note to scenario" - }), - error, - true, - ), + this.handleStickyNoteError( + "notification.error.failedToAddStickyNote", + "Failed to add sticky note to scenario", + error, + ), );Similarly update
deleteStickyNote,updateStickyNote, andgetStickyNotes.Also applies to: 710-717, 731-736, 742-744
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
⛔ Files ignored due to path filters (2)
designer/client/package-lock.jsonis excluded by!**/package-lock.jsondesigner/server/src/main/resources/web/static/assets/components/StickyNote.svgis excluded by!**/*.svg
📒 Files selected for processing (56)
designer/client/package.json(1 hunks)designer/client/src/actions/actionTypes.ts(1 hunks)designer/client/src/actions/nk/assignSettings.ts(1 hunks)designer/client/src/actions/nk/process.ts(3 hunks)designer/client/src/actions/notificationActions.tsx(1 hunks)designer/client/src/assets/json/nodeAttributes.json(1 hunks)designer/client/src/common/StickyNote.ts(1 hunks)designer/client/src/components/ComponentPreview.tsx(2 hunks)designer/client/src/components/StickyNotePreview.tsx(1 hunks)designer/client/src/components/graph/EspNode/stickyNote.ts(1 hunks)designer/client/src/components/graph/EspNode/stickyNoteElements.ts(1 hunks)designer/client/src/components/graph/Graph.tsx(13 hunks)designer/client/src/components/graph/GraphPartialsInTS/applyCellChanges.ts(2 hunks)designer/client/src/components/graph/GraphPartialsInTS/cellUtils.ts(2 hunks)designer/client/src/components/graph/NodeDescriptionPopover.tsx(2 hunks)designer/client/src/components/graph/ProcessGraph.tsx(4 hunks)designer/client/src/components/graph/StickyNoteElement.ts(1 hunks)designer/client/src/components/graph/fragmentGraph.tsx(1 hunks)designer/client/src/components/graph/graphStyledWrapper.ts(2 hunks)designer/client/src/components/graph/node-modal/node/FragmentContent.tsx(3 hunks)designer/client/src/components/graph/types.ts(3 hunks)designer/client/src/components/graph/utils/graphUtils.ts(1 hunks)designer/client/src/components/toolbars/creator/ComponentIcon.tsx(2 hunks)designer/client/src/components/toolbars/creator/StickyNoteComponent.tsx(1 hunks)designer/client/src/components/toolbars/creator/ToolBox.tsx(3 hunks)designer/client/src/components/toolbars/creator/ToolboxComponentGroup.tsx(1 hunks)designer/client/src/containers/theme/helpers.ts(2 hunks)designer/client/src/http/HttpService.ts(3 hunks)designer/client/src/reducers/graph/reducer.ts(2 hunks)designer/client/src/reducers/graph/types.ts(2 hunks)designer/client/src/reducers/graph/utils.ts(3 hunks)designer/client/src/reducers/selectors/graph.ts(2 hunks)designer/client/src/reducers/selectors/settings.ts(1 hunks)designer/client/src/types/component.ts(1 hunks)designer/client/src/types/node.ts(1 hunks)designer/client/src/types/stickyNote.ts(1 hunks)designer/server/src/main/resources/defaultDesignerConfig.conf(1 hunks)designer/server/src/main/scala/db/migration/V1_060__CreateStickyNotesDefinition.scala(1 hunks)designer/server/src/main/scala/db/migration/hsql/V1_060__CreateStickyNotes.scala(1 hunks)designer/server/src/main/scala/db/migration/postgres/V1_060__CreateStickyNotes.scala(1 hunks)designer/server/src/main/scala/pl/touk/nussknacker/ui/api/SettingsResources.scala(3 hunks)designer/server/src/main/scala/pl/touk/nussknacker/ui/api/StickyNotesApiHttpService.scala(1 hunks)designer/server/src/main/scala/pl/touk/nussknacker/ui/api/description/StickyNotesApiEndpoints.scala(1 hunks)designer/server/src/main/scala/pl/touk/nussknacker/ui/api/description/stickynotes/Dtos.scala(1 hunks)designer/server/src/main/scala/pl/touk/nussknacker/ui/api/description/stickynotes/StickyNoteEvent.scala(1 hunks)designer/server/src/main/scala/pl/touk/nussknacker/ui/config/FeatureTogglesConfig.scala(4 hunks)designer/server/src/main/scala/pl/touk/nussknacker/ui/db/NuTables.scala(1 hunks)designer/server/src/main/scala/pl/touk/nussknacker/ui/db/entity/StickyNotesEntityFactory.scala(1 hunks)designer/server/src/main/scala/pl/touk/nussknacker/ui/process/repository/stickynotes/DbStickyNotesRepository.scala(1 hunks)designer/server/src/main/scala/pl/touk/nussknacker/ui/process/repository/stickynotes/StickyNotesRepository.scala(1 hunks)designer/server/src/main/scala/pl/touk/nussknacker/ui/server/AkkaHttpBasedRouteProvider.scala(4 hunks)designer/server/src/test/resources/config/common-designer.conf(1 hunks)designer/server/src/test/scala/pl/touk/nussknacker/test/base/it/WithSimplifiedConfigScenarioHelper.scala(2 hunks)designer/server/src/test/scala/pl/touk/nussknacker/test/utils/domain/ScenarioHelper.scala(4 hunks)designer/server/src/test/scala/pl/touk/nussknacker/ui/api/StickyNotesApiHttpServiceBusinessSpec.scala(1 hunks)docs-internal/api/nu-designer-openapi.yaml(3 hunks)
🚧 Files skipped from review as they are similar to previous changes (32)
- designer/client/package.json
- designer/client/src/actions/actionTypes.ts
- designer/client/src/actions/notificationActions.tsx
- designer/client/src/assets/json/nodeAttributes.json
- designer/client/src/common/StickyNote.ts
- designer/client/src/components/ComponentPreview.tsx
- designer/client/src/components/StickyNotePreview.tsx
- designer/client/src/components/graph/GraphPartialsInTS/applyCellChanges.ts
- designer/client/src/components/graph/NodeDescriptionPopover.tsx
- designer/client/src/components/graph/ProcessGraph.tsx
- designer/client/src/components/graph/StickyNoteElement.ts
- designer/client/src/components/graph/fragmentGraph.tsx
- designer/client/src/components/graph/graphStyledWrapper.ts
- designer/client/src/components/graph/node-modal/node/FragmentContent.tsx
- designer/client/src/components/graph/types.ts
- designer/client/src/components/graph/utils/graphUtils.ts
- designer/client/src/components/toolbars/creator/ComponentIcon.tsx
- designer/client/src/reducers/graph/types.ts
- designer/client/src/reducers/selectors/graph.ts
- designer/client/src/types/node.ts
- designer/client/src/types/stickyNote.ts
- designer/server/src/main/scala/db/migration/V1_060__CreateStickyNotesDefinition.scala
- designer/server/src/main/scala/db/migration/hsql/V1_060__CreateStickyNotes.scala
- designer/server/src/main/scala/db/migration/postgres/V1_060__CreateStickyNotes.scala
- designer/server/src/main/scala/pl/touk/nussknacker/ui/api/StickyNotesApiHttpService.scala
- designer/server/src/main/scala/pl/touk/nussknacker/ui/api/description/stickynotes/StickyNoteEvent.scala
- designer/server/src/main/scala/pl/touk/nussknacker/ui/config/FeatureTogglesConfig.scala
- designer/server/src/main/scala/pl/touk/nussknacker/ui/db/NuTables.scala
- designer/server/src/main/scala/pl/touk/nussknacker/ui/process/repository/stickynotes/DbStickyNotesRepository.scala
- designer/server/src/test/resources/config/common-designer.conf
- designer/server/src/test/scala/pl/touk/nussknacker/test/base/it/WithSimplifiedConfigScenarioHelper.scala
- designer/server/src/test/scala/pl/touk/nussknacker/ui/api/StickyNotesApiHttpServiceBusinessSpec.scala
🧰 Additional context used
📓 Learnings (1)
designer/client/src/components/graph/Graph.tsx (1)
Learnt from: philemone
PR: TouK/nussknacker#7149
File: designer/client/src/components/graph/Graph.tsx:530-536
Timestamp: 2024-11-20T10:37:32.855Z
Learning: There is backend validation for sticky note content length, so frontend validation in the `updateStickyNote` method in `Graph.tsx` is unnecessary.
🔇 Additional comments (41)
designer/client/src/types/component.ts (1)
8-8: LGTM! Well-designed type definition.
The optional disabled property is correctly typed as a function returning boolean, allowing for dynamic evaluation of the disabled state. This is a good design pattern that enables context-dependent component disabling.
designer/client/src/components/toolbars/creator/StickyNoteComponent.tsx (2)
1-2: LGTM! Clean and focused imports.
The imports are well-organized and properly typed.
13-13: Verify the disabled state logic.
The component is disabled when the editor is not in a pristine state. Please verify if this is the intended behavior as it might affect user experience.
✅ Verification successful
Based on the gathered information, I can now provide the final response:
The disabled state logic is correct and intentional
The sticky note component is disabled when the process is not in a pristine state, which means there are unsaved changes or the process has been renamed (ProcessUtils.nothingToSave(state) && !isProcessRenamed(state)). This is a safety measure to prevent adding new sticky notes while there are pending changes, ensuring data consistency and preventing potential conflicts in the process state.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for other usages of disabled state in toolbox components
ast-grep --pattern 'disabled: () => $_'
Length of output: 41
Script:
#!/bin/bash
# Search for disabled state logic in the component and its related files
rg -l "disabled.*pristine" designer/client/src/components/
# Search for pristine state usage
rg "pristine" designer/client/src/components/ -A 2 -B 2
# Look for the component implementation
cat designer/client/src/components/toolbars/creator/StickyNoteComponent.tsx
Length of output: 4468
Script:
#!/bin/bash
# Let's check for the isPristine selector implementation to understand the state logic
rg "isPristine" -A 3 -B 3
# Check for any documentation or comments about pristine state
rg -l "pristine" . --type md
Length of output: 7029
designer/client/src/containers/theme/helpers.ts (1)
20-20: Add CSS.supports fallback
The current implementation might break in environments where CSS.supports is not available.
This issue was previously identified and the suggested fix is still applicable. Please refer to the previous review comment for the implementation details.
designer/client/src/components/graph/GraphPartialsInTS/cellUtils.ts (1)
12-21: Verify the integration of sticky note utility functions
Let's verify how these utility functions are integrated with the rest of the codebase.
✅ Verification successful
Based on the results, I can now generate the final response as the integration is well-verified through the codebase analysis.
Sticky note utility functions are properly integrated
The utility functions are well-integrated across the codebase:
isStickyNoteElementis used consistently in Graph.tsx for event handling, node selection, and sticky note-specific behaviorsgetStickyNoteCopyFromCellis properly used in Graph.tsx for managing sticky note state changes- Both functions are part of a complete sticky note implementation with dedicated components (StickyNoteElement, StickyNoteElementView) and proper type handling
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Check usage of sticky note utility functions and related components
# Check where these utility functions are used
echo "=== Checking usage of isStickyNoteElement ==="
rg "isStickyNoteElement" --type ts
echo -e "\n=== Checking usage of getStickyNoteCopyFromCell ==="
rg "getStickyNoteCopyFromCell" --type ts
echo -e "\n=== Checking related sticky note components ==="
rg "StickyNoteElement" --type ts
Length of output: 5564
designer/server/src/main/scala/pl/touk/nussknacker/ui/process/repository/stickynotes/StickyNotesRepository.scala (2)
1-16: LGTM! Well-organized imports and package structure.
The imports are clean, specific, and properly organized. Good use of the DB monad and proper separation of DTOs and entities.
31-50: Add input validation for content and dimensions.
Consider adding validation constraints for:
- Content length/format
- Color format (e.g., hex values)
- Dimensions boundaries
- Layout data validation
This will help prevent invalid data from being stored and ensure consistency.
designer/client/src/actions/nk/assignSettings.ts (2)
42-42:
Consider making stickyNotesSettings optional to avoid breaking changes.
Making stickyNotesSettings a required property in FeaturesSettings could break existing configurations. Consider adding a ? to make it optional:
- stickyNotesSettings: StickyNotesSettings;
+ stickyNotesSettings?: StickyNotesSettings;Let's check for existing configuration files that might be affected:
45-48: 🛠️ Refactor suggestion
Add JSDoc documentation and consider type refinements.
The StickyNotesSettings type would benefit from documentation explaining the purpose and constraints of each field. Also, consider using more specific types for the numeric fields to ensure positive values.
Here's a suggested improvement:
+/**
+ * Configuration settings for sticky notes feature
+ * @property maxContentLength - Maximum number of characters allowed in a sticky note
+ * @property maxNotesCount - Maximum number of sticky notes allowed per scenario
+ * @property enabled - Whether the sticky notes feature is enabled
+ */
export type StickyNotesSettings = {
- maxContentLength: number;
- maxNotesCount: number;
+ maxContentLength: Branded<number, 'PositiveInt'>; // Ensure positive integer
+ maxNotesCount: Branded<number, 'PositiveInt'>; // Ensure positive integer
enabled: boolean;
};
+// Type brand for positive integers
+type Branded<T, Brand> = T & { __brand: Brand };
+
+// Helper function to ensure positive integers
+function asPositiveInt(n: number): Branded<number, 'PositiveInt'> {
+ if (!Number.isInteger(n) || n <= 0) {
+ throw new Error('Value must be a positive integer');
+ }
+ return n as Branded<number, 'PositiveInt'>;
+}Let's check if there are any existing validations for these fields:
designer/server/src/main/scala/pl/touk/nussknacker/ui/db/entity/StickyNotesEntityFactory.scala (4)
1-20: LGTM! Well-organized imports
The imports are logically grouped and all necessary for the implementation.
66-72: LGTM! Well-implemented type mappings
The custom column type mappings for StickyNoteEvent, StickyNoteId, and StickyNoteCorrelationId are implemented correctly.
92-93: LGTM! Correct table query definition
The table query is defined correctly using LTableQuery.
57-62: Review cascade delete impact
The foreign key relationship uses CASCADE for both update and delete operations. While this maintains referential integrity, it means that deleting a process version will automatically delete all associated sticky notes.
Consider if this is the desired behavior or if sticky notes should be preserved for audit purposes.
✅ Verification successful
Let me analyze the cascade behavior in other tables to understand if this is a consistent pattern. I'll check the schema definitions of these tables.
Let me try a different approach to check the table definitions.
Based on the analysis of the codebase, I can see that CASCADE delete behavior is consistently used across related tables where child records should be removed when the parent is deleted. Let me check the specific relationships between process versions and other entities.
Let me check the complete relationships between sticky notes and other entities to understand the data model better.
Based on the analysis of the codebase, I can now provide a final response regarding the cascade delete behavior of sticky notes:
Cascade delete is the appropriate behavior here
The cascade delete behavior for sticky notes is consistent with the application's design and data model for several reasons:
- Sticky notes are version-specific annotations - they are tied to specific scenario versions through a foreign key relationship (
sticky_notes_scenario_version_fk). - The migration script (V1_060__CreateStickyNotesDefinition.scala) explicitly defines this relationship with CASCADE delete.
- Other related entities like tags and scenario labels also use CASCADE delete, showing this is a deliberate pattern for version-specific metadata.
- Sticky notes are UI elements for collaboration/documentation rather than critical business data that needs to be preserved for audit purposes.
The current implementation ensures data consistency by automatically cleaning up associated sticky notes when a process version is deleted, preventing orphaned annotations.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Check if there are other tables with similar cascade behavior
rg -A 2 "ForeignKeyAction.Cascade" --type scala
Length of output: 2371
Script:
#!/bin/bash
# Check the schema definitions and relationships of these tables
ast-grep --pattern 'class $_(Table)? extends Table[$_] {
$$$
}'
Length of output: 67
Script:
#!/bin/bash
# Look for table definitions and their relationships
rg -A 10 "class.*extends.*Table" --type scala
Length of output: 70357
Script:
#!/bin/bash
# Check foreign key relationships to process_versions table
rg "foreignKey.*process.*version" --type scala -A 3
Length of output: 623
Script:
#!/bin/bash
# Look for sticky notes table relationships and usage
rg "sticky.*notes.*" --type scala -A 5
Length of output: 23827
designer/server/src/main/scala/pl/touk/nussknacker/ui/api/description/stickynotes/Dtos.scala (2)
1-16: LGTM!
The imports are well-organized and all are actively used in the code.
89-89: Consider explicit configuration for LayoutData schema.
The derived schema for LayoutData might need explicit configuration to handle all cases correctly.
Let's verify the LayoutData structure:
designer/client/src/components/graph/EspNode/stickyNote.ts (2)
25-53: LGTM! Well-structured SVG definitions
The SVG element definitions are well-organized and follow JointJS best practices. The use of semantic selectors and proper attribute structuring will make maintenance easier.
67-67: Verify DOMPurify configuration
While DOMPurify is correctly used, we should verify if any additional configuration is needed for the specific use case.
designer/client/src/components/toolbars/creator/ToolBox.tsx (2)
5-5: LGTM: Import statements are well-structured
The new imports are properly organized and follow the project's conventions.
Also applies to: 14-16
112-113: LGTM: Clean implementation of sticky notes state management
The Redux selectors are properly integrated, and the state management follows React best practices with appropriate use of hooks.
Also applies to: 118-118
designer/client/src/actions/nk/process.ts (1)
9-11: LGTM: Imports are appropriate for the new functionality.
The new imports support the sticky notes feature with necessary types and utilities.
designer/client/src/components/graph/EspNode/stickyNoteElements.ts (2)
1-14: LGTM! Clean imports and type definition.
The imports are well-organized and the ModelWithTool type clearly defines the structure needed for sticky note elements.
16-49:
Consider sanitizing the icon URL for XSS prevention.
While the stickyNoteIcon() function is used to generate the icon URL, it's good practice to sanitize URLs before using them in xlinkHref to prevent potential XSS attacks.
Consider sanitizing the URL:
- xlinkHref: stickyNoteIcon(),
+ xlinkHref: sanitizeUrl(stickyNoteIcon()),designer/server/src/main/scala/pl/touk/nussknacker/ui/api/SettingsResources.scala (2)
10-10: LGTM!
The import statement for StickyNotesSettings is correctly placed and properly scoped.
46-47: Consider adding validation for sticky notes settings.
While the implementation is functionally correct, consider adding validation similar to how DeploymentCommentSettings is handled. This would ensure the settings are properly validated before being used.
Let's check if other settings use validation:
designer/server/src/main/resources/defaultDesignerConfig.conf (2)
Line range hint 218-229: LGTM! The scenario label validation rules are well-defined and secure.
The validation rules enforce:
- Only alphanumeric characters, hyphens, and underscores
- Maximum length of 20 characters
These constraints help maintain clean and consistent scenario labels while preventing injection attacks.
213-217: LGTM! The sticky notes configuration looks secure and well-constrained.
The configuration includes appropriate limits for content length and note count to prevent resource exhaustion. The feature flag provides flexibility for enabling/disabling the functionality.
Let's verify if these limits are properly enforced in the implementation:
designer/client/src/reducers/graph/utils.ts (3)
8-9: LGTM: Imports are properly scoped
The new imports for sticky note types and utilities are correctly placed and properly scoped to the new functionality.
98-110: Previous review comments are still applicable
The issues identified in the previous reviews for these functions remain valid:
- Input validation is missing
- Duplicate IDs are not handled
- State is being overwritten instead of merged
Also applies to: 126-135
Line range hint 85-135: Verify sticky note ID conflicts with node IDs
Since sticky notes and nodes share the same layout space, we should verify that sticky note IDs don't conflict with node IDs.
✅ Verification successful
Let me analyze the results and generate the final response.
No ID conflicts between sticky notes and nodes
The verification shows clear separation between sticky note and node IDs:
- Sticky note IDs are created using
createStickyNoteIdfunction intypes/stickyNote.ts, which is consistently used across the codebase. - Node IDs are managed through
createUniqueNodeId/generateUniqueNodeIdfunctions with a different implementation and type (NodeIdis defined asstringwhile sticky notes usenumber). - The layout system correctly handles both types without conflicts, as they maintain separate type definitions and generation mechanisms.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Check for potential ID conflicts between nodes and sticky notes
# Search for node ID generation patterns
echo "Node ID generation patterns:"
rg -n "createUniqueNodeId|generateUniqueNodeId" --type ts
# Search for sticky note ID generation patterns
echo "Sticky note ID generation patterns:"
rg -n "createStickyNoteId" --type ts
# Search for ID type definitions to verify separation
echo "ID type definitions:"
ast-grep --pattern 'type $_ = $_' | rg -A 2 "NodeId|StickyNote"
Length of output: 6772
designer/server/src/test/scala/pl/touk/nussknacker/test/utils/domain/ScenarioHelper.scala (2)
13-13: LGTM: Clean imports and repository initialization
The new imports and repository initialization are well-organized and follow the existing patterns in the codebase.
Also applies to: 19-23, 26-26, 48-48
240-258: LGTM: Well-structured sticky note addition logic
The implementation properly handles the database transaction and correctly unpacks the sticky note request data.
designer/client/src/reducers/graph/reducer.ts (2)
12-12: LGTM: New utility imports follow existing patterns
The new sticky note utility imports maintain consistency with existing node-related utilities in both naming and organization.
Also applies to: 17-18
249-260: Verify undo/redo behavior for sticky notes
The sticky note actions will be included in the undo/redo history. Please verify that:
- Adding sticky notes can be undone
- Deleting sticky notes can be undone
- The layout state is properly restored on undo/redo
✅ Verification successful
Sticky notes are properly integrated with undo/redo functionality
Based on the evidence from the reducer configuration:
- Sticky note actions (
STICKY_NOTES_UPDATED,STICKY_NOTE_DELETED) are not in theexcludeActionlist - The undo/redo system is configured to track state changes through
combineFiltersthat compares the undoable state - The actions will be automatically included in the undo history as they modify the graph state
The implementation ensures that sticky note operations can be undone/redone as expected, maintaining consistency with other graph operations.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for tests covering undo/redo functionality for sticky notes
rg -l "sticky.*undo|undo.*sticky" --type ts
Length of output: 45
Script:
#!/bin/bash
# Let's first check the undo/redo configuration in the reducer
rg -A 10 "undoableConfig|undoable" designer/client/src/reducers/graph/reducer.ts
# Then check for sticky note related actions in the reducer
rg "STICKY_NOTE" designer/client/src/reducers/graph/reducer.ts
# Also check for any test files related to sticky notes
fd ".*\.test\.ts|.*\.spec\.ts" | xargs rg "sticky"
Length of output: 2028
Script:
#!/bin/bash
# Let's check what actions are excluded from undo/redo
rg -B5 -A5 "excludeAction" designer/client/src/reducers/graph/reducer.ts
# Check the implementation of sticky note actions to ensure they modify undoable state
ast-grep --pattern 'function addStickyNotesWithLayout($_) { $$$ }'
ast-grep --pattern 'function removeStickyNoteFromLayout($_) { $$$ }'
ast-grep --pattern 'function prepareNewStickyNotesWithLayout($_) { $$$ }'
Length of output: 1693
designer/server/src/main/scala/pl/touk/nussknacker/ui/server/AkkaHttpBasedRouteProvider.scala (5)
72-72: LGTM!
The import statement for DbStickyNotesRepository is correctly placed with other repository imports.
171-171: LGTM!
The stickyNotesRepository initialization follows the established pattern of other repository initializations in the file, using the same dependencies (dbRef and designerClock).
603-603: LGTM!
The stickyNotesApiHttpService is correctly registered in the nuDesignerApi alongside other API services.
Line range hint 402-603: Verify authorization checks for sticky notes operations
The service setup looks good, but let's ensure proper authorization checks are in place for sticky note operations.
Let's verify the authorization implementation:
402-409: Consider adding error handling for database operations
The StickyNotesApiHttpService initialization looks good, but since it's handling database operations through dbioRunner, consider adding explicit error handling for database failures.
Let's verify if error handling is implemented in the service:
designer/client/src/components/graph/Graph.tsx (2)
Line range hint 21-76: LGTM: Clean namespace integration for sticky notes
The sticky note components are properly integrated into the JointJS namespace, maintaining consistency with the existing architecture.
563-563: LGTM: Proper integration of sticky notes in component updates
The drawGraph method properly includes sticky notes in the update cycle.
designer/server/src/main/scala/pl/touk/nussknacker/ui/api/description/StickyNotesApiEndpoints.scala (1)
39-50: Verify the correctness of exampleInstantDate value
The exampleInstantDate is set using Instant.ofEpochMilli(1730136602), which corresponds to a date in 1970. Please verify if this epoch value represents the intended date or if an error has occurred. Using a more recent or representative date may make the examples clearer.
designer/client/src/components/toolbars/creator/ToolboxComponentGroup.tsx
Show resolved
Hide resolved
designer/server/src/main/scala/pl/touk/nussknacker/ui/db/entity/StickyNotesEntityFactory.scala
Show resolved
Hide resolved
...r/server/src/main/scala/pl/touk/nussknacker/ui/api/description/StickyNotesApiEndpoints.scala
Show resolved
Hide resolved
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Outside diff range and nitpick comments (1)
designer/client/cypress/e2e/creatorToolbar.cy.ts (1)
29-29: LGTM! Consider adding dedicated sticky notes test cases.While the addition fits well with the existing toolbar collapse test, consider adding dedicated test cases for sticky notes functionality:
- Verify sticky notes creation/deletion
- Test sticky notes persistence
- Validate XSS protection (given the new dompurify dependency)
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
⛔ Files ignored due to path filters (1)
designer/client/cypress/e2e/__image_snapshots__/electron/Linux/Creator toolbar should allow collapse (persist) and filtering #0.pngis excluded by!**/*.png
📒 Files selected for processing (2)
designer/client/cypress/e2e/creatorToolbar.cy.ts(1 hunks)designer/client/src/components/graph/EspNode/stickyNote.ts(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- designer/client/src/components/graph/EspNode/stickyNote.ts
🔇 Additional comments (1)
designer/client/cypress/e2e/creatorToolbar.cy.ts (1)
29-29: Verify integration with sticky notes components.
Let's verify that the required sticky notes components and actions are properly integrated.
✅ Verification successful
Sticky notes functionality is properly integrated and working as expected
The verification shows a complete and well-integrated sticky notes implementation:
- StickyNoteComponent is properly implemented with correct type definitions and Redux integration
- Reducer handles sticky note actions (STICKY_NOTES_UPDATED, STICKY_NOTE_DELETED) correctly
- Graph component includes proper sticky notes rendering and state management
- Preview component exists with proper styling and interactions
- HTTP service includes all necessary sticky notes API endpoints
- Layout and state management utilities are properly implemented
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Verify the existence of related sticky notes components and actions
# Check for sticky notes related files and components
echo "Checking for sticky notes related files..."
fd -e ts -e tsx "sticky" designer/client/src
# Check for sticky notes action types
echo "Checking for sticky notes action types..."
rg -A 2 "STICKY_NOTES_UPDATED|STICKY_NOTE_DELETED" designer/client/src
# Check for sticky notes components in toolbar
echo "Checking for sticky notes toolbar integration..."
rg -A 5 "stickyNotes" designer/client/src --type ts --type tsx
Length of output: 2273
Script:
#!/bin/bash
# Let's verify the sticky notes integration more thoroughly
# Check for sticky notes toolbar component implementation
echo "Checking StickyNoteComponent implementation..."
ast-grep --pattern 'export const StickyNoteComponent = $_' designer/client/src/components/toolbars/creator/StickyNoteComponent.tsx
# Check for sticky notes state handling
echo "Checking sticky notes state handling..."
rg -A 5 "stickyNotes" designer/client/src --type typescript
# Check for sticky notes preview component
echo "Checking StickyNotePreview implementation..."
ast-grep --pattern 'export const StickyNotePreview = $_' designer/client/src/components/StickyNotePreview.tsx
# Check for sticky notes type definitions
echo "Checking sticky notes types..."
cat designer/client/src/types/stickyNote.ts
Length of output: 888
Script:
#!/bin/bash
# Let's try one more verification with correct file type flags
# Check sticky notes component implementation
echo "Checking StickyNoteComponent content..."
cat designer/client/src/components/toolbars/creator/StickyNoteComponent.tsx
# Check sticky notes state handling with correct file type
echo "Checking sticky notes state handling..."
rg -A 5 "stickyNotes" designer/client/src --type ts
# Check sticky notes preview implementation
echo "Checking StickyNotePreview content..."
cat designer/client/src/components/StickyNotePreview.tsx
# Check for sticky notes usage in reducer
echo "Checking sticky notes reducer handling..."
rg -A 5 "case \"STICKY_NOTES_UPDATED\"" designer/client/src
Length of output: 29672
philemone
force-pushed
the
stickyNotes
branch
from
56d5da2 to
6dab7b1
Compare
There was a problem hiding this comment.
Actionable comments posted: 9
🧹 Outside diff range and nitpick comments (25)
designer/server/src/main/scala/pl/touk/nussknacker/ui/api/description/StickyNotesApiEndpoints.scala (2)
59-59: Typo in endpoint summaryIn the
stickyNotesGetEndpoint, the summary has a typo: "Returns sticky nodes" should be "Returns sticky notes".Apply this diff to correct the typo:
- .summary("Returns sticky nodes for given scenario with version") + .summary("Returns sticky notes for given scenario with version")
62-62: Consistent query parameter namingThe query parameter is named
scenarioVersionId, whereas elsewhere in the code, version IDs are referred to asversionId. For consistency and clarity, consider renaming the parameter toversionId.Apply this diff to rename the parameter:
- .in("processes" / path[ProcessName]("scenarioName") / "stickyNotes" / query[VersionId]("scenarioVersionId")) + .in("processes" / path[ProcessName]("scenarioName") / "stickyNotes" / query[VersionId]("versionId"))Remember to update any references to this parameter in related code.
designer/client/src/components/graph/ProcessGraph.tsx (1)
49-54: Consider extracting position calculation logicThe position calculation logic is duplicated between sticky notes and regular nodes. Consider extracting it to improve maintainability.
drop: (item: NodeType, monitor) => { const clientOffset = monitor.getClientOffset(); const relOffset = graph.current.processGraphPaper.clientToLocalPoint(clientOffset); - // to make node horizontally aligned - const nodeInputRelOffset = relOffset.offset(RECT_WIDTH * -0.8, RECT_HEIGHT * -0.5); + const getAlignedPosition = (offset) => mapValues( + offset.offset(RECT_WIDTH * -0.8, RECT_HEIGHT * -0.5), + Math.round + ); + const position = getAlignedPosition(relOffset); if (item?.type === StickyNoteType) { - graph.current.addStickyNote(scenario.name, scenario.processVersionId, mapValues(nodeInputRelOffset, Math.round)); + graph.current.addStickyNote(scenario.name, scenario.processVersionId, position); } else { - graph.current.addNode(monitor.getItem(), mapValues(nodeInputRelOffset, Math.round)); + graph.current.addNode(monitor.getItem(), position); setLinksHovered(graph.current.graph); } },designer/server/src/main/scala/pl/touk/nussknacker/ui/db/entity/StickyNotesEntityFactory.scala (1)
110-112: Consider adding validation in toStickyNote conversionThe
toStickyNoteconversion method directly maps fields without validation. Consider adding validation checks for critical fields before creating theStickyNoteinstance.Example improvement:
def toStickyNote: StickyNote = { + require(content.nonEmpty, "Content cannot be empty") + require(eventCreator.nonEmpty, "Event creator cannot be empty") StickyNote(id, content, layoutData, color, dimensions, targetEdge, eventCreator, eventDate.toInstant) }designer/server/src/main/scala/pl/touk/nussknacker/ui/api/description/stickynotes/Dtos.scala (2)
56-75: Consider extracting common fields into a base traitThe
StickyNoteAddRequestandStickyNoteUpdateRequestshare many fields. Consider extracting these into a base trait:trait StickyNoteBase { def content: String def layoutData: LayoutData def color: String def dimensions: Dimensions def targetEdge: Option[String] } @derive(encoder, decoder, schema) case class StickyNoteAddRequest( scenarioVersionId: VersionId, content: String, layoutData: LayoutData, color: String, dimensions: Dimensions, targetEdge: Option[String] ) extends StickyNoteBase @derive(encoder, decoder, schema) case class StickyNoteUpdateRequest( noteId: StickyNoteId, scenarioVersionId: VersionId, content: String, layoutData: LayoutData, color: String, dimensions: Dimensions, targetEdge: Option[String] ) extends StickyNoteBase
87-117: Add documentation for error casesConsider adding scaladoc to document the error hierarchy and when each error occurs:
/** Base trait for all sticky note related errors */ sealed trait StickyNotesError object StickyNotesError { /** Thrown when attempting to operate on a non-existent scenario */ final case class NoScenario(scenarioName: ProcessName) extends StickyNotesError /** Thrown when user lacks required permissions */ final case object NoPermission extends StickyNotesError with CustomAuthorizationError /** Thrown when note content exceeds maximum allowed length */ final case class StickyNoteContentTooLong(count: Int, max: Int) extends StickyNotesError /** Thrown when attempting to add notes beyond the configured limit */ final case class StickyNoteCountLimitReached(max: Int) extends StickyNotesError /** Thrown when attempting to operate on a non-existent note */ final case class NoStickyNote(noteId: StickyNoteId) extends StickyNotesErrordesigner/client/src/components/graph/EspNode/stickyNote.ts (4)
10-17: LGTM! Consider adding documentation for constraints.The constraints are well-defined with reasonable bounds. Consider adding JSDoc comments to document the purpose and impact of these constraints.
+/** + * Defines the size constraints for sticky notes. + * - MIN/MAX dimensions ensure the note remains usable and doesn't overflow + * - DEFAULT dimensions provide a comfortable starting size + */ export const STICKY_NOTE_CONSTRAINTS = {
38-48: Consider making icon positioning more responsive.The icon position is currently hardcoded using absolute values. Consider making it relative to the container size for better responsiveness.
const icon: dia.MarkupNodeJSON = { selector: "icon", tagName: "use", attributes: { opacity: 1, width: ICON_SIZE, height: ICON_SIZE, - x: ICON_SIZE / 2, - y: ICON_SIZE / 2, + x: `${CONTENT_PADDING}`, + y: `${CONTENT_PADDING}`, }, };
66-71: Enhance error handling with specific messages.The current error handling catches all errors and displays a generic message. Consider providing more specific error messages based on the error type.
try { parsed = DOMPurify.sanitize(marked.parse(stickyNote.content, { renderer }), { ADD_ATTR: ["target"] }); } catch (error) { console.error("Failed to parse markdown:", error); - parsed = "Error: Could not parse content. See error logs in console"; + parsed = `Error: ${error instanceof Error ? error.message : "Could not parse content"}`; }
97-105: Consider making shadow effect configurable.The shadow effect values are hardcoded. Consider extracting these values into constants or making them configurable through theme or props.
+const SHADOW_DEFAULTS = { + dx: 1, + dy: 1, + blur: 5, + opacity: 0.4, +} as const; + filter: { name: "dropShadow", - args: { - dx: 1, - dy: 1, - blur: 5, - opacity: 0.4, - }, + args: SHADOW_DEFAULTS, },designer/client/src/components/toolbars/creator/ToolBox.tsx (1)
125-128: Consider simplifying the groups logicThe current implementation could be made more readable by using array spread operator and extracting the filtering logic.
Consider this refactor:
const groups = useMemo(() => { - const allComponentGroups = stickyNotesSettings.enabled ? concat(componentGroups, stickyNoteToolGroup) : componentGroups; - return allComponentGroups.map(filterComponentsByLabel(filters)).filter((g) => g.components.length > 0); + const allComponentGroups = stickyNotesSettings.enabled + ? [...componentGroups, stickyNoteToolGroup] + : componentGroups; + + return allComponentGroups + .map(filterComponentsByLabel(filters)) + .filter((g) => g.components.length > 0); }, [componentGroups, filters, stickyNoteToolGroup, stickyNotesSettings]);designer/client/src/reducers/graph/utils.ts (1)
Line range hint
82-145: Consider architectural improvements for better maintainabilityThe sticky note management functions share similar patterns with node management functions. Consider:
- Creating a common interface for layout management to reduce code duplication
- Extracting shared utility functions for ID generation and validation
- Implementing consistent error handling patterns across both node and sticky note operations
Example interface:
interface LayoutManageable { id: string | number; layoutData: Position; } interface LayoutManager<T extends LayoutManageable> { add(state: GraphState, items: T[]): GraphState; remove(state: GraphState, itemId: T['id']): GraphState; prepare(state: GraphState, items: T[]): { layout: NodePosition[]; items: T[] }; }designer/server/src/test/scala/pl/touk/nussknacker/test/utils/domain/ScenarioHelper.scala (1)
85-97: Add documentation for version increment behaviorThe
updateScenariomethod would benefit from documentation explaining when and how the version is incremented, especially given theincreaseVersionWhenJsonNotChanged = trueparameter in the implementation.Add ScalaDoc comment:
+/** + * Updates the scenario and always increments the version, even when the JSON content hasn't changed. + * + * @param scenarioName name of the scenario to update + * @param newScenario new scenario content + * @return ProcessUpdated containing the update results + */ def updateScenario( scenarioName: ProcessName, newScenario: CanonicalProcess ): ProcessUpdated = {designer/server/src/main/scala/pl/touk/nussknacker/ui/server/AkkaHttpBasedRouteProvider.scala (1)
402-409: LGTM! Consider adding documentation.The initialization of
stickyNotesApiHttpServicefollows the established patterns and correctly injects all required dependencies. However, consider adding a brief comment describing the service's purpose and configuration options.Add a brief comment above the service initialization:
+// Service handling sticky notes functionality, configured via stickyNotesSettings in featureTogglesConfig val stickyNotesApiHttpService = new StickyNotesApiHttpService( authManager = authManager, stickyNotesRepository = stickyNotesRepository, scenarioService = processService, scenarioAuthorizer = processAuthorizer, dbioRunner, stickyNotesSettings = featureTogglesConfig.stickyNotesSettings )designer/client/src/components/graph/Graph.tsx (2)
234-242: Consider extracting sticky note event handlersThe event handling logic for sticky notes is scattered across multiple event handlers. Consider extracting these into dedicated methods to improve maintainability.
+private handleStickyNoteMove(cell: dia.CellView): void { + this.processGraphPaper.hideTools(); + cell.showTools(); + const updatedStickyNote = getStickyNoteCopyFromCell(this.props.stickyNotes, cell.model); + if (!updatedStickyNote) return; + const position = cell.model.get("position"); + updatedStickyNote.layoutData = { x: position.x, y: position.y }; + this.updateStickyNote(this.props.scenario.name, this.props.scenario.processVersionId, updatedStickyNote); +} .on(Events.CELL_MOVED, (cell: dia.CellView) => { if (isModelElement(cell.model)) { const linkBelowCell = this.getLinkBelowCell(); const group = batchGroupBy.startOrContinue(); this.changeLayoutIfNeeded(); this.handleInjectBetweenNodes(cell.model, linkBelowCell); batchGroupBy.end(group); } - if (isStickyNoteElement(cell.model)) { - this.processGraphPaper.hideTools(); - cell.showTools(); - const updatedStickyNote = getStickyNoteCopyFromCell(this.props.stickyNotes, cell.model); - if (!updatedStickyNote) return; - const position = cell.model.get("position"); - updatedStickyNote.layoutData = { x: position.x, y: position.y }; - this.updateStickyNote(this.props.scenario.name, this.props.scenario.processVersionId, updatedStickyNote); - } + if (isStickyNoteElement(cell.model)) { + this.handleStickyNoteMove(cell); + } })Also applies to: 372-386, 401-416
470-489: Address the TODO comment about configurationThe TODO comment suggests moving max width and height constraints to a configuration file. This would improve maintainability and make the constraints more configurable.
Would you like me to help create a configuration file for these constraints?
designer/client/src/http/HttpService.ts (3)
695-707: Consider implementing configurable default sticky note colorsThe TODO comment suggests implementing configurable default colors. This would improve customization options and maintainability.
Consider:
- Adding a configuration option for default sticky note colors
- Moving the default color to a configuration file
- Making it themeable if the application supports theming
- color: STICKY_NOTE_DEFAULT_COLOR, //TODO add config for default sticky note color? For now this is default. + color: config.stickyNotes.defaultColor,
709-738: Enhance type safety for sticky note operationsWhile the implementation is correct, we can improve type safety:
Consider creating a dedicated type for the sticky note update payload:
interface StickyNoteUpdatePayload { noteId: number; scenarioVersionId: number; content: string; layoutData: Position; color: string; dimensions: Dimensions; }Then use it in the updateStickyNote method:
- updateStickyNote(scenarioName: string, scenarioVersionId: number, stickyNote: StickyNote) { - const promise = api.put(`/processes/${encodeURIComponent(scenarioName)}/stickyNotes`, { - noteId: stickyNote.noteId, - scenarioVersionId, - content: stickyNote.content, - layoutData: stickyNote.layoutData, - color: stickyNote.color, - dimensions: stickyNote.dimensions, - }); + updateStickyNote(scenarioName: string, scenarioVersionId: number, stickyNote: StickyNote) { + const payload: StickyNoteUpdatePayload = { + noteId: stickyNote.noteId, + scenarioVersionId, + content: stickyNote.content, + layoutData: stickyNote.layoutData, + color: stickyNote.color, + dimensions: stickyNote.dimensions, + }; + const promise = api.put(`/processes/${encodeURIComponent(scenarioName)}/stickyNotes`, payload);
740-746: Improve query parameter handlingConsider using a dedicated URL search params builder for better maintainability and type safety.
- const promise = api.get(`/processes/${encodeURIComponent(scenarioName)}/stickyNotes?scenarioVersionId=${scenarioVersionId}`); + const params = new URLSearchParams({ + scenarioVersionId: scenarioVersionId.toString(), + }); + const promise = api.get(`/processes/${encodeURIComponent(scenarioName)}/stickyNotes?${params}`);docs/Changelog.md (1)
13-16: LGTM! Well documented feature addition.The changelog entry clearly describes the sticky notes feature, its purpose, API endpoint, and configuration capabilities.
Consider enhancing the documentation by adding details about the specific configuration parameters available in
stickyNotesSettings, such as:
- The default values for
maxContentLengthandmaxNotesCount- The impact of the
enabledflagdocs/MigrationGuide.md (1)
4-6: Add unit tests as indicated by TODO commentThe function implementation looks correct, but test coverage is needed to ensure reliability.
Would you like me to help generate unit tests for this function?
docs-internal/api/nu-designer-openapi.yaml (4)
2530-2643: Enhance GET endpoint with pagination and filtering capabilities.The GET endpoint returns all sticky notes without pagination or filtering options. For better scalability and performance:
- Add pagination parameters:
parameters: - name: scenarioVersionId in: query required: true schema: type: integer format: int64 + - name: page + in: query + required: false + schema: + type: integer + minimum: 0 + default: 0 + - name: size + in: query + required: false + schema: + type: integer + minimum: 1 + maximum: 100 + default: 20
- Add filtering parameters:
+ - name: editedBy + in: query + required: false + schema: + type: string + - name: editedAfter + in: query + required: false + schema: + type: string + format: date-time
2644-2742: Add rate limiting headers to PUT endpoint.The PUT endpoint should include rate limiting headers to prevent abuse:
responses: '200': description: '' + headers: + X-RateLimit-Limit: + description: Request limit per hour + schema: + type: integer + X-RateLimit-Remaining: + description: Remaining requests for the time window + schema: + type: integer + X-RateLimit-Reset: + description: Time when the rate limit resets + schema: + type: integer + format: unix-timestamp
2743-2845: Add bulk creation capability to POST endpoint.The POST endpoint only supports creating single notes. Consider adding bulk creation:
requestBody: content: application/json: schema: - $ref: '#/components/schemas/StickyNoteAddRequest' + oneOf: + - $ref: '#/components/schemas/StickyNoteAddRequest' + - type: array + items: + $ref: '#/components/schemas/StickyNoteAddRequest' + maxItems: 10
2846-2929: Add bulk deletion capability to DELETE endpoint.Similar to POST, add bulk deletion support:
parameters: - - name: noteId + - name: noteIds in: path required: true schema: - type: integer - format: int64 + type: array + items: + type: integer + format: int64 + maxItems: 10
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
⛔ Files ignored due to path filters (3)
designer/client/cypress/e2e/__image_snapshots__/electron/Linux/Creator toolbar should allow collapse (persist) and filtering #0.pngis excluded by!**/*.pngdesigner/client/package-lock.jsonis excluded by!**/package-lock.jsondesigner/server/src/main/resources/web/static/assets/components/StickyNote.svgis excluded by!**/*.svg
📒 Files selected for processing (59)
designer/client/cypress/e2e/creatorToolbar.cy.ts(1 hunks)designer/client/package.json(1 hunks)designer/client/src/actions/actionTypes.ts(1 hunks)designer/client/src/actions/nk/assignSettings.ts(1 hunks)designer/client/src/actions/nk/process.ts(3 hunks)designer/client/src/actions/notificationActions.tsx(1 hunks)designer/client/src/assets/json/nodeAttributes.json(1 hunks)designer/client/src/common/StickyNote.ts(1 hunks)designer/client/src/components/ComponentPreview.tsx(2 hunks)designer/client/src/components/StickyNotePreview.tsx(1 hunks)designer/client/src/components/graph/EspNode/stickyNote.ts(1 hunks)designer/client/src/components/graph/EspNode/stickyNoteElements.ts(1 hunks)designer/client/src/components/graph/Graph.tsx(13 hunks)designer/client/src/components/graph/GraphPartialsInTS/applyCellChanges.ts(2 hunks)designer/client/src/components/graph/GraphPartialsInTS/cellUtils.ts(2 hunks)designer/client/src/components/graph/NodeDescriptionPopover.tsx(2 hunks)designer/client/src/components/graph/ProcessGraph.tsx(4 hunks)designer/client/src/components/graph/StickyNoteElement.ts(1 hunks)designer/client/src/components/graph/fragmentGraph.tsx(1 hunks)designer/client/src/components/graph/graphStyledWrapper.ts(2 hunks)designer/client/src/components/graph/node-modal/node/FragmentContent.tsx(3 hunks)designer/client/src/components/graph/types.ts(3 hunks)designer/client/src/components/graph/utils/graphUtils.ts(1 hunks)designer/client/src/components/toolbars/creator/ComponentIcon.tsx(2 hunks)designer/client/src/components/toolbars/creator/StickyNoteComponent.tsx(1 hunks)designer/client/src/components/toolbars/creator/ToolBox.tsx(2 hunks)designer/client/src/components/toolbars/creator/ToolboxComponentGroup.tsx(1 hunks)designer/client/src/containers/theme/helpers.ts(2 hunks)designer/client/src/http/HttpService.ts(3 hunks)designer/client/src/reducers/graph/reducer.ts(2 hunks)designer/client/src/reducers/graph/types.ts(2 hunks)designer/client/src/reducers/graph/utils.ts(3 hunks)designer/client/src/reducers/selectors/graph.ts(2 hunks)designer/client/src/reducers/selectors/settings.ts(1 hunks)designer/client/src/types/component.ts(1 hunks)designer/client/src/types/node.ts(1 hunks)designer/client/src/types/stickyNote.ts(1 hunks)designer/server/src/main/resources/defaultDesignerConfig.conf(1 hunks)designer/server/src/main/scala/db/migration/V1_060__CreateStickyNotesDefinition.scala(1 hunks)designer/server/src/main/scala/db/migration/hsql/V1_060__CreateStickyNotes.scala(1 hunks)designer/server/src/main/scala/db/migration/postgres/V1_060__CreateStickyNotes.scala(1 hunks)designer/server/src/main/scala/pl/touk/nussknacker/ui/api/SettingsResources.scala(3 hunks)designer/server/src/main/scala/pl/touk/nussknacker/ui/api/StickyNotesApiHttpService.scala(1 hunks)designer/server/src/main/scala/pl/touk/nussknacker/ui/api/description/StickyNotesApiEndpoints.scala(1 hunks)designer/server/src/main/scala/pl/touk/nussknacker/ui/api/description/stickynotes/Dtos.scala(1 hunks)designer/server/src/main/scala/pl/touk/nussknacker/ui/api/description/stickynotes/StickyNoteEvent.scala(1 hunks)designer/server/src/main/scala/pl/touk/nussknacker/ui/config/FeatureTogglesConfig.scala(4 hunks)designer/server/src/main/scala/pl/touk/nussknacker/ui/db/NuTables.scala(1 hunks)designer/server/src/main/scala/pl/touk/nussknacker/ui/db/entity/StickyNotesEntityFactory.scala(1 hunks)designer/server/src/main/scala/pl/touk/nussknacker/ui/process/repository/stickynotes/DbStickyNotesRepository.scala(1 hunks)designer/server/src/main/scala/pl/touk/nussknacker/ui/process/repository/stickynotes/StickyNotesRepository.scala(1 hunks)designer/server/src/main/scala/pl/touk/nussknacker/ui/server/AkkaHttpBasedRouteProvider.scala(4 hunks)designer/server/src/test/resources/config/common-designer.conf(1 hunks)designer/server/src/test/scala/pl/touk/nussknacker/test/base/it/WithSimplifiedConfigScenarioHelper.scala(2 hunks)designer/server/src/test/scala/pl/touk/nussknacker/test/utils/domain/ScenarioHelper.scala(4 hunks)designer/server/src/test/scala/pl/touk/nussknacker/ui/api/StickyNotesApiHttpServiceBusinessSpec.scala(1 hunks)docs-internal/api/nu-designer-openapi.yaml(3 hunks)docs/Changelog.md(1 hunks)docs/MigrationGuide.md(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (42)
- designer/client/cypress/e2e/creatorToolbar.cy.ts
- designer/client/package.json
- designer/client/src/actions/actionTypes.ts
- designer/client/src/actions/nk/assignSettings.ts
- designer/client/src/actions/notificationActions.tsx
- designer/client/src/assets/json/nodeAttributes.json
- designer/client/src/common/StickyNote.ts
- designer/client/src/components/ComponentPreview.tsx
- designer/client/src/components/StickyNotePreview.tsx
- designer/client/src/components/graph/EspNode/stickyNoteElements.ts
- designer/client/src/components/graph/GraphPartialsInTS/applyCellChanges.ts
- designer/client/src/components/graph/GraphPartialsInTS/cellUtils.ts
- designer/client/src/components/graph/NodeDescriptionPopover.tsx
- designer/client/src/components/graph/StickyNoteElement.ts
- designer/client/src/components/graph/fragmentGraph.tsx
- designer/client/src/components/graph/graphStyledWrapper.ts
- designer/client/src/components/graph/node-modal/node/FragmentContent.tsx
- designer/client/src/components/graph/types.ts
- designer/client/src/components/graph/utils/graphUtils.ts
- designer/client/src/components/toolbars/creator/ComponentIcon.tsx
- designer/client/src/components/toolbars/creator/StickyNoteComponent.tsx
- designer/client/src/components/toolbars/creator/ToolboxComponentGroup.tsx
- designer/client/src/containers/theme/helpers.ts
- designer/client/src/reducers/graph/types.ts
- designer/client/src/reducers/selectors/graph.ts
- designer/client/src/reducers/selectors/settings.ts
- designer/client/src/types/component.ts
- designer/client/src/types/node.ts
- designer/client/src/types/stickyNote.ts
- designer/server/src/main/scala/db/migration/V1_060__CreateStickyNotesDefinition.scala
- designer/server/src/main/scala/db/migration/hsql/V1_060__CreateStickyNotes.scala
- designer/server/src/main/scala/db/migration/postgres/V1_060__CreateStickyNotes.scala
- designer/server/src/main/scala/pl/touk/nussknacker/ui/api/SettingsResources.scala
- designer/server/src/main/scala/pl/touk/nussknacker/ui/api/StickyNotesApiHttpService.scala
- designer/server/src/main/scala/pl/touk/nussknacker/ui/api/description/stickynotes/StickyNoteEvent.scala
- designer/server/src/main/scala/pl/touk/nussknacker/ui/config/FeatureTogglesConfig.scala
- designer/server/src/main/scala/pl/touk/nussknacker/ui/db/NuTables.scala
- designer/server/src/main/scala/pl/touk/nussknacker/ui/process/repository/stickynotes/DbStickyNotesRepository.scala
- designer/server/src/main/scala/pl/touk/nussknacker/ui/process/repository/stickynotes/StickyNotesRepository.scala
- designer/server/src/test/resources/config/common-designer.conf
- designer/server/src/test/scala/pl/touk/nussknacker/test/base/it/WithSimplifiedConfigScenarioHelper.scala
- designer/server/src/test/scala/pl/touk/nussknacker/ui/api/StickyNotesApiHttpServiceBusinessSpec.scala
🧰 Additional context used
📓 Learnings (1)
designer/client/src/components/graph/Graph.tsx (1)
Learnt from: philemone
PR: TouK/nussknacker#7149
File: designer/client/src/components/graph/Graph.tsx:530-536
Timestamp: 2024-11-20T10:37:32.855Z
Learning: There is backend validation for sticky note content length, so frontend validation in the `updateStickyNote` method in `Graph.tsx` is unnecessary.
🔇 Additional comments (40)
designer/server/src/main/scala/pl/touk/nussknacker/ui/api/description/StickyNotesApiEndpoints.scala (1)
87-119: Align noteId parameter placement with RESTful conventions
As previously suggested, consider passing the noteId as a path parameter in the stickyNotesUpdateEndpoint, similar to how it's handled in the stickyNotesDeleteEndpoint. This change enhances API consistency and adheres to RESTful design principles.
designer/client/src/components/graph/ProcessGraph.tsx (5)
6-6: LGTM: Import statements are well-organized
The new imports for sticky notes functionality are properly organized and grouped with related imports.
Also applies to: 13-24, 28-28
36-36: LGTM: State management follows React-Redux best practices
The sticky notes state is properly retrieved using the useSelector hook and the dedicated selector.
87-89: LGTM: Action creators follow Redux patterns
The sticky note action creators are properly integrated using bindActionCreators and follow the established pattern.
49-51: Verify XSS protection for sticky note content
Since sticky notes likely contain user-input content, ensure that proper sanitization is implemented in the addStickyNote method.
✅ Verification successful
XSS protection is properly implemented for sticky notes
The implementation in stickyNote.ts shows proper content sanitization using DOMPurify before rendering markdown content. The code:
- Uses DOMPurify.sanitize() on the parsed markdown
- Safely handles markdown parsing errors
- Properly configures link rendering with security attributes (target="_blank" and rel="noopener noreferrer")
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for DOMPurify usage in sticky note related files
rg -l "DOMPurify" --type ts
# Search for content sanitization in sticky note components
ast-grep --pattern 'addStickyNote($$$) {
$$$
}'
Length of output: 138
Script:
#!/bin/bash
# Check the implementation of sticky note sanitization
cat designer/client/src/components/graph/EspNode/stickyNote.ts
# Look for any other sticky note related files
rg -l "StickyNote" --type ts
# Search for sanitization in graph component
rg -A 5 "addStickyNote" --type ts
Length of output: 10499
104-104: Verify prop types in GraphWrapped component
Ensure that the stickyNotes prop is properly typed in the GraphWrapped component.
✅ Verification successful
Prop types are correctly defined
The stickyNotes prop is properly typed in both ScenarioGraphProps and FragmentGraphProps interfaces as stickyNotes: StickyNote[], which are unified in the GraphProps type used by the GraphWrapped component. The prop types are well-defined and match the usage.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for GraphWrapped component's prop types
ast-grep --pattern 'interface $PROPS {
$$$
stickyNotes: $_
$$$
}'
# Alternatively, search for type definition
rg -A 5 'type.*Props.*=.*{' designer/client/src/components/graph/GraphWrapped.tsx
Length of output: 255
Script:
#!/bin/bash
# Search for GraphWrapped component's prop types with fixed regex
rg -A 5 "type.*Props.*=" designer/client/src/components/graph/GraphWrapped.tsx
# Search for interface definitions
rg -A 10 "interface.*Props" designer/client/src/components/graph/GraphWrapped.tsx
# Get the entire file content to analyze prop types
cat designer/client/src/components/graph/GraphWrapped.tsx
Length of output: 2573
Script:
#!/bin/bash
# Let's check the GraphProps type definition from the types file
cat designer/client/src/components/graph/types.ts
# Also search for any other relevant type definitions
rg -g '*.ts' -g '*.tsx' -A 5 'interface.*GraphProps|type.*GraphProps'
Length of output: 4805
designer/server/src/main/scala/pl/touk/nussknacker/ui/db/entity/StickyNotesEntityFactory.scala (6)
1-20: LGTM! Imports are well-organized
All necessary imports are present and properly organized for database operations, JSON handling, and domain types.
31-34: Add validation constraints for user input
The existing review comment about adding validation constraints for content and color columns is still valid. Consider implementing the suggested changes.
57-62: LGTM! Well-defined foreign key relationship
The foreign key relationship with processVersionsTable is properly configured with appropriate cascade actions for both update and delete operations, ensuring referential integrity.
66-72: LGTM! Well-implemented type mappings
The type mappings for StickyNoteEvent, StickyNoteId, and StickyNoteCorrelationId are correctly implemented using MappedColumnType.base.
74-81: Enhance error handling in JSON parsing
The existing review comments about improving error handling in layoutDataColumnTyped and dimensionsColumnTyped are still valid. Consider implementing the suggested changes to improve debuggability.
Also applies to: 83-90
31-31: Verify content sanitization
The content column stores user-provided text that might be rendered in the UI. Ensure that proper XSS prevention measures are in place.
designer/server/src/main/scala/pl/touk/nussknacker/ui/api/description/stickynotes/Dtos.scala (2)
1-16: LGTM! Well-organized imports
The imports are clean, well-organized, and all are being used in the code.
77-85: Previous validation comment still applies
designer/client/src/components/graph/EspNode/stickyNote.ts (3)
59-62: LGTM! Good handling of SVG image limitations.
The image renderer properly handles the SVG limitation by providing a clear alternative representation.
128-129: LGTM! Clean factory function implementation.
The shape factory function properly combines defaults and props while maintaining type safety.
75-75: Verify if textarea should always be disabled.
The textarea is permanently disabled. If this is intentional, consider adding a comment explaining why. If not, this might prevent users from editing the content.
designer/client/src/actions/nk/process.ts (4)
9-11: LGTM: Imports are well-organized and necessary
The new imports properly support the sticky notes functionality with required types and utilities.
23-23: Add error handling for sticky notes fetch
This issue was previously identified and the comment is still valid. The sticky notes fetch should be handled gracefully if it fails.
63-70: Add TypeScript types to the helper function
The helper function parameters should be properly typed:
-const refreshStickyNotes = (dispatch, scenarioName: string, scenarioVersionId: number) => {
+const refreshStickyNotes = (
+ dispatch: ThunkDispatch<AppState, void, AnyAction>,
+ scenarioName: string,
+ scenarioVersionId: number
+) => {Additionally, the previous review comment about error handling is still valid.
72-100: Consider implementing optimistic updates
While the current implementation is functional, consider implementing optimistic updates for a better user experience. This would involve:
- Immediately updating the UI state
- Making the API call in the background
- Rolling back on failure
Example implementation for stickyNoteDeleted:
export function stickyNoteDeleted(scenarioName: string, stickyNoteId: number): ThunkAction {
return (dispatch) => {
+ // Optimistically update UI
+ dispatch({ type: "STICKY_NOTE_DELETED", stickyNoteId });
+
return HttpService.deleteStickyNote(scenarioName, stickyNoteId)
- .then(() => {
- flushSync(() => {
- dispatch({ type: "STICKY_NOTE_DELETED", stickyNoteId });
- });
- });
+ .catch((error) => {
+ // Rollback on failure
+ dispatch({ type: "STICKY_NOTE_DELETE_ERROR", error });
+ return refreshStickyNotes(dispatch, scenarioName, stickyNoteId);
+ });
};
}Additionally, the previous review comments about error handling and promise chains are still valid.
designer/client/src/components/toolbars/creator/ToolBox.tsx (2)
1-15: Export of StyledToolbox can be made private
The StyledToolbox component is only used internally within the ToolBox.tsx file and is not imported anywhere else in the codebase. The export declaration can be removed to keep the styled component scoped to its file.
118-119: Verify the scope of pristine state usage
The pristine state is only used for sticky notes, which might indicate a tight coupling. Consider whether this state should be part of the sticky notes settings instead.
Let's verify the usage of pristine state:
Also applies to: 124-124
designer/server/src/main/resources/defaultDesignerConfig.conf (1)
Line range hint 219-229: LGTM! Well-structured validation rules.
The validation rules for scenario labels are well-defined with clear patterns and descriptive error messages.
designer/client/src/reducers/graph/utils.ts (4)
8-9: LGTM: Import statements are correctly added
The new imports for StickyNote and createStickyNoteId are properly added and necessary for the sticky note functionality.
82-93: Previous review comment is still applicable
The previously suggested improvements for this function remain valid and unaddressed.
95-107: Previous review comment is still applicable
The previously suggested improvements for this function remain valid and unaddressed.
136-145: Previous review comment is still applicable
The previously suggested improvements for this function remain valid and unaddressed.
designer/server/src/test/scala/pl/touk/nussknacker/test/utils/domain/ScenarioHelper.scala (2)
13-13: LGTM: Well-organized imports
The new imports are properly organized and scoped for the sticky notes functionality.
Also applies to: 19-23, 26-26
48-48: LGTM: Consistent repository initialization
The sticky notes repository initialization follows the established pattern of other repositories in the class.
designer/client/src/reducers/graph/reducer.ts (3)
19-19: LGTM: Import statements are well-organized
The new utility function imports follow the existing naming conventions and are properly grouped with related layout utilities.
Also applies to: 23-24
244-249: Add type safety and payload validation
While the implementation follows Redux patterns well, it should validate the payload before processing to prevent runtime errors.
The previous review comment about adding payload validation is still applicable here. Consider implementing the suggested validation checks:
- Existence of
action.stickyNotes - Valid structure of sticky notes array
244-255: Verify sticky notes state handling in other cases
Ensure that sticky notes state is properly maintained in other relevant reducer cases, particularly:
- CLEAR_PROCESS
- DISPLAY_PROCESS
- UPDATE_IMPORTED_PROCESS
✅ Verification successful
Let me generate additional verification scripts to check the handling of sticky notes in the specific cases mentioned in the review comment.
Let me try a different approach to search for these cases in the reducer.
Based on the search results, I can now generate the final response since we have found how these cases handle the state:
Sticky notes state is properly handled in reducer cases
The review concern has been addressed as the sticky notes state is properly maintained:
CLEAR_PROCESS: ReturnsemptyGraphStatewhich resets all state including sticky notesDISPLAY_PROCESS: Properly initializes state with new scenario dataUPDATE_IMPORTED_PROCESS: Maintains sticky notes while updating scenario data
The implementation is consistent with the state management patterns used throughout the reducer.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Check if sticky notes are properly handled in other reducer cases
# Search for sticky notes related state handling
rg -A 5 'stickyNotes' --type ts
# Check if the initial state includes sticky notes
ast-grep --pattern 'const emptyGraphState: GraphState = {
$$$
}'
Length of output: 26770
Script:
#!/bin/bash
# Check how sticky notes are handled in CLEAR_PROCESS, DISPLAY_PROCESS, and UPDATE_IMPORTED_PROCESS cases
# Search for these specific cases in the reducer
ast-grep --pattern 'case "CLEAR_PROCESS": {
$$$
}'
ast-grep --pattern 'case "DISPLAY_PROCESS": {
$$$
}'
ast-grep --pattern 'case "UPDATE_IMPORTED_PROCESS": {
$$$
}'
# Also check the initial state handling
rg "stickyNotes.*=.*\[\]" --type ts
Length of output: 214
Script:
#!/bin/bash
# Search for the specific cases and their handling in the reducer file
# First, locate the reducer file and examine its content
rg -l "CLEAR_PROCESS|DISPLAY_PROCESS|UPDATE_IMPORTED_PROCESS" --type ts
# Then examine the content of the reducer file
rg -A 10 "case \"(CLEAR_PROCESS|DISPLAY_PROCESS|UPDATE_IMPORTED_PROCESS)\"" --type ts
# Also check if there are any other files that might handle these actions
rg "type = \"(CLEAR_PROCESS|DISPLAY_PROCESS|UPDATE_IMPORTED_PROCESS)\"" --type ts
Length of output: 4423
designer/server/src/main/scala/pl/touk/nussknacker/ui/server/AkkaHttpBasedRouteProvider.scala (1)
603-603: LGTM!
The stickyNotesApiHttpService is properly registered in the nuDesignerApi alongside other API services.
designer/client/src/components/graph/Graph.tsx (3)
21-28: LGTM: Clean imports and proper namespace extension
The imports are well-organized, and the graph namespace is properly extended to include sticky note components.
Also applies to: 49-50, 70-76
491-506: LGTM: Well-structured event handlers
The event handlers for content updates and deletion are well-implemented with proper checks and error handling.
530-553: LGTM: Operations align with backend validation
The sticky note operations are well-implemented, and correctly rely on backend validation for content length as established in PR #7149.
designer/client/src/http/HttpService.ts (1)
34-35: LGTM!
The imports are properly organized and correctly typed.
docs/MigrationGuide.md (1)
Line range hint 1-2: LGTM!
The subtraction function is implemented correctly.
docs-internal/api/nu-designer-openapi.yaml (1)
7191-7222: Add validation constraints to StickyNote schema.
The schema lacks validation for content length, color format, and dimensions:
properties:
content:
type: string
+ maxLength: 5000
+ description: Note content in markdown format
color:
type: string
+ pattern: '^#[0-9A-Fa-f]{6}$'
+ description: Hex color code (e.g. #FF0000)
dimensions:
$ref: '#/components/schemas/Dimensions'
...r/server/src/main/scala/pl/touk/nussknacker/ui/api/description/StickyNotesApiEndpoints.scala
Show resolved
Hide resolved
...r/server/src/main/scala/pl/touk/nussknacker/ui/api/description/StickyNotesApiEndpoints.scala
Show resolved
Hide resolved
designer/server/src/main/scala/pl/touk/nussknacker/ui/api/description/stickynotes/Dtos.scala
Show resolved
Hide resolved
designer/server/src/main/scala/pl/touk/nussknacker/ui/api/description/stickynotes/Dtos.scala
Show resolved
Hide resolved
designer/server/src/test/scala/pl/touk/nussknacker/test/utils/domain/ScenarioHelper.scala
Show resolved
Hide resolved
philemone
force-pushed
the
stickyNotes
branch
from
6725c09 to
fe86c9b
Compare
There was a problem hiding this comment.
Actionable comments posted: 7
🧹 Outside diff range and nitpick comments (7)
docs/MigrationGuide.md (1)
15-15: Remove empty line between sectionsThere is an unnecessary empty line between sections that should be removed for better readability.
* enabled - if set to false stickyNotes feature is disabled, stickyNotes cant be created, they are also not loaded to graph - ### Other changesdesigner/server/src/test/scala/pl/touk/nussknacker/test/utils/domain/ScenarioHelper.scala (1)
85-97: Add ScalaDoc for new public methodsConsider adding ScalaDoc comments to document these test utility methods, including:
- Purpose of the methods
- Expected parameters
- Return value semantics
- Example usage in tests
designer/server/src/main/scala/pl/touk/nussknacker/ui/process/repository/stickynotes/StickyNotesRepository.scala (1)
17-19: Add documentation for the public trait and its methods.Consider adding ScalaDoc documentation to describe the purpose of the trait and its methods, including parameter descriptions and return value explanations.
+/** + * Repository trait for managing sticky notes in scenarios. + * Provides CRUD operations for sticky notes with version control and user tracking. + */ trait StickyNotesRepository { + /** Clock instance used for timestamping operations */ def clock: Clockdesigner/server/src/main/scala/pl/touk/nussknacker/ui/api/description/stickynotes/Dtos.scala (1)
87-98: Add documentation for error types.Consider adding ScalaDoc documentation for the error hierarchy to improve API understanding.
+/** + * Base trait for all sticky note related errors. + * Used for consistent error handling across the application. + */ sealed trait StickyNotesError object StickyNotesError { + /** Indicates that the referenced scenario does not exist */ final case class NoScenario(scenarioName: ProcessName) extends StickyNotesError + /** Indicates that the user lacks necessary permissions */ final case object NoPermission extends StickyNotesError with CustomAuthorizationErrordesigner/client/src/components/graph/EspNode/stickyNote.ts (1)
10-17: Document constraints and consider making them configurable.The sticky note constraints are hardcoded. Consider making them configurable through theme or application settings.
+/** + * Constraints for sticky note dimensions. + * TODO: Consider making these configurable through application settings. + */ export const STICKY_NOTE_CONSTRAINTS = { MIN_WIDTH: 100, MAX_WIDTH: 3000,designer/client/src/components/graph/Graph.tsx (1)
234-242: Consider debouncing position updatesThe position updates are triggered directly on cell move, which could lead to excessive API calls during dragging operations.
Consider debouncing the position updates:
-if (isStickyNoteElement(cell.model)) { +if (isStickyNoteElement(cell.model)) { + const debouncedUpdate = debounce((cell) => { this.processGraphPaper.hideTools(); cell.showTools(); const updatedStickyNote = getStickyNoteCopyFromCell(this.props.stickyNotes, cell.model); if (!updatedStickyNote) return; const position = cell.model.get("position"); updatedStickyNote.layoutData = { x: position.x, y: position.y }; this.updateStickyNote(this.props.scenario.name, this.props.scenario.processVersionId, updatedStickyNote); + }, 250); + debouncedUpdate(cell); }designer/server/src/main/scala/pl/touk/nussknacker/ui/process/repository/stickynotes/DbStickyNotesRepository.scala (1)
34-48: Refactor duplicated query logic infindStickyNotesandcountStickyNotesThe methods
findStickyNotes(lines 34-48) andcountStickyNotes(lines 51-64) share similar query logic for retrieving the latest events per sticky note. Consider extracting the common parts into a private helper method to adhere to the DRY (Don't Repeat Yourself) principle and improve maintainability.Also applies to: 51-64
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
⛔ Files ignored due to path filters (3)
designer/client/cypress/e2e/__image_snapshots__/electron/Linux/Creator toolbar should allow collapse (persist) and filtering #0.pngis excluded by!**/*.pngdesigner/client/package-lock.jsonis excluded by!**/package-lock.jsondesigner/server/src/main/resources/web/static/assets/components/StickyNote.svgis excluded by!**/*.svg
📒 Files selected for processing (59)
designer/client/cypress/e2e/creatorToolbar.cy.ts(1 hunks)designer/client/package.json(1 hunks)designer/client/src/actions/actionTypes.ts(1 hunks)designer/client/src/actions/nk/assignSettings.ts(1 hunks)designer/client/src/actions/nk/process.ts(3 hunks)designer/client/src/actions/notificationActions.tsx(1 hunks)designer/client/src/assets/json/nodeAttributes.json(1 hunks)designer/client/src/common/StickyNote.ts(1 hunks)designer/client/src/components/ComponentPreview.tsx(2 hunks)designer/client/src/components/StickyNotePreview.tsx(1 hunks)designer/client/src/components/graph/EspNode/stickyNote.ts(1 hunks)designer/client/src/components/graph/EspNode/stickyNoteElements.ts(1 hunks)designer/client/src/components/graph/Graph.tsx(13 hunks)designer/client/src/components/graph/GraphPartialsInTS/applyCellChanges.ts(2 hunks)designer/client/src/components/graph/GraphPartialsInTS/cellUtils.ts(2 hunks)designer/client/src/components/graph/NodeDescriptionPopover.tsx(2 hunks)designer/client/src/components/graph/ProcessGraph.tsx(4 hunks)designer/client/src/components/graph/StickyNoteElement.ts(1 hunks)designer/client/src/components/graph/fragmentGraph.tsx(1 hunks)designer/client/src/components/graph/graphStyledWrapper.ts(2 hunks)designer/client/src/components/graph/node-modal/node/FragmentContent.tsx(3 hunks)designer/client/src/components/graph/types.ts(3 hunks)designer/client/src/components/graph/utils/graphUtils.ts(1 hunks)designer/client/src/components/toolbars/creator/ComponentIcon.tsx(2 hunks)designer/client/src/components/toolbars/creator/StickyNoteComponent.tsx(1 hunks)designer/client/src/components/toolbars/creator/ToolBox.tsx(2 hunks)designer/client/src/components/toolbars/creator/ToolboxComponentGroup.tsx(1 hunks)designer/client/src/containers/theme/helpers.ts(2 hunks)designer/client/src/http/HttpService.ts(3 hunks)designer/client/src/reducers/graph/reducer.ts(2 hunks)designer/client/src/reducers/graph/types.ts(2 hunks)designer/client/src/reducers/graph/utils.ts(3 hunks)designer/client/src/reducers/selectors/graph.ts(2 hunks)designer/client/src/reducers/selectors/settings.ts(1 hunks)designer/client/src/types/component.ts(1 hunks)designer/client/src/types/node.ts(1 hunks)designer/client/src/types/stickyNote.ts(1 hunks)designer/server/src/main/resources/defaultDesignerConfig.conf(1 hunks)designer/server/src/main/scala/db/migration/V1_060__CreateStickyNotesDefinition.scala(1 hunks)designer/server/src/main/scala/db/migration/hsql/V1_060__CreateStickyNotes.scala(1 hunks)designer/server/src/main/scala/db/migration/postgres/V1_060__CreateStickyNotes.scala(1 hunks)designer/server/src/main/scala/pl/touk/nussknacker/ui/api/SettingsResources.scala(3 hunks)designer/server/src/main/scala/pl/touk/nussknacker/ui/api/StickyNotesApiHttpService.scala(1 hunks)designer/server/src/main/scala/pl/touk/nussknacker/ui/api/description/StickyNotesApiEndpoints.scala(1 hunks)designer/server/src/main/scala/pl/touk/nussknacker/ui/api/description/stickynotes/Dtos.scala(1 hunks)designer/server/src/main/scala/pl/touk/nussknacker/ui/api/description/stickynotes/StickyNoteEvent.scala(1 hunks)designer/server/src/main/scala/pl/touk/nussknacker/ui/config/FeatureTogglesConfig.scala(4 hunks)designer/server/src/main/scala/pl/touk/nussknacker/ui/db/NuTables.scala(1 hunks)designer/server/src/main/scala/pl/touk/nussknacker/ui/db/entity/StickyNotesEntityFactory.scala(1 hunks)designer/server/src/main/scala/pl/touk/nussknacker/ui/process/repository/stickynotes/DbStickyNotesRepository.scala(1 hunks)designer/server/src/main/scala/pl/touk/nussknacker/ui/process/repository/stickynotes/StickyNotesRepository.scala(1 hunks)designer/server/src/main/scala/pl/touk/nussknacker/ui/server/AkkaHttpBasedRouteProvider.scala(4 hunks)designer/server/src/test/resources/config/common-designer.conf(1 hunks)designer/server/src/test/scala/pl/touk/nussknacker/test/base/it/WithSimplifiedConfigScenarioHelper.scala(2 hunks)designer/server/src/test/scala/pl/touk/nussknacker/test/utils/domain/ScenarioHelper.scala(4 hunks)designer/server/src/test/scala/pl/touk/nussknacker/ui/api/StickyNotesApiHttpServiceBusinessSpec.scala(1 hunks)docs-internal/api/nu-designer-openapi.yaml(3 hunks)docs/Changelog.md(1 hunks)docs/MigrationGuide.md(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (41)
- designer/client/src/types/component.ts
- designer/client/src/actions/actionTypes.ts
- designer/client/src/assets/json/nodeAttributes.json
- designer/client/src/reducers/graph/types.ts
- designer/client/src/reducers/selectors/settings.ts
- designer/client/package.json
- designer/client/src/actions/nk/assignSettings.ts
- designer/server/src/test/resources/config/common-designer.conf
- designer/client/src/types/node.ts
- designer/client/src/components/graph/NodeDescriptionPopover.tsx
- designer/client/src/containers/theme/helpers.ts
- designer/client/src/types/stickyNote.ts
- designer/client/src/actions/notificationActions.tsx
- designer/client/src/components/ComponentPreview.tsx
- designer/server/src/main/scala/db/migration/postgres/V1_060__CreateStickyNotes.scala
- designer/server/src/main/scala/pl/touk/nussknacker/ui/db/NuTables.scala
- designer/client/cypress/e2e/creatorToolbar.cy.ts
- designer/client/src/components/toolbars/creator/ToolboxComponentGroup.tsx
- designer/server/src/main/scala/pl/touk/nussknacker/ui/api/SettingsResources.scala
- designer/server/src/main/scala/db/migration/hsql/V1_060__CreateStickyNotes.scala
- designer/client/src/common/StickyNote.ts
- designer/client/src/components/graph/utils/graphUtils.ts
- designer/client/src/components/toolbars/creator/ComponentIcon.tsx
- designer/server/src/main/scala/pl/touk/nussknacker/ui/config/FeatureTogglesConfig.scala
- designer/client/src/components/graph/GraphPartialsInTS/cellUtils.ts
- designer/client/src/components/graph/graphStyledWrapper.ts
- designer/client/src/components/graph/GraphPartialsInTS/applyCellChanges.ts
- designer/client/src/components/graph/node-modal/node/FragmentContent.tsx
- designer/client/src/components/StickyNotePreview.tsx
- designer/client/src/components/toolbars/creator/StickyNoteComponent.tsx
- designer/server/src/test/scala/pl/touk/nussknacker/test/base/it/WithSimplifiedConfigScenarioHelper.scala
- designer/client/src/components/graph/types.ts
- designer/client/src/components/graph/fragmentGraph.tsx
- designer/client/src/components/graph/ProcessGraph.tsx
- designer/client/src/components/graph/StickyNoteElement.ts
- designer/server/src/test/scala/pl/touk/nussknacker/ui/api/StickyNotesApiHttpServiceBusinessSpec.scala
- designer/client/src/components/graph/EspNode/stickyNoteElements.ts
- designer/client/src/reducers/selectors/graph.ts
- designer/server/src/main/scala/pl/touk/nussknacker/ui/api/description/stickynotes/StickyNoteEvent.scala
- designer/server/src/main/scala/db/migration/V1_060__CreateStickyNotesDefinition.scala
- designer/server/src/main/scala/pl/touk/nussknacker/ui/api/StickyNotesApiHttpService.scala
👮 Files not reviewed due to content moderation or server errors (3)
- designer/client/src/actions/nk/process.ts
- designer/client/src/components/toolbars/creator/ToolBox.tsx
- designer/server/src/main/scala/pl/touk/nussknacker/ui/api/description/StickyNotesApiEndpoints.scala
🧰 Additional context used
📓 Learnings (1)
designer/client/src/components/graph/Graph.tsx (1)
Learnt from: philemone
PR: TouK/nussknacker#7149
File: designer/client/src/components/graph/Graph.tsx:530-536
Timestamp: 2024-11-20T10:37:32.855Z
Learning: There is backend validation for sticky note content length, so frontend validation in the `updateStickyNote` method in `Graph.tsx` is unnecessary.
🔇 Additional comments (32)
docs/MigrationGuide.md (2)
10-14: LGTM! Clear documentation of sticky notes configuration changes
The documentation clearly explains the new sticky notes configuration parameters and their effects:
- maxContentLength: Character limit for sticky note content
- maxNotesCount: Maximum number of sticky notes per scenario/fragment
- enabled: Global toggle for sticky notes feature
Line range hint 1-1024: Verify changelog entries for completeness
Let's verify that all significant changes from PR #7181 are properly documented in the migration guide.
✅ Verification successful
Based on the provided output, I can verify that the migration guide entry for PR #7181 is complete and accurate. The changes are properly documented in the migration guide under "Configuration changes" section:
**** The migration guide correctly documents the new stickyNotesSettings configuration with all its parameters:
maxContentLength- max length of sticky notes content (in characters)maxNotesCount- max count of sticky notes inside one scenario/fragmentenabled- flag to enable/disable sticky notes feature
The documentation matches the actual implementation as seen in:
- Default configuration in
defaultDesignerConfig.conf - Feature toggle configuration in
FeatureTogglesConfig.scala - Frontend implementation in
designer/client/src/ - API implementation in
StickyNotesApiHttpService.scala
The migration guide entry provides all the necessary information for users to understand and configure the new sticky notes feature.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Check if all major changes from PR #7181 are documented in the migration guide
# Check PR description and commits for major changes
gh pr view 7181 --json title,body,commits
# Look for any additional configuration changes in the codebase
rg -A 5 "stickyNotesSettings"
Length of output: 22855
designer/server/src/test/scala/pl/touk/nussknacker/test/utils/domain/ScenarioHelper.scala (3)
13-13: LGTM: Import changes are well-organized
The new imports are properly grouped and necessary for the sticky notes functionality.
Also applies to: 19-23, 26-26
48-48: LGTM: Repository field properly initialized
The sticky notes repository field follows the established pattern of repository initialization in the class.
222-238: Improve error handling in updateAndGetScenarioVersions
The use of .get on the Option from fetchProcessId could throw if the scenario is not found.
designer/server/src/main/scala/pl/touk/nussknacker/ui/db/entity/StickyNotesEntityFactory.scala (6)
1-20: LGTM!
The imports are well-organized and all are necessary for the implementation.
31-34: Add validation constraints and XSS prevention
- Add length constraints to prevent excessive data storage
- Add color format validation
- Consider sanitizing content to prevent XSS attacks
Additionally, consider adding server-side HTML sanitization for the content field:
import org.owasp.html.PolicyFactory
import org.owasp.html.Sanitizers
def sanitizeContent(content: String): String = {
val policy = Sanitizers.FORMATTING.and(Sanitizers.LINKS)
policy.sanitize(content)
}74-81: Enhance error handling in layoutDataColumnTyped JSON parsing
Add context to parsing errors to improve debuggability.
83-90: Enhance error handling in dimensionsColumnTyped JSON parsing
Add context to parsing errors to improve debuggability.
92-94: LGTM!
The table query definition follows Slick's best practices.
110-112: Verify intentional field dropping in toStickyNote conversion
The toStickyNote method doesn't include several fields from the entity in the resulting StickyNote:
- noteCorrelationId
- eventType
- scenarioId
- scenarioVersionId
Please verify if this is intentional. If these fields are needed for tracking or auditing purposes only, consider documenting this design decision.
✅ Verification successful
Let me gather more information about the StickyNote class definition to understand the required fields.
Let me search for the StickyNote class definition to understand its structure.
From the search results, I can now see the complete picture. The StickyNote case class in stickynotes/Dtos.scala represents the API/UI model, while StickyNoteEventEntityData is the database entity model. The dropped fields (noteCorrelationId, eventType, scenarioId, scenarioVersionId) are used for database tracking and event sourcing purposes, and they are not needed in the UI representation.
Field dropping in toStickyNote conversion is intentional
The dropped fields serve different purposes:
noteCorrelationId: Used for event correlation in the databaseeventType: Tracks the type of event in the databasescenarioIdandscenarioVersionId: Used for database relationships and versioning
These fields are infrastructure concerns that shouldn't leak into the UI model. The toStickyNote method correctly converts only the fields needed for UI representation (id, content, layoutData, color, dimensions, targetEdge, eventCreator, eventDate).
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for usages of StickyNote to verify if these fields are needed elsewhere
ast-grep --pattern 'StickyNote($$$)'
Length of output: 3807
Script:
#!/bin/bash
# Search for StickyNote class definition
ast-grep --pattern 'case class StickyNote($$$)'
# Also search for any references to the dropped fields in the codebase
rg "noteCorrelationId|eventType|scenarioId|scenarioVersionId" -A 2
Length of output: 78142
Script:
#!/bin/bash
# Search for StickyNote class/case class definition
rg "class StickyNote" -A 5
Length of output: 12010
designer/server/src/main/scala/pl/touk/nussknacker/ui/api/description/stickynotes/Dtos.scala (2)
38-42: Add validation for dimensions.
The validation suggestion from the previous review is still applicable.
77-85: Add validation and documentation for settings.
The validation suggestion from the previous review is still applicable.
designer/client/src/components/graph/EspNode/stickyNote.ts (1)
55-62: Add URL validation for security.
The URL validation suggestion from the previous review is still applicable.
designer/server/src/main/resources/defaultDesignerConfig.conf (1)
213-217: Consider improvements to the sticky notes configuration.
The configuration needs the following improvements:
- Add environment variable overrides for flexibility
- Remove trailing comma after maxContentLength
- Consider increasing maxNotesCount as 5 might be restrictive
- Add documentation comments explaining each setting
Apply this diff:
stickyNotesSettings: {
- maxContentLength: 5000,
+ # Maximum length of content in a single sticky note
+ maxContentLength: 5000
+ maxContentLength: ${?STICKY_NOTES_MAX_CONTENT_LENGTH}
+
+ # Maximum number of sticky notes allowed per scenario
maxNotesCount: 5
+ maxNotesCount: ${?STICKY_NOTES_MAX_COUNT}
+
+ # Feature flag to enable/disable sticky notes functionality
enabled: true
+ enabled: ${?STICKY_NOTES_ENABLED}
}designer/client/src/reducers/graph/utils.ts (3)
82-93: Optimize performance and add safety checks
The function has several areas for improvement:
- Unnecessary recreation of layout for all remaining sticky notes
- Missing type validation
- Potential undefined state access
Apply this optimized implementation:
export function removeStickyNoteFromLayout(state: GraphState, stickyNoteId: number): { layout: NodePosition[]; stickyNotes: StickyNote[] } {
+ if (typeof stickyNoteId !== 'number') {
+ throw new TypeError('stickyNoteId must be a number');
+ }
+
const { layout } = state;
+ const stickyNotes = state.stickyNotes || [];
const stickyNoteLayoutId = createStickyNoteId(stickyNoteId);
- const updatedStickyNotes = state.stickyNotes.filter((n) => n.noteId !== stickyNoteId);
- const updatedLayout = updatedStickyNotes.map((stickyNote) => {
- return { id: createStickyNoteId(stickyNote.noteId), position: stickyNote.layoutData };
- });
+ const updatedStickyNotes = stickyNotes.filter((n) => n.noteId !== stickyNoteId);
return {
stickyNotes: [...updatedStickyNotes],
- layout: [...layout.filter((l) => l.id !== stickyNoteLayoutId), ...updatedLayout],
+ layout: layout.filter((l) => l.id !== stickyNoteLayoutId),
};
}95-107: Add validation and handle potential duplicates
The function should validate inputs and handle potential ID conflicts:
- Validate input stickyNotes array
- Check for duplicate IDs in existing layout
- Optimize array operations
Apply this improved implementation:
export function prepareNewStickyNotesWithLayout(
state: GraphState,
stickyNotes: StickyNote[],
): { layout: NodePosition[]; stickyNotes: StickyNote[] } {
- const { layout } = state;
+ const { layout = [] } = state;
+
+ if (!Array.isArray(stickyNotes)) {
+ throw new Error("stickyNotes must be an array");
+ }
+
+ const existingIds = new Set(layout.map(l => l.id));
const updatedLayout = stickyNotes.map((stickyNote) => {
- return { id: createStickyNoteId(stickyNote.noteId), position: stickyNote.layoutData };
+ const id = createStickyNoteId(stickyNote.noteId);
+ if (existingIds.has(id)) {
+ throw new Error(`Duplicate sticky note ID: ${id}`);
+ }
+ return { id, position: stickyNote.layoutData };
});
+
return {
stickyNotes: [...stickyNotes],
layout: [...layout, ...updatedLayout],
};
}136-145: Improve state management and add validation
The function needs improvements in state management and validation:
- Merge with existing sticky notes instead of overwriting
- Add type validation
- Ensure state consistency
Apply this improved implementation:
export function addStickyNotesWithLayout(
state: GraphState,
{ stickyNotes, layout }: ReturnType<typeof prepareNewStickyNotesWithLayout>,
): GraphState {
+ if (!Array.isArray(stickyNotes) || !Array.isArray(layout)) {
+ throw new Error("Invalid input: stickyNotes and layout must be arrays");
+ }
+
+ const existingStickyNotes = state.stickyNotes || [];
+ const existingLayout = state.layout || [];
+
return {
...state,
- stickyNotes: stickyNotes,
- layout,
+ stickyNotes: [...existingStickyNotes, ...stickyNotes],
+ layout: [...existingLayout, ...layout],
};
}designer/client/src/reducers/graph/reducer.ts (2)
244-249: Add payload validation for sticky notes actions
The STICKY_NOTES_UPDATED case should validate the action payload before processing.
Apply this improvement:
case "STICKY_NOTES_UPDATED": {
+ if (!Array.isArray(action.stickyNotes)) {
+ console.warn("Invalid sticky notes payload");
+ return state;
+ }
const { stickyNotes, layout } = prepareNewStickyNotesWithLayout(state, action.stickyNotes);
return {
...addStickyNotesWithLayout(state, { stickyNotes, layout }),
};
}250-255: Validate stickyNoteId before deletion
Good code reuse with addStickyNotesWithLayout, but the stickyNoteId should be validated before processing.
Apply this improvement:
case "STICKY_NOTE_DELETED": {
+ if (!action.stickyNoteId) {
+ console.warn("Invalid sticky note ID");
+ return state;
+ }
const { stickyNotes, layout } = removeStickyNoteFromLayout(state, action.stickyNoteId);
return {
...addStickyNotesWithLayout(state, { stickyNotes, layout }),
};
}designer/client/src/components/graph/Graph.tsx (2)
21-28: LGTM: Clean integration of sticky note components
The sticky note components are properly integrated into the graph namespace with clear type definitions and constraints.
Also applies to: 49-51, 70-76
530-553: Extract duplicated permission checks
The permission check this.props.capabilities.editFrontend is duplicated across methods.
designer/client/src/http/HttpService.ts (3)
6-6: LGTM: Clean type imports
The sticky note related types are properly imported and organized.
Also applies to: 34-34
703-705: Correct the usage of i18next.t for error messages
The i18next.t usage for error messages needs to be updated to properly handle default values and interpolation.
Also applies to: 711-717, 730-736, 742-744
696-696: LGTM: Proper URL encoding
The code correctly uses encodeURIComponent for scenario names in API endpoints, preventing URL injection vulnerabilities.
Also applies to: 710-710, 722-722, 741-741
docs/Changelog.md (1)
13-16: LGTM! Well-documented changelog entry.
The changelog entry clearly describes the sticky notes feature, its purpose, API endpoint, and configuration options. This provides good documentation for users upgrading to this version.
docs-internal/api/nu-designer-openapi.yaml (2)
2530-2643: LGTM! Well-structured API endpoints with proper security.
The sticky notes API endpoints are well-designed with:
- Clear CRUD operations
- Proper authentication and authorization
- Consistent error handling
- Appropriate HTTP methods
7182-7213: 🛠️ Refactor suggestion
Add validation constraints to sticky note schemas.
The StickyNote schema should include validation constraints:
color:
type: string
+ pattern: '^#[0-9A-Fa-f]{6}$'
+ description: "Hex color code (e.g. #FF0000)"
dimensions:
width:
type: integer
format: int64
+ minimum: 50
+ maximum: 1000
height:
type: integer
format: int64
+ minimum: 50
+ maximum: 1000
content:
type: string
+ maxLength: 5000
+ description: "Note content in markdown format"Likely invalid or redundant comment.
designer/server/src/main/scala/pl/touk/nussknacker/ui/server/AkkaHttpBasedRouteProvider.scala (4)
72-72: LGTM!
The import statement follows the package naming convention and is correctly placed in the imports section.
171-171: LGTM!
The repository initialization follows the established pattern and includes all required dependencies.
603-603: LGTM!
The sticky notes API service is correctly registered in the nuDesignerApi and maintains the alphabetical ordering of services.
402-409: Verify sticky notes settings configuration
The service initialization is correct and includes all necessary dependencies. However, let's verify that the sticky notes settings are properly configured in the feature toggles.
✅ Verification successful
Sticky notes settings configuration is properly configured and used
The sticky notes settings are well-configured with:
- Default configuration in
defaultDesignerConfig.confwith maxContentLength: 5000, maxNotesCount: 5, enabled: true - Settings are properly passed through FeatureTogglesConfig and validated in StickyNotesApiHttpService
- Frontend components respect these settings, particularly the enabled flag for UI rendering
- Configuration is documented in Changelog and MigrationGuide
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Check for sticky notes settings configuration in feature toggles
# Search for sticky notes settings configuration
rg -A 5 'stickyNotesSettings'
Length of output: 12428
designer/server/src/test/scala/pl/touk/nussknacker/test/utils/domain/ScenarioHelper.scala
Show resolved
Hide resolved
...main/scala/pl/touk/nussknacker/ui/process/repository/stickynotes/StickyNotesRepository.scala
Show resolved
Hide resolved
...in/scala/pl/touk/nussknacker/ui/process/repository/stickynotes/DbStickyNotesRepository.scala
Show resolved
Hide resolved
…ation, fix error method (or did i?)
philemone
force-pushed
the
stickyNotes
branch
from
fe86c9b to
fe751f1
Compare
There was a problem hiding this comment.
Actionable comments posted: 4
🧹 Outside diff range and nitpick comments (12)
docs/Changelog.md (1)
13-16: LGTM with a minor suggestion.The changelog entry is well-structured and provides good information about the StickyNotes feature. Consider adding a brief example of the
stickyNotesSettingsconfiguration format to make it more helpful for users.Example enhancement:
* configuration `stickyNotesSettings` allowing to hide/show stickyNotes, set sticky notes max content length or its max number on a graph + Example: stickyNotesSettings: { enabled: true, maxContentLength: 500, maxNotesCount: 10 }docs/MigrationGuide.md (2)
4-6: TODO comment should be addressedThe TODO comment indicates missing tests for this function. Tests are important to verify the behavior with the new parameter z.
Would you like me to help create unit tests for this function?
6-6: Consider using version range instead of exact versionUsing exact version pinning (==) for dependencies can lead to conflicts with other packages. Consider using a version range instead.
- "requests==2.26.0", + "requests>=2.26.0,<3.0.0",designer/server/src/test/scala/pl/touk/nussknacker/ui/api/StickyNotesApiHttpServiceBusinessSpec.scala (2)
84-84: Address TODO: Add missing test coverage for CRUD operations.The current test suite only covers GET operations. Consider adding tests for:
- Creating sticky notes
- Updating sticky notes
- Deleting sticky notes
- Error cases (invalid data, permissions)
Would you like me to help generate additional test cases for these scenarios?
42-43: Consider parameterizing test data.The
stickyNoteToAddhelper method uses hardcoded values. Consider parameterizing these values to make tests more flexible and maintainable.- private def stickyNoteToAdd(versionId: VersionId): StickyNoteAddRequest = - StickyNoteAddRequest(versionId, "", LayoutData(0, 1), "#aabbcc", Dimensions(300, 200), None) + private def stickyNoteToAdd( + versionId: VersionId, + content: String = "", + layout: LayoutData = LayoutData(0, 1), + color: String = "#aabbcc", + dimensions: Dimensions = Dimensions(300, 200), + targetEdge: Option[String] = None + ): StickyNoteAddRequest = + StickyNoteAddRequest(versionId, content, layout, color, dimensions, targetEdge)designer/client/src/components/graph/ProcessGraph.tsx (2)
13-24: Group related action imports.Consider grouping related actions together for better maintainability.
import { + // Node actions injectNode, nodeAdded, nodesConnected, nodesDisconnected, + // Layout actions layoutChanged, + // Selection actions resetSelection, toggleSelection, + // Sticky note actions stickyNoteAdded, stickyNoteUpdated, stickyNoteDeleted, } from "../../actions/nk";
36-36: Consider memoizing sticky notes selector.The
getStickyNotesselector might benefit from memoization if the sticky notes array is large or frequently updated.Consider using
createSelectorfrom Redux Toolkit or Reselect:import { createSelector } from '@reduxjs/toolkit'; export const getStickyNotes = createSelector( [(state) => state.graph.stickyNotes], (stickyNotes) => stickyNotes );designer/server/src/main/scala/pl/touk/nussknacker/ui/process/repository/stickynotes/DbStickyNotesRepository.scala (1)
34-75: Consider optimizing the database queries for sticky notes retrieval.The
findStickyNotesandcountStickyNotesmethods use similar query logic with group by and joins. Consider extracting the common query parts into a private method to improve maintainability.+ private def baseNotesQuery(scenarioId: ProcessId, scenarioVersionId: VersionId) = { + stickyNotesTable + .filter(event => event.scenarioId === scenarioId && event.scenarioVersionId <= scenarioVersionId) + .groupBy(_.noteCorrelationId) + .map { case (noteCorrelationId, notes) => (noteCorrelationId, notes.map(_.eventDate).max) } + .join(stickyNotesTable) + .on { case ((noteCorrelationId, eventDate), event) => + event.noteCorrelationId === noteCorrelationId && event.eventDate === eventDate + } + .map { case ((_, _), event) => event } + } override def findStickyNotes(scenarioId: ProcessId, scenarioVersionId: VersionId): DB[Seq[StickyNote]] = { run( - stickyNotesTable - .filter(event => event.scenarioId === scenarioId && event.scenarioVersionId <= scenarioVersionId) - .groupBy(_.noteCorrelationId) - .map { case (noteCorrelationId, notes) => (noteCorrelationId, notes.map(_.eventDate).max) } - .join(stickyNotesTable) - .on { case ((noteCorrelationId, eventDate), event) => - event.noteCorrelationId === noteCorrelationId && event.eventDate === eventDate - } - .map { case ((_, _), event) => event } + baseNotesQuery(scenarioId, scenarioVersionId) .result .map(events => events.filter(_.eventType != StickyNoteEvent.StickyNoteDeleted).map(_.toStickyNote)) ) } override def countStickyNotes(scenarioId: ProcessId, scenarioVersionId: VersionId): DB[Int] = { run( - stickyNotesTable - .filter(event => event.scenarioId === scenarioId && event.scenarioVersionId <= scenarioVersionId) - .groupBy(_.noteCorrelationId) - .map { case (noteCorrelationId, notes) => (noteCorrelationId, notes.map(_.eventDate).max) } - .join(stickyNotesTable) - .on { case ((noteCorrelationId, eventDate), event) => - event.noteCorrelationId === noteCorrelationId && event.eventDate === eventDate - } - .map { case ((_, _), event) => event } + baseNotesQuery(scenarioId, scenarioVersionId) .result .map(events => events.count(_.eventType != StickyNoteEvent.StickyNoteDeleted)) ) }designer/client/src/components/graph/Graph.tsx (2)
372-379: Consider consolidating tool hiding logic.The tool hiding logic appears in multiple places. Consider extracting it to a private method to maintain DRY principles.
+private hideTools = (): void => { + this.processGraphPaper.hideTools(); +}; const showStickyNoteTools = (cellView: dia.CellView) => { cellView.showTools(); }; const hideToolsOnBlankClick = (evt: dia.Event) => { evt.preventDefault(); - this.processGraphPaper.hideTools(); + this.hideTools(); };
Line range hint
401-416: Consider extracting the event handler filter to a constant.The filter function
(view) => !isStickyNoteElement(view.model)is duplicated. Consider extracting it to a constant for better maintainability.+private readonly isNotStickyNote = (view: dia.CellView) => !isStickyNoteElement(view.model); this.processGraphPaper.on( Events.CELL_POINTERDOWN, - handleGraphEvent(handleActionOnLongPress(showNodeDetails, selectNode), null, (view) => !isStickyNoteElement(view.model)), + handleGraphEvent(handleActionOnLongPress(showNodeDetails, selectNode), null, this.isNotStickyNote), ); this.processGraphPaper.on( Events.CELL_POINTERCLICK, - handleGraphEvent(null, selectNode, (view) => !isStickyNoteElement(view.model)), + handleGraphEvent(null, selectNode, this.isNotStickyNote), );designer/server/src/main/scala/pl/touk/nussknacker/ui/api/description/StickyNotesApiEndpoints.scala (2)
59-59: Typo in API endpoint summaryThe summary in
stickyNotesGetEndpointreads "Returns sticky nodes for given scenario with version." It should be "Returns sticky notes for given scenario with version."Apply this diff to fix the typo:
- .summary("Returns sticky nodes for given scenario with version") + .summary("Returns sticky notes for given scenario with version")
145-145: Maintain consistency in summary descriptionsIn the
stickyNotesDeleteEndpoint, the summary reads "Deletes stickyNote by given noteId." For consistency and clarity, consider changing "stickyNote" to "sticky note" to match the terminology used elsewhere in the codebase.Apply this diff to fix the summary:
- .summary("Deletes stickyNote by given noteId") + .summary("Deletes sticky note by given noteId")
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
⛔ Files ignored due to path filters (3)
designer/client/cypress/e2e/__image_snapshots__/electron/Linux/Creator toolbar should allow collapse (persist) and filtering #0.pngis excluded by!**/*.pngdesigner/client/package-lock.jsonis excluded by!**/package-lock.jsondesigner/server/src/main/resources/web/static/assets/components/StickyNote.svgis excluded by!**/*.svg
📒 Files selected for processing (59)
designer/client/cypress/e2e/creatorToolbar.cy.ts(1 hunks)designer/client/package.json(1 hunks)designer/client/src/actions/actionTypes.ts(1 hunks)designer/client/src/actions/nk/assignSettings.ts(1 hunks)designer/client/src/actions/nk/process.ts(3 hunks)designer/client/src/actions/notificationActions.tsx(1 hunks)designer/client/src/assets/json/nodeAttributes.json(1 hunks)designer/client/src/common/StickyNote.ts(1 hunks)designer/client/src/components/ComponentPreview.tsx(2 hunks)designer/client/src/components/StickyNotePreview.tsx(1 hunks)designer/client/src/components/graph/EspNode/stickyNote.ts(1 hunks)designer/client/src/components/graph/EspNode/stickyNoteElements.ts(1 hunks)designer/client/src/components/graph/Graph.tsx(13 hunks)designer/client/src/components/graph/GraphPartialsInTS/applyCellChanges.ts(2 hunks)designer/client/src/components/graph/GraphPartialsInTS/cellUtils.ts(2 hunks)designer/client/src/components/graph/NodeDescriptionPopover.tsx(2 hunks)designer/client/src/components/graph/ProcessGraph.tsx(4 hunks)designer/client/src/components/graph/StickyNoteElement.ts(1 hunks)designer/client/src/components/graph/fragmentGraph.tsx(1 hunks)designer/client/src/components/graph/graphStyledWrapper.ts(2 hunks)designer/client/src/components/graph/node-modal/node/FragmentContent.tsx(3 hunks)designer/client/src/components/graph/types.ts(3 hunks)designer/client/src/components/graph/utils/graphUtils.ts(1 hunks)designer/client/src/components/toolbars/creator/ComponentIcon.tsx(2 hunks)designer/client/src/components/toolbars/creator/StickyNoteComponent.tsx(1 hunks)designer/client/src/components/toolbars/creator/ToolBox.tsx(2 hunks)designer/client/src/components/toolbars/creator/ToolboxComponentGroup.tsx(1 hunks)designer/client/src/containers/theme/helpers.ts(2 hunks)designer/client/src/http/HttpService.ts(3 hunks)designer/client/src/reducers/graph/reducer.ts(2 hunks)designer/client/src/reducers/graph/types.ts(2 hunks)designer/client/src/reducers/graph/utils.ts(3 hunks)designer/client/src/reducers/selectors/graph.ts(2 hunks)designer/client/src/reducers/selectors/settings.ts(1 hunks)designer/client/src/types/component.ts(1 hunks)designer/client/src/types/node.ts(1 hunks)designer/client/src/types/stickyNote.ts(1 hunks)designer/server/src/main/resources/defaultDesignerConfig.conf(1 hunks)designer/server/src/main/scala/db/migration/V1_060__CreateStickyNotesDefinition.scala(1 hunks)designer/server/src/main/scala/db/migration/hsql/V1_060__CreateStickyNotes.scala(1 hunks)designer/server/src/main/scala/db/migration/postgres/V1_060__CreateStickyNotes.scala(1 hunks)designer/server/src/main/scala/pl/touk/nussknacker/ui/api/SettingsResources.scala(3 hunks)designer/server/src/main/scala/pl/touk/nussknacker/ui/api/StickyNotesApiHttpService.scala(1 hunks)designer/server/src/main/scala/pl/touk/nussknacker/ui/api/description/StickyNotesApiEndpoints.scala(1 hunks)designer/server/src/main/scala/pl/touk/nussknacker/ui/api/description/stickynotes/Dtos.scala(1 hunks)designer/server/src/main/scala/pl/touk/nussknacker/ui/api/description/stickynotes/StickyNoteEvent.scala(1 hunks)designer/server/src/main/scala/pl/touk/nussknacker/ui/config/FeatureTogglesConfig.scala(4 hunks)designer/server/src/main/scala/pl/touk/nussknacker/ui/db/NuTables.scala(1 hunks)designer/server/src/main/scala/pl/touk/nussknacker/ui/db/entity/StickyNotesEntityFactory.scala(1 hunks)designer/server/src/main/scala/pl/touk/nussknacker/ui/process/repository/stickynotes/DbStickyNotesRepository.scala(1 hunks)designer/server/src/main/scala/pl/touk/nussknacker/ui/process/repository/stickynotes/StickyNotesRepository.scala(1 hunks)designer/server/src/main/scala/pl/touk/nussknacker/ui/server/AkkaHttpBasedRouteProvider.scala(4 hunks)designer/server/src/test/resources/config/common-designer.conf(1 hunks)designer/server/src/test/scala/pl/touk/nussknacker/test/base/it/WithSimplifiedConfigScenarioHelper.scala(2 hunks)designer/server/src/test/scala/pl/touk/nussknacker/test/utils/domain/ScenarioHelper.scala(4 hunks)designer/server/src/test/scala/pl/touk/nussknacker/ui/api/StickyNotesApiHttpServiceBusinessSpec.scala(1 hunks)docs-internal/api/nu-designer-openapi.yaml(3 hunks)docs/Changelog.md(1 hunks)docs/MigrationGuide.md(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (42)
- designer/client/src/assets/json/nodeAttributes.json
- designer/client/src/reducers/selectors/settings.ts
- designer/client/cypress/e2e/creatorToolbar.cy.ts
- designer/client/src/reducers/graph/types.ts
- designer/server/src/main/scala/db/migration/postgres/V1_060__CreateStickyNotes.scala
- designer/client/src/actions/actionTypes.ts
- designer/client/src/reducers/selectors/graph.ts
- designer/client/src/types/stickyNote.ts
- designer/client/src/types/component.ts
- designer/client/src/components/graph/NodeDescriptionPopover.tsx
- designer/client/src/types/node.ts
- designer/client/src/actions/nk/assignSettings.ts
- designer/server/src/test/resources/config/common-designer.conf
- designer/client/src/components/graph/node-modal/node/FragmentContent.tsx
- designer/server/src/main/scala/db/migration/hsql/V1_060__CreateStickyNotes.scala
- designer/client/src/components/ComponentPreview.tsx
- designer/client/src/actions/notificationActions.tsx
- designer/client/src/components/toolbars/creator/ToolboxComponentGroup.tsx
- designer/server/src/main/resources/defaultDesignerConfig.conf
- designer/client/src/components/toolbars/creator/StickyNoteComponent.tsx
- designer/client/src/common/StickyNote.ts
- designer/server/src/main/scala/pl/touk/nussknacker/ui/db/NuTables.scala
- designer/client/src/components/graph/fragmentGraph.tsx
- designer/client/src/containers/theme/helpers.ts
- designer/server/src/main/scala/pl/touk/nussknacker/ui/api/SettingsResources.scala
- designer/client/src/components/graph/EspNode/stickyNoteElements.ts
- designer/client/src/components/graph/GraphPartialsInTS/cellUtils.ts
- designer/client/src/components/graph/utils/graphUtils.ts
- designer/client/src/components/graph/StickyNoteElement.ts
- designer/client/src/components/toolbars/creator/ComponentIcon.tsx
- designer/client/src/components/graph/GraphPartialsInTS/applyCellChanges.ts
- designer/client/package.json
- designer/server/src/test/scala/pl/touk/nussknacker/test/base/it/WithSimplifiedConfigScenarioHelper.scala
- designer/client/src/components/toolbars/creator/ToolBox.tsx
- designer/server/src/main/scala/pl/touk/nussknacker/ui/config/FeatureTogglesConfig.scala
- designer/client/src/components/graph/types.ts
- designer/client/src/components/graph/graphStyledWrapper.ts
- designer/client/src/components/StickyNotePreview.tsx
- designer/server/src/main/scala/pl/touk/nussknacker/ui/api/description/stickynotes/StickyNoteEvent.scala
- designer/server/src/main/scala/pl/touk/nussknacker/ui/db/entity/StickyNotesEntityFactory.scala
- designer/server/src/main/scala/pl/touk/nussknacker/ui/process/repository/stickynotes/StickyNotesRepository.scala
- designer/server/src/main/scala/pl/touk/nussknacker/ui/api/StickyNotesApiHttpService.scala
👮 Files not reviewed due to content moderation or server errors (3)
- designer/server/src/main/scala/pl/touk/nussknacker/ui/api/description/stickynotes/Dtos.scala
- designer/client/src/components/graph/EspNode/stickyNote.ts
- designer/client/src/actions/nk/process.ts
🧰 Additional context used
📓 Learnings (1)
designer/client/src/components/graph/Graph.tsx (1)
Learnt from: philemone
PR: TouK/nussknacker#7149
File: designer/client/src/components/graph/Graph.tsx:530-536
Timestamp: 2024-11-20T10:37:32.855Z
Learning: There is backend validation for sticky note content length, so frontend validation in the `updateStickyNote` method in `Graph.tsx` is unnecessary.
🔇 Additional comments (25)
designer/server/src/main/scala/pl/touk/nussknacker/ui/server/AkkaHttpBasedRouteProvider.scala (2)
402-409: LGTM! Well-structured service setup.
The StickyNotesApiHttpService setup follows the established pattern and includes all necessary dependencies for proper functionality.
603-603: LGTM! Proper service registration.
The sticky notes service is correctly registered in the nuDesignerApi following the alphabetical ordering convention.
designer/server/src/test/scala/pl/touk/nussknacker/test/utils/domain/ScenarioHelper.scala (4)
13-13: LGTM: Import statements are well-organized
The new imports are properly organized and necessary for the sticky notes functionality.
Also applies to: 19-23, 26-26
48-48: LGTM: Repository field follows established patterns
The stickyNotesRepository field is properly initialized and follows the same patterns as other repository fields in the class.
85-97: LGTM: Public methods are well-designed test utilities
The new public methods follow the class's established patterns for test utilities and properly handle Future results using futureValue.
222-258: 🛠️ Refactor suggestion
Improve error handling in private implementation methods
Both methods use .get on optional scenario ID which could make test failures less clear. Consider using more explicit error handling as suggested in previous reviews.
Apply this diff to improve error handling:
private def updateAndGetScenarioVersions(
scenarioName: ProcessName,
newScenario: CanonicalProcess
): Future[ProcessUpdated] = {
for {
- scenarioId <- futureFetchingScenarioRepository.fetchProcessId(scenarioName).map(_.get)
+ scenarioIdOpt <- futureFetchingScenarioRepository.fetchProcessId(scenarioName)
+ scenarioId <- scenarioIdOpt match {
+ case Some(id) => Future.successful(id)
+ case None => Future.failed(new IllegalArgumentException(s"Test scenario not found: ${scenarioName.value}"))
+ }
action = UpdateProcessAction(
scenarioId,
newScenario,
comment = None,
labels = List.empty,
increaseVersionWhenJsonNotChanged = true,
forwardedUserName = None
)
processUpdated <- dbioRunner.runInTransaction(writeScenarioRepository.updateProcess(action))
} yield processUpdated
}
private def addStickyNoteForScenario(
scenarioName: ProcessName,
request: StickyNoteAddRequest
): Future[StickyNoteCorrelationId] = {
for {
- scenarioId <- futureFetchingScenarioRepository.fetchProcessId(scenarioName).map(_.get)
+ scenarioIdOpt <- futureFetchingScenarioRepository.fetchProcessId(scenarioName)
+ scenarioId <- scenarioIdOpt match {
+ case Some(id) => Future.successful(id)
+ case None => Future.failed(new IllegalArgumentException(s"Test scenario not found: ${scenarioName.value}"))
+ }
noteCorrelationId <- dbioRunner.runInTransaction(
stickyNotesRepository.addStickyNote(
request.content,
request.layoutData,
request.color,
request.dimensions,
request.targetEdge,
scenarioId,
request.scenarioVersionId
)
)
} yield noteCorrelationId
}This change will:
- Make test failures more descriptive when scenarios are not found
- Follow Scala's idioms for handling Options in a Future context
designer/server/src/main/scala/pl/touk/nussknacker/ui/process/repository/stickynotes/DbStickyNotesRepository.scala (4)
1-30: LGTM! Well-structured repository implementation.
The class is well-organized with clear separation of concerns and proper dependency injection through the constructor.
114-164: LGTM! Robust update implementation with proper error handling.
The update operations (updateStickyNote and deleteStickyNote) are well-implemented with:
- Proper error handling for non-existent notes
- Event-sourcing pattern for tracking changes
- Immutable state updates using
copy
168-174: LGTM! Clean companion object implementation.
The companion object provides a clean factory method for creating repository instances.
77-112: Consider adding validation for sticky note content length.
The addStickyNote method should validate the content length before persisting to prevent potential database issues with overly large content.
designer/client/src/reducers/graph/utils.ts (3)
82-93: 🛠️ Refactor suggestion
Optimize performance by avoiding unnecessary layout reconstruction.
The current implementation rebuilds the entire layout for remaining sticky notes, which is inefficient.
Apply the optimization as suggested in the previous review:
-def removeStickyNoteFromLayout(state: GraphState, stickyNoteId: number): { layout: NodePosition[]; stickyNotes: StickyNote[] } {
- const { layout } = state;
- const stickyNoteLayoutId = createStickyNoteId(stickyNoteId);
- const updatedStickyNotes = state.stickyNotes.filter((n) => n.noteId !== stickyNoteId);
- const updatedLayout = updatedStickyNotes.map((stickyNote) => {
- return { id: createStickyNoteId(stickyNote.noteId), position: stickyNote.layoutData };
- });
- return {
- stickyNotes: [...updatedStickyNotes],
- layout: [...layout.filter((l) => l.id !== stickyNoteLayoutId), ...updatedLayout],
- };
-}
+def removeStickyNoteFromLayout(state: GraphState, stickyNoteId: number): { layout: NodePosition[]; stickyNotes: StickyNote[] } {
+ if (typeof stickyNoteId !== 'number') {
+ throw new TypeError('stickyNoteId must be a number');
+ }
+
+ const { layout } = state;
+ const stickyNotes = state.stickyNotes || [];
+ const stickyNoteLayoutId = createStickyNoteId(stickyNoteId);
+ const updatedStickyNotes = stickyNotes.filter((n) => n.noteId !== stickyNoteId);
+ return {
+ stickyNotes: [...updatedStickyNotes],
+ layout: layout.filter((l) => l.id !== stickyNoteLayoutId),
+ };
+}95-107: 🛠️ Refactor suggestion
Add validation and handle potential duplicates.
The function should validate inputs and handle potential ID conflicts.
Apply the validation as suggested in the previous review:
-export function prepareNewStickyNotesWithLayout(
- state: GraphState,
- stickyNotes: StickyNote[],
-): { layout: NodePosition[]; stickyNotes: StickyNote[] } {
- const { layout } = state;
- const updatedLayout = stickyNotes.map((stickyNote) => {
- return { id: createStickyNoteId(stickyNote.noteId), position: stickyNote.layoutData };
- });
- return {
- stickyNotes: [...stickyNotes],
- layout: [...layout, ...updatedLayout],
- };
-}
+export function prepareNewStickyNotesWithLayout(
+ state: GraphState,
+ stickyNotes: StickyNote[],
+): { layout: NodePosition[]; stickyNotes: StickyNote[] } {
+ const { layout = [] } = state;
+
+ if (!Array.isArray(stickyNotes)) {
+ throw new Error("stickyNotes must be an array");
+ }
+
+ const existingIds = new Set(layout.map(l => l.id));
+ const updatedLayout = stickyNotes.map((stickyNote) => {
+ const id = createStickyNoteId(stickyNote.noteId);
+ if (existingIds.has(id)) {
+ throw new Error(`Duplicate sticky note ID: ${id}`);
+ }
+ return { id, position: stickyNote.layoutData };
+ });
+
+ return {
+ stickyNotes: [...stickyNotes],
+ layout: [...layout, ...updatedLayout],
+ };
+}136-145: 🛠️ Refactor suggestion
Improve state management and add validation.
The function needs improvements in state management and validation.
Apply the improvements as suggested in the previous review:
-export function addStickyNotesWithLayout(
- state: GraphState,
- { stickyNotes, layout }: ReturnType<typeof prepareNewStickyNotesWithLayout>,
-): GraphState {
- return {
- ...state,
- stickyNotes: stickyNotes,
- layout,
- };
-}
+export function addStickyNotesWithLayout(
+ state: GraphState,
+ { stickyNotes, layout }: ReturnType<typeof prepareNewStickyNotesWithLayout>,
+): GraphState {
+ if (!Array.isArray(stickyNotes) || !Array.isArray(layout)) {
+ throw new Error("Invalid input: stickyNotes and layout must be arrays");
+ }
+
+ const existingStickyNotes = state.stickyNotes || [];
+ const existingLayout = state.layout || [];
+
+ return {
+ ...state,
+ stickyNotes: [...existingStickyNotes, ...stickyNotes],
+ layout: [...existingLayout, ...layout],
+ };
+}designer/client/src/reducers/graph/reducer.ts (2)
244-249: 🛠️ Refactor suggestion
Add payload validation for sticky notes actions.
The action handler should validate its payload before processing.
Apply the validation as suggested in the previous review:
case "STICKY_NOTES_UPDATED": {
+ if (!Array.isArray(action.stickyNotes)) {
+ console.warn("Invalid sticky notes payload");
+ return state;
+ }
const { stickyNotes, layout } = prepareNewStickyNotesWithLayout(state, action.stickyNotes);
return {
...addStickyNotesWithLayout(state, { stickyNotes, layout }),
};
}250-255: 🛠️ Refactor suggestion
Validate stickyNoteId before deletion.
The action handler should validate the sticky note ID before processing.
Apply the validation as suggested in the previous review:
case "STICKY_NOTE_DELETED": {
+ if (!action.stickyNoteId) {
+ console.warn("Invalid sticky note ID");
+ return state;
+ }
const { stickyNotes, layout } = removeStickyNoteFromLayout(state, action.stickyNoteId);
return {
...addStickyNotesWithLayout(state, { stickyNotes, layout }),
};
}designer/client/src/components/graph/Graph.tsx (4)
Line range hint 21-77: LGTM! Clean namespace extension for sticky notes.
The namespace is properly extended to include sticky note components while maintaining the existing shapes.
234-242: LGTM! Proper handling of sticky note movement.
The code correctly:
- Hides tools during movement
- Shows tools for the moved note
- Updates position in backend
470-489: Move size constraints to configuration.
The size constraints are hardcoded in the resize handler. Consider moving them to a configuration file for better maintainability.
530-553: 🛠️ Refactor suggestion
Extract permission check to a private method.
The permission check this.props.capabilities.editFrontend is duplicated across methods. Consider extracting it to a private method.
designer/client/src/http/HttpService.ts (1)
695-707: 🛠️ Refactor suggestion
Improve error message handling.
The error messages should use i18next's options object for better localization support.
docs-internal/api/nu-designer-openapi.yaml (2)
7182-7213: 🛠️ Refactor suggestion
Add validation constraints to sticky note schemas.
Consider adding OpenAPI constraints to improve validation.
7214-7239:
Add validation to prevent XSS in note content.
The content field accepts any string without sanitization, which could lead to XSS attacks.
designer/server/src/main/scala/pl/touk/nussknacker/ui/api/description/StickyNotesApiEndpoints.scala (3)
34-34: Incorrect import path
The import statement import stickynotes.Dtos._ lacks the full package path, which may lead to import errors or ambiguity. To ensure clarity and prevent potential issues, specify the full package path.
Apply this diff to correct the import:
- import stickynotes.Dtos._
+ import pl.touk.nussknacker.ui.api.description.stickynotes.Dtos._39-39: Incorrect epoch conversion for example date
The exampleInstantDate uses Instant.ofEpochMilli(1730136602), which corresponds to a date in 1970. If the intention is to represent the date "2024-10-29T18:10:02Z", you should use Instant.ofEpochSecond(1730136602).
Apply this diff to fix the date:
- private val exampleInstantDate = Instant.ofEpochMilli(1730136602)
+ private val exampleInstantDate = Instant.ofEpochSecond(1730136602)93-107: Align noteId parameter placement with RESTful conventions
In the stickyNotesUpdateEndpoint, the noteId is currently provided in the request body as part of StickyNoteUpdateRequest. For consistency and adherence to RESTful API design principles, consider passing the noteId as a path parameter, similar to how it's handled in the stickyNotesDeleteEndpoint. This change enhances API intuitiveness and consistency.
Apply this diff to adjust the endpoint:
.put
- .in("processes" / path[ProcessName]("scenarioName") / "stickyNotes")
+ .in("processes" / path[ProcessName]("scenarioName") / "stickyNotes" / path[StickyNoteId]("noteId"))
.in(
- jsonBody[StickyNoteUpdateRequest]
+ jsonBody[StickyNoteUpdateRequestWithoutId]
.example(
- StickyNoteUpdateRequest(
- StickyNoteId(1),
+ StickyNoteUpdateRequestWithoutId(
VersionId(1),
// other fields...You'll need to create a new DTO StickyNoteUpdateRequestWithoutId that excludes noteId, as it will now be obtained from the path.
designer/server/src/main/scala/db/migration/V1_060__CreateStickyNotesDefinition.scala
Show resolved
Hide resolved
designer/server/src/main/scala/db/migration/V1_060__CreateStickyNotesDefinition.scala
Show resolved
Hide resolved
Co-authored-by: philemone <[email protected]>
Describe your changes
Checklist before merge
Summary by CodeRabbit
Release Notes
New Features
Bug Fixes
Documentation