[Draft] Sticky notes #7149
[Draft] Sticky notes #7149
Conversation
|
created: #7179 |
e30ef54 to
1fcea94
Compare
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
|
Important Review skippedDraft detected. Please check the settings in the CodeRabbit UI or the You can disable this status message by setting the WalkthroughThe changes in this pull request introduce a comprehensive sticky notes feature within the application. This includes new action types, Redux actions for managing sticky notes, and modifications to various components for rendering and interacting with sticky notes. Additionally, a new API service is implemented for CRUD operations on sticky notes, along with corresponding database migrations and repository definitions. The UI is updated to include a sticky notes panel in the toolbar, enhancing user interaction. Overall, the changes significantly expand the application's capabilities regarding sticky notes. Changes
Poem
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
There was a problem hiding this comment.
Actionable comments posted: 39
🧹 Outside diff range and nitpick comments (78)
designer/client/src/types/stickyNote.ts (1)
1-1: Consider enhancing type safety with a string literal type.To prevent typos and improve maintainability, consider defining this as a string literal type:
-export const StickyNoteType = "StickyNote"; +export const StickyNoteType = "StickyNote" as const; +export type StickyNoteType = typeof StickyNoteType;designer/server/src/main/scala/db/migration/hsql/V1_060__CreateStickyNotes.scala (1)
6-8: Consider adding migration tests.Database migrations are critical operations. Ensure there are corresponding tests to verify the migration works correctly, especially the table creation and foreign key constraints defined in the parent trait.
Would you like me to help create a test suite for this migration? This could include:
- Verification of table creation
- Foreign key constraint validation
- Data integrity checks
designer/client/src/common/StickyNote.ts (2)
1-1: Consider using path aliases for importsInstead of using relative imports, consider configuring path aliases (e.g., '@types') in your TypeScript configuration. This makes imports more maintainable and resistant to refactoring.
-import { LayoutData } from "../types"; +import { LayoutData } from "@types";
3-3: Add JSDoc documentation and consider validationThe
Dimensionstype could benefit from documentation and runtime validation to ensure positive numbers.+/** + * Represents the dimensions of a sticky note + * @property width The width of the sticky note in pixels + * @property height The height of the sticky note in pixels + */ export type Dimensions = { width: number; height: number }; +// Consider adding a validation function: +export const validateDimensions = (dimensions: Dimensions): boolean => { + return dimensions.width > 0 && dimensions.height > 0; +};designer/server/src/main/scala/pl/touk/nussknacker/ui/db/NuTables.scala (1)
14-15: Consider adding integration tests for the database layerSince this trait combines multiple database-related factories, including the new
StickyNotesEntityFactory, ensure there are integration tests covering:
- The interaction between different entity factories
- Transaction handling across multiple entity types
- CRUD operations for sticky notes alongside other entities
Would you like me to help create a template for integration tests covering these scenarios?
designer/server/src/main/scala/pl/touk/nussknacker/ui/api/description/stickynotes/StickyNoteEvent.scala (1)
9-13: Add scaladoc comments for better documentation.The event types would benefit from documentation explaining their purpose and usage context.
Consider adding documentation:
+/** Represents events that can occur on sticky notes in the system */ object StickyNoteEvent extends Enumeration { type StickyNoteEvent = Value + /** Event emitted when a new sticky note is created */ val StickyNoteCreated: Value = Value("CREATED") + /** Event emitted when an existing sticky note is modified */ val StickyNoteUpdated: Value = Value("UPDATED") + /** Event emitted when a sticky note is removed from the system */ val StickyNoteDeleted: Value = Value("DELETED") }designer/client/src/reducers/graph/types.ts (1)
17-17: Consider adding documentation for the new state property.While the implementation is correct, adding JSDoc comments would help other developers understand:
- The purpose of sticky notes in the graph context
- The relationship with other state properties
- Whether sticky notes persist across different scenarios
Consider adding documentation like this:
export type GraphState = { scenarioLoading: boolean; scenario?: Scenario; + /** Collection of sticky notes associated with the current graph. + * These notes provide additional context or documentation for specific parts of the graph. + * Notes persist with the scenario and are loaded/saved along with other graph data. + */ stickyNotes?: StickyNote[]; selectionState?: string[];designer/client/src/components/graph/GraphPartialsInTS/cellUtils.ts (1)
11-11: Remove unnecessary template literalThe string literal uses backticks but doesn't utilize any template features.
Apply this diff:
- return isModelElement(el) && el.get("type") === `stickyNote.Model`; + return isModelElement(el) && el.get("type") === "stickyNote.Model";designer/client/src/components/stickyNotes/StickyNotesPanel.tsx (1)
12-12: Memoize the noteModel objectThe
noteModelobject is recreated on every render. Consider memoizing it withuseMemoto optimize performance.- const noteModel = { id: "StickyNote", type: StickyNoteType, isDisabled: false }; + const noteModel = React.useMemo(() => ({ + id: "StickyNote", + type: StickyNoteType, + isDisabled: false + }), []);designer/client/src/assets/json/nodeAttributes.json (1)
44-46: Consider maintaining alphabetical ordering of node typesThe "StickyNote" entry is currently placed between "Properties" and "CustomNode". To maintain consistency with the rest of the file, consider moving it to maintain alphabetical ordering.
"Properties": { "name": "Properties" }, - "StickyNote": { - "name": "StickyNote" - }, "CustomNode": { "name": "CustomNode" }, "Join": { "name": "Join" }, + "StickyNote": { + "name": "StickyNote" + }, "_group": { "name": "Group", "notEditable": true }designer/client/src/actions/actionTypes.ts (1)
15-16: Consider adding CREATE action and maintaining consistent plurality.Two suggestions for improvement:
- Add a
STICKY_NOTE_CREATEDaction type for consistency with CRUD operations- Maintain consistent plurality in action names:
- Either use singular:
STICKY_NOTE_UPDATED,STICKY_NOTE_DELETED- Or use plural:
STICKY_NOTES_UPDATED,STICKY_NOTES_DELETEDexport type ActionTypes = // ... other actions ... | "STICKY_NOTES_UPDATED" | "STICKY_NOTE_DELETED" + | "STICKY_NOTE_CREATED" // ... other actions ...designer/client/src/containers/theme/helpers.ts (1)
19-26: Add JSDoc documentation for the new functionConsider adding documentation to explain:
- The purpose of the function
- Parameter expectations
- Return value structure
- Usage examples
Add this documentation above the function:
+/** + * Generates a background color for sticky notes using the theme's palette. + * @param theme - The Material-UI theme object + * @param color - A valid CSS color string (e.g., '#FF0000', 'rgb(255, 0, 0)') + * @returns An augmented color object from the theme's palette + * @example + * const color = getStickyNoteBackgroundColor(theme, '#FF0000'); + * console.log(color.main); // '#FF0000' + */ export function getStickyNoteBackgroundColor(theme: Theme, color: string) {designer/server/src/main/scala/pl/touk/nussknacker/ui/process/repository/stickynotes/StickyNotesRepository.scala (3)
3-3: Consider documenting the DB type aliasThe
DBtype fromDBIOActionInstancesappears to be a crucial part of the repository's interface. Consider adding documentation comments explaining this type alias and its implications for better maintainability.
16-19: Document clock usage and specify timezone requirementsThe
clockfield is crucial for time-based operations but lacks documentation. Consider:
- Adding ScalaDoc explaining how the clock is used
- Specifying if UTC clock should be used
- Documenting any timezone considerations
Example documentation:
/** Repository for managing sticky notes. * @note The clock should be UTC-based for consistent timestamp handling across different timezones. */ trait StickyNotesRepository { /** Clock used for generating timestamps. * Implementations should use UTC timezone for consistency. */ def clock: Clock
1-47: Add comprehensive trait documentationThe trait would benefit from comprehensive ScalaDoc documentation covering:
- Purpose and responsibilities
- Thread safety guarantees
- Transaction boundaries
- Error handling approach
- Version management strategy
Example:
/** Repository for managing sticky notes with version control support. * * This repository handles CRUD operations for sticky notes within scenarios. * All operations are transactional and thread-safe. * * @note Version management: Notes are always associated with specific scenario versions. * When a scenario is branched or copied, sticky notes are not automatically copied. * * @note Error handling: Operations return within DB monad. Implementations should handle: * - Version mismatch errors * - Concurrent modification conflicts * - Invalid input validation */designer/server/src/test/resources/config/common-designer.conf (1)
37-37: Consider adding configuration options for the sticky notes panel.The current implementation might benefit from additional configuration options such as:
- Maximum number of sticky notes allowed
- Default note size/color
- Visibility/permission settings
- Position constraints
This would provide more flexibility and control over the sticky notes feature.
Example enhancement:
- { type: "sticky-notes-panel" } + { + type: "sticky-notes-panel", + config: { + maxNotes: 50, + defaultColor: "#FFEB3B", + defaultSize: { width: 200, height: 150 }, + permissions: ["create", "edit", "delete"] + } + }designer/client/src/types/node.ts (2)
5-5: Consider improving type safetyThe union type includes a catch-all
stringwhich reduces type safety. Additionally,typeof StickyNoteTypesuggestsStickyNoteTypemight be a value/const enum rather than a type.Consider:
- Removing the
stringtype if possible to make the union more strict- If
StickyNoteTypeis a const enum or value, consider creating a proper type instead-type Type = "Properties" | "FragmentInput" | typeof StickyNoteType | string; +type Type = "Properties" | "FragmentInput" | StickyNoteType;
Line range hint
13-13: Address TODO and FIXME commentsThere are several unresolved issues in the code:
$TodoTypeplaceholder type is used in multiple places- FIXME comment indicates confusion about process and node mixing
These should be addressed to improve code quality and prevent potential issues.
Would you like me to:
- Help define proper types to replace
$TodoType?- Analyze the process/node relationship to resolve the architectural concern?
I can create GitHub issues to track these improvements if you'd like.
Also applies to: 15-15
designer/client/src/components/graph/GraphPartialsInTS/applyCellChanges.ts (1)
23-24: Consider adding error handling for sticky note processingWhile the processing logic is clean, consider adding error handling to gracefully handle potential invalid sticky notes.
- const stickyNotesModelsWithTools: ModelWithTool[] = stickyNotes.map(makeStickyNoteElement(processDefinitionData, theme)); + const stickyNotesModelsWithTools: ModelWithTool[] = stickyNotes + .filter(note => note !== null && note !== undefined) + .map(note => { + try { + return makeStickyNoteElement(processDefinitionData, theme)(note); + } catch (error) { + console.error(`Failed to process sticky note:`, error); + return null; + } + }) + .filter((model): model is ModelWithTool => model !== null);designer/server/src/test/scala/pl/touk/nussknacker/test/base/it/WithSimplifiedConfigScenarioHelper.scala (2)
44-46: Consider adding ScalaDoc for the new methodThe method implementation looks good and follows the established pattern. However, since this is a test helper trait, adding documentation would help other developers understand the method's purpose and usage in tests.
Consider adding documentation like:
+ /** + * Updates an existing scenario with a new version. + * @param scenarioName name of the scenario to update + * @param newVersion new version of the scenario + * @return details about the updated process + */ def updateScenario(scenarioName: ProcessName, newVersion: CanonicalProcess): ProcessUpdated = { rawScenarioHelper.updateScenario(scenarioName, newVersion) }
Line range hint
13-51: Good separation of concerns in test infrastructureThe additions maintain the trait's role as a simplified test helper while properly extending it for sticky note testing. The consistent delegation pattern to
rawScenarioHelperkeeps the implementation clean and maintainable.Consider creating a dedicated test suite that demonstrates common sticky note testing scenarios using these new helper methods, which would serve as both documentation and validation of the test infrastructure.
designer/client/src/components/graph/types.ts (1)
31-31: Consider extracting common props to a shared interface.The
stickyNotesproperty is duplicated in bothScenarioGraphPropsandFragmentGraphProps. Consider creating a shared interface to reduce duplication.+interface BaseGraphProps { + scenario: Scenario; + stickyNotes: StickyNote[]; + divId: string; + processCounts: ProcessCounts; + layout: Layout; +} -type ScenarioGraphProps = { +type ScenarioGraphProps = BaseGraphProps & { nodesConnected: typeof nodesConnected; // ... other action props - stickyNotes: StickyNote[]; - scenario: Scenario; - divId: string; nodeIdPrefixForFragmentTests?: string; - processCounts: ProcessCounts; capabilities: Capabilities; - layout: Layout; // ... other props }; -type FragmentGraphProps = { +type FragmentGraphProps = BaseGraphProps & { - scenario: Scenario; - stickyNotes: StickyNote[]; - divId: string; nodeIdPrefixForFragmentTests: string; - processCounts: ProcessCounts; - layout: Layout; isFragment: true; readonly: true; };Also applies to: 49-49
designer/client/src/components/StickyNotePreview.tsx (2)
11-12: Consider extracting props interface.For better reusability and maintainability, consider extracting the props interface:
+interface StickyNotePreviewProps { + node: NodeType; + isActive?: boolean; + isOver?: boolean; +} + -export function StickyNotePreview({ node, isActive, isOver }: { node: NodeType; isActive?: boolean; isOver?: boolean }): JSX.Element { +export function StickyNotePreview({ node, isActive, isOver }: StickyNotePreviewProps): JSX.Element {
1-68: Add unit tests for component states.Consider adding comprehensive unit tests to verify:
- Component rendering in different states (active/inactive, over/not over)
- Theme integration and color calculations
- Animation class applications
Would you like me to help generate a test suite for this component?
designer/client/src/components/toolbarSettings/defaultToolbarsConfig.ts (1)
74-80: Consider documenting the panel ordering rationale.The placement of
sticky-notes-panelbetweentips-panelandcreator-panelseems logical, but it would be helpful to document why this specific order was chosen to maintain consistency in future updates.Consider adding a comment above the TopLeft configuration explaining the panel ordering strategy:
+ // Panel ordering: Information panels (survey, tips) -> Content panels (sticky notes, creator) -> Activity tracking [ToolbarsSide.TopLeft]: [designer/server/src/test/scala/pl/touk/nussknacker/ui/api/StickyNotesApiHttpServiceBusinessSpec.scala (3)
42-43: Consider parameterizing the helper method.The
stickyNoteToAddhelper method uses hardcoded values for coordinates, color, and dimensions. Consider making these parameters configurable to support a wider range of test scenarios.- 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) + ): StickyNoteAddRequest = + StickyNoteAddRequest(versionId, content, layout, color, dimensions, None)
84-84: Additional test coverage needed for complete API testing.The TODO indicates more tests are needed. Based on the API functionality, consider adding test cases for:
- POST endpoint (creating sticky notes)
- PUT endpoint (updating sticky notes)
- DELETE endpoint (removing sticky notes)
- Error cases for each operation (invalid input, unauthorized access, etc.)
Would you like me to help generate these additional test cases or create a GitHub issue to track this task?
1-88: Consider implementing a comprehensive test matrix.To ensure robust testing of the Sticky Notes API, consider organizing tests into a matrix covering:
- All CRUD operations (GET, POST, PUT, DELETE)
- Authentication scenarios (authorized, unauthorized)
- Input validation cases
- Version-specific behaviors
- Concurrent access scenarios
This would provide a systematic approach to achieving comprehensive test coverage.
designer/client/src/components/graph/EspNode/stickyNote.ts (3)
43-45: Consider alternative approaches for markdown handling.The TODO comment suggests uncertainty about the implementation approach. Consider these alternatives:
- Using a dedicated markdown editor component
- Implementing a custom renderer with better integration with JointJS
- Using a lightweight markdown parser with real-time preview
Would you like me to elaborate on any of these approaches?
55-55: Consider enhancing the sticky note path for better visual appeal.The current path appears too simple for a sticky note. Consider adding a subtle curve or fold effect for better visual representation.
Example of an enhanced path:
-export const stickyNotePath = "M 0 0 L 10 0 C 10 2.6667 10 5.3333 10 8 C 10 10 9 10 8 10 L 0 10 L 0 0"; +export const stickyNotePath = "M 0 0 L 10 0 L 10.5 0.5 C 10 2.6667 10 5.3333 10 8 C 10 10 9 10 8 10 L 0 10 L 0 0 Z";
64-88: Enhance visibility and accessibility of the sticky note.Consider these improvements:
- The shadow effect might need stronger opacity for better visibility
- The typography color should be explicitly set for better contrast
Apply these changes:
filter: { name: "dropShadow", args: { dx: 1, dy: 1, blur: 5, - opacity: 0.4, + opacity: 0.6, }, }, }, foreignObject: { width: STICKY_NOTE_WIDTH - ICON_SIZE - CONTENT_PADDING * 2, height: STICKY_NOTE_HEIGHT - ICON_SIZE - CONTENT_PADDING * 2, x: ICON_SIZE + CONTENT_PADDING, y: ICON_SIZE + CONTENT_PADDING, fill: getBorderColor(theme), "pointer-events": "none", - ...theme.typography.caption, + ...theme.typography.caption, + color: theme.palette.text.primary, },designer/client/src/components/graph/ProcessGraph.tsx (1)
49-54: Consider extracting drop handling logicWhile the logic is correct, consider extracting the drop handling into a separate function to improve readability and maintainability.
Here's a suggested refactor:
- 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 handleDrop = (item: NodeType, position: any) => { + const roundedPosition = mapValues(position, Math.round); + if (item?.type === StickyNoteType) { + graph.current.addStickyNote(scenario.name, scenario.processVersionId, roundedPosition); + return; + } + graph.current.addNode(item, roundedPosition); + setLinksHovered(graph.current.graph); + }; + + handleDrop(item, nodeInputRelOffset);designer/client/src/actions/nk/process.ts (2)
9-12: Consider the performance implications of usingflushSync.While
flushSyncensures synchronous updates, it forces React to flush pending work and can impact performance. Consider if asynchronous updates would be sufficient for the sticky note operations.
72-84: Consider optimizing network requests.The update and add operations make two sequential network requests. Consider returning the updated sticky notes directly from the update/add endpoints to reduce network calls.
Also applies to: 97-109
designer/client/src/reducers/selectors/graph.ts (1)
74-74: Consider adding JSDoc documentation.The selector implementation looks good and follows the established patterns. Consider adding JSDoc documentation to maintain consistency with other selectors and improve maintainability.
+/** + * Selector to get sticky notes from the graph state. + * @param state - The root state + * @returns An array of sticky notes or an empty array if none exist + */ export const getStickyNotes = createSelector(getGraph, (g) => g.stickyNotes || ([] as StickyNote[]));designer/client/src/components/graph/NodeDescriptionPopover.tsx (1)
Line range hint
108-108: Consider addressing touch screen support with sticky notes additionThe existing TODO comment about touch screen support becomes more critical with the addition of interactive sticky notes. This would be an opportune time to implement touch screen support as part of the sticky notes feature.
Would you like me to help create a GitHub issue to track the implementation of touch screen support for node descriptions and sticky notes?
designer/server/src/main/resources/defaultDesignerConfig.conf (3)
Line range hint
91-91: LGTM! Document the authentication configuration.The OAuth2 and JWT configuration is well-structured and follows security best practices. Consider adding these environment variables to the documentation.
Would you like me to help create a documentation PR for these environment variables?
Also applies to: 92-92, 93-93, 94-94, 95-95, 96-96, 97-97, 98-98, 99-99, 100-100, 101-101, 102-108, 109-109, 110-110, 111-111, 112-112, 113-113, 114-114, 115-115, 116-116
Line range hint
236-236: Consider making the documentation URL configurable.While other tab URLs are configurable via environment variables, the documentation URL is hardcoded. Consider making it configurable for different environments or versions.
Apply this change:
- {id: "docs", url: "https://nussknacker.io/documentation/", title: "Docs", type: "Url"} + {id: "docs", url: "https://nussknacker.io/documentation/", url: ${?DOCS_URL}, title: "Docs", type: "Url"}
Line range hint
251-260: Maintain consistent syntax throughout the configuration.The
validationRulesuses=while other parts of the file use:. Consider maintaining consistency by using:throughout the file.Apply this change:
- validationRules = [ + validationRules: [designer/server/src/test/scala/pl/touk/nussknacker/test/utils/domain/ScenarioHelper.scala (2)
85-97: Add ScalaDoc for new public methodsWhile the methods are well-implemented, adding ScalaDoc would improve maintainability by documenting:
- Method purpose
- Parameter descriptions
- Return value significance
- Usage in test scenarios
Example documentation:
/** Updates an existing scenario with new content. * @param scenarioName name of the scenario to update * @param newScenario new scenario content * @return updated process details */ def updateScenario(...) /** Adds a sticky note to an existing scenario. * @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-238: Consider making UpdateProcessAction parameters configurableThe
UpdateProcessActionhas hardcoded values that might need to be configurable for different test scenarios:
comment = Nonelabels = List.emptyincreaseVersionWhenJsonNotChanged = trueConsider adding optional parameters to
updateScenarioto allow test cases to override these values.Example:
def updateScenario( scenarioName: ProcessName, newScenario: CanonicalProcess, comment: Option[Comment] = None, labels: List[String] = List.empty, increaseVersion: Boolean = true ): ProcessUpdateddesigner/client/src/reducers/graph/reducer.ts (1)
239-244: LGTM: Sticky notes update case follows Redux patternsThe implementation maintains immutability and follows similar patterns to existing node operations. However, consider grouping this case with other similar state update operations for better code organization.
Consider moving this case next to other layout-related cases like
LAYOUT_CHANGEDfor better code organization.designer/server/src/main/scala/pl/touk/nussknacker/ui/server/AkkaHttpBasedRouteProvider.scala (1)
Line range hint
397-596: Consider adding metrics for sticky notes operations.While the implementation looks solid, consider adding metrics to track sticky notes operations similar to how other services are monitored (see
initMetricsmethod).Example implementation:
// Add to initMetrics method metricsRegistry.register( MetricRegistry.name("sticky-notes", "count"), new Gauge[Int] { override def getValue: Int = { // Add method in repository to get count stickyNotesRepository.getCount() } } )designer/client/src/http/HttpService.ts (2)
690-702: Consider making sticky note defaults configurableThe method hardcodes empty content and uses a default color. Consider making these configurable through a settings object.
- addStickyNote(scenarioName: string, scenarioVersionId: number, position: Position, dimensions: Dimensions) { + addStickyNote( + scenarioName: string, + scenarioVersionId: number, + position: Position, + dimensions: Dimensions, + options: { content?: string; color?: string } = {} + ) { const promise = api.put(`/processes/${encodeURIComponent(scenarioName)}/stickyNotes`, { scenarioVersionId, - content: "", + content: options.content ?? "", layoutData: position, - color: STICKY_NOTE_DEFAULT_COLOR, //TODO add config for default sticky note color? For now this is default. + color: options.color ?? STICKY_NOTE_DEFAULT_COLOR, dimensions: dimensions, });
716-733: Consider using PUT/PATCH for update operationsThe method uses POST for an update operation. Consider using PUT or PATCH to better align with REST conventions. Also, consider making the error message more specific by including the note ID.
- updateStickyNote(scenarioName: string, scenarioVersionId: number, stickyNote: StickyNote) { - const promise = api.post(`/processes/${encodeURIComponent(scenarioName)}/stickyNotes`, { + updateStickyNote(scenarioName: string, scenarioVersionId: number, stickyNote: StickyNote) { + const promise = api.put(`/processes/${encodeURIComponent(scenarioName)}/stickyNotes/${stickyNote.noteId}`, { noteId: stickyNote.noteId, scenarioVersionId, content: stickyNote.content, layoutData: stickyNote.layoutData, color: stickyNote.color, dimensions: stickyNote.dimensions, }); promise.catch((error) => this.#addError( - i18next.t("notification.error.failedToUpdateStickyNote", "Failed to update sticky note for scenario"), + i18next.t("notification.error.failedToUpdateStickyNote", `Failed to update sticky note ${stickyNote.noteId} for scenario`), error, true, ), );docs/configuration/DesignerConfiguration.md (1)
593-593: Documentation needed for the sticky notes panel.While the addition of the
sticky-notes-panelis aligned with the PR objectives, the documentation should include details about:
- Purpose and functionality of the sticky notes panel
- Available configuration options (if any)
- Example usage scenarios
Consider adding a section under "Process Toolbar Configuration" that describes the sticky notes panel, similar to how other panels are documented. Example addition:
### Process Toolbar Configuration +#### Sticky Notes Panel + +The sticky notes panel (`type: "sticky-notes-panel"`) allows users to add, edit, and manage sticky notes within the process window. These notes can be used to: +- Add temporary reminders or annotations +- Highlight important aspects of the process +- Collaborate with team members through visual annotations + +Example configuration: +```hocon +processToolbarConfig { + defaultConfig { + topLeft: [ + { type: "sticky-notes-panel" } + ] + } +} +```docs-internal/api/nu-designer-openapi.yaml (3)
2530-2643: Consider making scenarioVersionId optional in GET endpointThe GET endpoint requires
scenarioVersionIdas a mandatory query parameter. Consider making it optional to allow fetching sticky notes for the latest version by default.parameters: - name: scenarioVersionId in: query - required: true + required: false schema: type: integer format: int64
2644-2739: Add more specific error responses for PUT endpointThe PUT endpoint could benefit from more specific error responses for business validation cases like:
- Invalid dimensions (negative values)
- Invalid color format
- Content length/format validation
Add these response codes:
responses: + '422': + description: 'Business validation failed' + content: + application/json: + schema: + type: object + properties: + code: + type: string + enum: [INVALID_DIMENSIONS, INVALID_COLOR, INVALID_CONTENT] + message: + type: string
Line range hint
7489-7501: Add validation constraints to Dimensions schemaThe Dimensions schema should include min/max values to prevent invalid dimensions.
width: type: integer format: int64 + minimum: 50 + maximum: 1000 + description: Width in pixels (50-1000) height: type: integer format: int64 + minimum: 50 + maximum: 1000 + description: Height in pixels (50-1000)designer/server/src/main/scala/db/migration/V1_060__CreateStickyNotesDefinition.scala (2)
12-12: Eliminate unused import of ExecutionContextThe import
scala.concurrent.ExecutionContext.Implicits.globalis not used in this file. Removing unused imports enhances code readability and maintainability.
25-25: Define foreign key constraint using Slick instead of raw SQLUsing Slick's schema definition to add foreign key constraints can improve code clarity and portability across different database systems. Consider defining the foreign key constraint within the Slick table definition rather than executing raw SQL.
Here's how you might define the foreign key constraint in Slick:
def processVersions = TableQuery[ProcessVersionsEntity] // Ensure this TableQuery is available def scenarioVersionFK = foreignKey( "sticky_notes_scenario_version_fk", (scenarioId, scenarioVersionId), processVersions )( r => (r.processId, r.id), onDelete = ForeignKeyAction.Cascade )Make sure to import or define the
ProcessVersionsEntityto establish the relationship.designer/server/src/main/scala/pl/touk/nussknacker/ui/api/description/stickynotes/Dtos.scala (2)
78-78: Consider renamingcellErrorSchematolayoutDataSchemafor clarityThe variable
cellErrorSchemaat line 78 defines aSchema[LayoutData]. Renaming it tolayoutDataSchemawould improve readability and accurately reflect the associated type, thereby reducing potential confusion.
95-98: RenamenoCommentCodectostickyNoteContentTooLongCodecfor consistencyThe implicit codec
noCommentCodecis associated with theStickyNoteContentTooLongerror. Renaming it tostickyNoteContentTooLongCodecwould enhance consistency with the error class and improve code clarity.designer/server/src/main/scala/pl/touk/nussknacker/ui/db/entity/StickyNotesEntityFactory.scala (1)
29-40: Consider adding database constraints for data integrityAdding constraints such as
UNIQUE,DEFAULT, or additionalNOT NULLclauses can enhance data integrity at the database level.For example, if
noteCorrelationIdshould be unique per scenario, you might add a unique constraint:def noteCorrelationId = column[StickyNoteCorrelationId]("note_correlation_id", NotNull, O.Unique)Please adjust according to your specific requirements.
designer/client/src/components/graph/EspNode/stickyNoteElements.ts (3)
21-48: Optimize repeated function calls by caching computed valuesMultiple calls to
getStickyNoteBackgroundColor(theme, stickyNote.color)andtheme.palette.getContrastText(...)can be optimized by caching their results. This will enhance performance and improve code readability.Apply this diff to cache the computed values:
+ const backgroundColor = getStickyNoteBackgroundColor(theme, stickyNote.color); + const backgroundColorMain = backgroundColor.main; + const backgroundColorDark = backgroundColor.dark; + const backgroundColorLight = backgroundColor.light; + const contrastTextColor = theme.palette.getContrastText(backgroundColorMain); const attributes: shapes.devs.ModelAttributes = { id: createStickyNoteId(stickyNote.noteId), noteId: stickyNote.noteId, attrs: { size: { width: stickyNote.dimensions.width, height: stickyNote.dimensions.height, }, body: { - fill: getStickyNoteBackgroundColor(theme, stickyNote.color).main, + fill: backgroundColorMain, opacity: 1, }, foreignObject: { width: stickyNote.dimensions.width - ICON_SIZE - CONTENT_PADDING * 2, height: stickyNote.dimensions.height - ICON_SIZE - CONTENT_PADDING * 2, - color: theme.palette.getContrastText(getStickyNoteBackgroundColor(theme, stickyNote.color).main), + color: contrastTextColor, }, icon: { xlinkHref: iconHref, opacity: 1, - color: theme.palette.getContrastText(getStickyNoteBackgroundColor(theme, stickyNote.color).main), + color: contrastTextColor, }, border: { - stroke: getStickyNoteBackgroundColor(theme, stickyNote.color).dark, + stroke: backgroundColorDark, strokeWidth: 1, }, }, rankDir: "R", };
104-104: Use predefined constants for minimum dimensions in 'setPosition'In the
setPositionmethod, the minimum width and height are hardcoded as100. For consistency and ease of maintenance, consider using theMIN_STICKY_NOTE_WIDTHandMIN_STICKY_NOTE_HEIGHTconstants defined earlier.Apply this diff to use the constants:
setPosition: function (view, coordinates) { const model = view.model; - model.resize(Math.max(coordinates.x - 10, 100), Math.max(coordinates.y - 10, 100)); + model.resize( + Math.max(coordinates.x - 10, MIN_STICKY_NOTE_WIDTH), + Math.max(coordinates.y - 10, MIN_STICKY_NOTE_HEIGHT) + ); },
127-129: Define constants for resize offsets to enhance readabilityThe offsets of
+10and-10used in themodel.resizecall are magic numbers. Defining them as constants will improve code readability and make future adjustments easier.Apply this diff to define offset constants:
+ const WIDTH_OFFSET = 10; + const HEIGHT_OFFSET = 10; model.resize( - Math.max(stickyNote.dimensions.width + 10, MIN_STICKY_NOTE_WIDTH), - Math.max(stickyNote.dimensions.height - 10, MIN_STICKY_NOTE_HEIGHT), + Math.max(stickyNote.dimensions.width + WIDTH_OFFSET, MIN_STICKY_NOTE_WIDTH), + Math.max(stickyNote.dimensions.height - HEIGHT_OFFSET, MIN_STICKY_NOTE_HEIGHT), );designer/server/src/main/scala/pl/touk/nussknacker/ui/process/repository/stickynotes/DbStickyNotesRepository.scala (5)
37-47: Consider optimizing thefindStickyNotesquery for performanceThe query in the
findStickyNotesmethod may become inefficient with large datasets due to grouping and joining operations. Consider optimizing the query or adding appropriate database indexes onnoteCorrelationId,eventDate, andscenarioVersionIdto improve performance.
77-83: Enhance exception messages for better debuggingThe exception messages in the
addStickyNotemethod could be more informative. Including additional context such asscenarioId,scenarioVersionId, ornoteCorrelationIdcan aid in debugging and provide clearer insight into the failure.
90-96: Refine error handling when sticky note is not foundInstead of throwing a generic
NoSuchElementException, consider throwing a custom exception or returning a meaningful error message when a sticky note is not found during an update. This enhances clarity for the caller.
50-85: Reduce code duplication by abstracting event creation logicBoth
addStickyNoteandupdateStickyNoteinvolve creatingStickyNoteEventEntityDatainstances and inserting them into thestickyNotesTable. Extracting this logic into a helper method can improve maintainability and adhere to the DRY principle.
103-127: Maintain consistent parameter ordering between methodsFor better readability and to reduce potential errors, consider aligning the parameter order in
updateStickyNotewithaddStickyNote. Consistent method signatures enhance developer experience.designer/server/src/main/scala/pl/touk/nussknacker/ui/api/description/StickyNotesApiEndpoints.scala (8)
50-50: Typo in Endpoint SummaryThe summary for
stickyNotesGetEndpointcontains a typo: "Returns sticky nodes" should be "Returns sticky notes".
28-28: Unused Encoder VariableThe
encodervariable is defined but not used in the code. Consider removing it if it's unnecessary to keep the code clean.
84-84: IncludeStickyNoteIdin Update Endpoint PathThe update endpoint does not include the
StickyNoteIdin the URL path, which is essential for identifying the specific sticky note to update. Including thenoteIdin the path makes the API more RESTful and clearly specifies the resource being modified.Apply this diff to include the
noteIdin the path:- .in("processes" / path[ProcessName]("scenarioName") / "stickyNotes") + .in("processes" / path[ProcessName]("scenarioName") / "stickyNotes" / path[StickyNoteId]("noteId"))And update the input parameters accordingly.
86-96: Adjust Input Model for Update EndpointWith
StickyNoteIdincluded in the path, it's redundant to have it in theStickyNoteUpdateRequest. Consider removingnoteIdfrom the request body to avoid duplication.Apply this diff to adjust the request model:
- jsonBody[StickyNoteUpdateRequest] + jsonBody[StickyNoteUpdateRequestWithoutId]And modify the
StickyNoteUpdateRequestto excludenoteId.
158-159: UsejsonBodyInstead ofplainBodyfor Error ResponsesThe error response for
NoScenariois defined withplainBody, which may not serialize the error object correctly. UsingjsonBodyensures proper JSON serialization and aligns with the API's response format.Apply this diff:
- plainBody[NoScenario] + jsonBody[NoScenario]
170-171: UsejsonBodyInstead ofplainBodyfor Error ResponsesSimilarly, the error response for
NoStickyNoteshould usejsonBodyto correctly serialize the error object.Apply this diff:
- plainBody[NoStickyNote] + jsonBody[NoStickyNote]
65-65: Complete TODO for Error ExamplesThere's a TODO comment to add error examples in the
stickyNotesGetEndpoint. Including error examples enhances API documentation and helps clients understand possible error responses.Would you like assistance in adding error examples to the endpoint documentation?
30-30: Example Date Possibly IncorrectThe
exampleInstantDateis set usingInstant.ofEpochMilli(1730136602), which corresponds to a date in the 1970s. Consider updating it to a more recent date for clarity in examples.Suggestion:
- private val exampleInstantDate = Instant.ofEpochMilli(1730136602) + private val exampleInstantDate = Instant.now()designer/server/src/main/scala/pl/touk/nussknacker/ui/api/StickyNotesApiHttpService.scala (1)
39-93: Refactor to reduce code duplication in endpoint definitionsThe endpoint definitions for
stickyNotesGetEndpoint,stickyNotesAddEndpoint,stickyNotesUpdateEndpoint, andstickyNotesDeleteEndpointshare similar structures. Consider refactoring to abstract common logic, enhancing maintainability and reducing redundancy.designer/client/src/components/graph/Graph.tsx (7)
220-220: Use strict equality===instead of==for note ID comparisonWhen comparing
noteIdvalues, it's safer to use strict equality===to avoid unexpected type coercion issues.Apply this diff to make the change:
-const stickyNote = this.props.stickyNotes.find((a) => a.noteId == noteId); +const stickyNote = this.props.stickyNotes.find((a) => a.noteId === noteId);Also applies to: 446-446
354-354: Implement logic to hide sticky note toolsThere's a TODO comment indicating a need to hide the sticky note tools upon certain actions. Please implement the necessary logic to ensure the tools are hidden appropriately.
Would you like assistance in implementing the functionality to hide the sticky note tools when they're no longer needed?
360-363: Consider consistent handling of sticky note selectionIn the
selectNodefunction, when a sticky note is selected, the function shows the sticky note tools and returns early. Ensure that this behavior aligns with the expected user experience and that it doesn't interfere with other selection functionalities.If appropriate, consider whether additional actions (e.g., updating selection state) should occur when a sticky note is selected.
446-446: Use strict equality===for note ID comparisonConsistent with best practices, use
===instead of==when comparingnoteIdvalues to avoid type coercion issues.Apply this diff:
-const stickyNote = this.props.stickyNotes.find((a) => a.noteId == noteId); +const stickyNote = this.props.stickyNotes.find((a) => a.noteId === noteId);
484-491: Consolidate permission checks to improve code maintainabilityThe methods
addStickyNote,updateStickyNote, anddeleteStickyNoteall checkthis.props.capabilities.editFrontendbefore proceeding. Consider extracting this permission check into a separate method to reduce code duplication and improve readability.For example:
private canEditStickyNotes(): boolean { return this.props.capabilities.editFrontend && !this.props.isFragment; }Then use it in the methods:
-addStickyNote(...) { - if (this.props.isFragment === true) return; - const canAddStickyNote = this.props.capabilities.editFrontend; - if (canAddStickyNote) { +addStickyNote(...) { + if (this.canEditStickyNotes()) { // ... } }Also applies to: 493-500
501-506: Rename variablecanUpdateStickyNotetocanDeleteStickyNotefor clarityIn the
deleteStickyNotemethod, the variablecanUpdateStickyNoteshould be renamed tocanDeleteStickyNoteto reflect its purpose accurately.Apply this diff:
deleteStickyNote(scenarioName: string, stickyNoteId: number): void { if (this.props.isFragment === true) return; - const canUpdateStickyNote = this.props.capabilities.editFrontend; - if (canUpdateStickyNote) { + const canDeleteStickyNote = this.props.capabilities.editFrontend; + if (canDeleteStickyNote) { this.props.stickyNoteDeleted(scenarioName, stickyNoteId); } }
354-354: Remove or address the TODO comment regarding hiding toolsThe TODO comment suggests hiding the tools "on some other action." Leaving TODO comments can lead to technical debt. Please address the comment by implementing the necessary functionality or provide a clear plan for future implementation.
If the implementation is planned for later, consider creating a tracking issue to manage this task.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
⛔ Files ignored due to path filters (1)
designer/server/src/main/resources/web/static/assets/components/StickyNote.svgis excluded by!**/*.svg
📒 Files selected for processing (50)
designer/client/src/actions/actionTypes.ts(1 hunks)designer/client/src/actions/nk/process.ts(3 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(9 hunks)designer/client/src/components/graph/GraphPartialsInTS/applyCellChanges.ts(2 hunks)designer/client/src/components/graph/GraphPartialsInTS/cellUtils.ts(1 hunks)designer/client/src/components/graph/NodeDescriptionPopover.tsx(2 hunks)designer/client/src/components/graph/ProcessGraph.tsx(4 hunks)designer/client/src/components/graph/fragmentGraph.tsx(1 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/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(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/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/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(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/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 comments (77)
designer/server/src/main/scala/db/migration/hsql/V1_060__CreateStickyNotes.scala (1)
1-8: Implementation looks good, verify version sequence.
The implementation correctly extends the base definition and properly configures HSQLDB profile with lazy initialization. The code structure follows good practices for database migrations.
Let's verify the migration version sequence:
✅ Verification successful
Version sequence verified - implementation is correct
The migration version V1_060 follows the correct sequence:
- Previous version was V1_058 (scenario activities)
- No duplicate versions exist
- Implementation present for both HSQLDB and Postgres
- Base definition and specific implementations are properly aligned
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Check if V1_060 follows the correct version sequence
# Expected: No gaps in version numbers and no duplicate versions
# Search for all migration files
echo "Checking migration sequence..."
fd -e scala "V1_.*__.*\.scala" designer/server/src/main/scala/db/migration | sort
Length of output: 4455
designer/server/src/main/scala/db/migration/postgres/V1_060__CreateStickyNotes.scala (2)
1-8: LGTM! Clean and well-structured implementation.
The implementation follows best practices with:
- Clear separation of concerns between base definition and PostgreSQL-specific implementation
- Proper use of lazy initialization for the profile
- Correct package structure for database-specific migrations
1-1: Verify migration version sequence.
The migration version V1_060 should be verified to ensure it follows the correct sequence of existing migrations.
✅ Verification successful
Migration version V1_060 follows the correct sequence
The migration version V1_060 is correctly placed as the latest migration file in the sequence, following V1_058. While there are some gaps in the version numbers throughout the sequence (e.g., between V1_032 and V1_039), this is common in migration versioning and doesn't indicate an issue. The important aspect is that V1_060 is properly placed after the previous migration V1_058.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Check if the migration version is in sequence with existing migrations
# Expected: V1_060 should be the next version after the latest existing migration
# List all migration files and verify the sequence
fd -e scala "V1_.*__.*\.scala$" designer/server/src/main/scala/db/migration/postgres | sort -V
# Check if there are any gaps in version numbers
echo "Checking for version gaps..."
prev_version=0
fd -e scala "V1_.*__.*\.scala$" designer/server/src/main/scala/db/migration/postgres | sort -V | while read -r file; do
current_version=$(echo "$file" | grep -o 'V1_[0-9]\+' | grep -o '[0-9]\+')
if [ $((current_version - prev_version)) -gt 1 ]; then
echo "Gap found between V1_$(printf "%03d" $prev_version) and V1_$(printf "%03d" $current_version)"
fi
prev_version=$current_version
done
Length of output: 1882
Script:
#!/bin/bash
# Let's try a different approach to check the migration sequence
# List all migration files and extract version numbers for comparison
echo "Migration files in sequence:"
fd -e scala "V1_.*__.*\.scala$" designer/server/src/main/scala/db/migration/postgres | sort -V
echo -e "\nChecking version sequence..."
# Extract and compare version numbers using awk for better number handling
fd -e scala "V1_.*__.*\.scala$" designer/server/src/main/scala/db/migration/postgres | sort -V | awk -F'[/_]' '
BEGIN { prev=0 }
{
current=substr($4, 1, length($4)-2)
if (prev > 0 && current - prev > 1) {
printf "Gap detected between V1_%03d and V1_%03d\n", prev, current
}
prev=current
}
'
Length of output: 1868
designer/client/src/common/StickyNote.ts (1)
5-15: Verify type consistency across the application
Let's ensure this interface is consistently used across the application.
✅ Verification successful
Let me gather more specific information about the TypeScript usage since the previous command had an issue.
Let me try one more search to find UI component usage with a different approach:
Based on the comprehensive search results, I can now provide a final response:
StickyNote interface is consistently implemented across the application
The type is properly integrated throughout the application:
- Correctly used in Redux state management (
reducers/graph/types.ts,reducers/selectors/graph.ts) - Properly handled in HTTP services (
http/HttpService.ts) - Consistently implemented in UI components (
components/graph/*) - Matches the backend schema (Scala case classes in
designer/server/src/main/scala/pl/touk/nussknacker/ui/api/description/stickynotes/Dtos.scala)
All properties defined in the interface are used correctly across the codebase, with proper type safety and consistent data flow from backend to frontend.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for StickyNote type usage
echo "Checking StickyNote type usage in components:"
rg -t typescript "StickyNote" --glob "!{designer/client/src/common/StickyNote.ts}"
echo "\nChecking for potential type mismatches in Redux actions:"
ast-grep --pattern 'interface $action {
type: $_;
payload: StickyNote;
}'
echo "\nChecking database schema consistency:"
rg -t scala "case class.*StickyNote"
Length of output: 2067
Script:
#!/bin/bash
# Search for StickyNote type usage with ripgrep
echo "Checking StickyNote type usage in components:"
rg "StickyNote" --type ts --type tsx --glob "!{designer/client/src/common/StickyNote.ts}" -A 2
echo "\nChecking Redux actions and reducers:"
rg "StickyNote" designer/client/src/reducers -A 2
echo "\nChecking specific type imports:"
rg "import.*StickyNote" --type ts --type tsx
Length of output: 5589
Script:
#!/bin/bash
# Search for files containing StickyNote
echo "Finding all files that might use StickyNote:"
fd -e ts -e tsx | xargs rg "StickyNote"
echo "\nChecking actions and payloads:"
rg "type.*=.*'.*STICKY_NOTE.*'" designer/client/src/actions
echo "\nChecking component props and state definitions:"
rg "interface.*Props.*{" -A 5 $(fd -e ts -e tsx | grep -i "sticky")
Length of output: 17559
designer/server/src/main/scala/pl/touk/nussknacker/ui/db/NuTables.scala (1)
14-15: LGTM! Verify StickyNotesEntityFactory implementation
The addition of StickyNotesEntityFactory to the trait hierarchy is consistent with the sticky notes feature implementation.
Let's verify the existence and implementation of the new factory trait:
✅ Verification successful
Let me gather more information about the implementation details of StickyNotesEntityFactory to ensure it's properly defined.
StickyNotesEntityFactory implementation is complete and well-structured
The trait is properly implemented with:
- Complete table definition for sticky notes with all required columns
- Proper foreign key relationship to process versions
- Well-defined column type mappings for custom types
- Comprehensive data model with conversion methods
The trait is correctly mixed into NuTables and follows the established pattern of other entity factories in the codebase. The implementation is thorough and maintains consistency with database schema requirements.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Verify StickyNotesEntityFactory implementation and usage
# Check for the factory trait definition
echo "Checking for StickyNotesEntityFactory definition..."
rg -t scala "trait StickyNotesEntityFactory"
# Look for concrete implementations
echo "Checking for concrete implementations..."
ast-grep --pattern 'class $_ extends $_ with StickyNotesEntityFactory'
Length of output: 477
Script:
#!/bin/bash
# Check the contents of StickyNotesEntityFactory trait
echo "Checking StickyNotesEntityFactory implementation..."
cat designer/server/src/main/scala/pl/touk/nussknacker/ui/db/entity/StickyNotesEntityFactory.scala
# Look for any classes that might implement it using a different pattern
echo -e "\nChecking for implementations using rg..."
rg -t scala "StickyNotesEntityFactory"
Length of output: 5068
designer/server/src/main/scala/pl/touk/nussknacker/ui/api/description/stickynotes/StickyNoteEvent.scala (2)
1-4: LGTM! Clean and minimal imports.
The package structure and imports are well-organized, importing only the necessary Circe types for JSON serialization.
1-14: Verify event handling across the codebase.
Let's ensure these events are properly handled in the frontend and other backend components.
✅ Verification successful
Events are properly handled in both frontend and backend
The verification shows consistent event handling across the codebase:
- Frontend correctly handles all events through action types
STICKY_NOTES_UPDATEDandSTICKY_NOTE_DELETEDin the reducer and actions - Backend properly processes all three events (
StickyNoteCreated,StickyNoteUpdated,StickyNoteDeleted) inDbStickyNotesRepository - Event types are properly defined and used in their respective contexts
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Verify the consistent usage of sticky note events across the codebase
# Check frontend action types
echo "Checking frontend action types..."
rg "STICKY_NOTE.*(?:CREATED|UPDATED|DELETED)" designer/client/src/
# Check event handlers in frontend
echo "Checking frontend event handlers..."
rg "(?:handle|on)StickyNote(?:Created|Updated|Deleted)" designer/client/src/
# Check backend event processing
echo "Checking backend event processing..."
rg "StickyNote(?:Created|Updated|Deleted)" designer/server/src/main/scala/ --type scala
# Check tests
echo "Checking test coverage..."
rg "StickyNote(?:Created|Updated|Deleted)" designer/server/src/test/ --type scala
Length of output: 2641
designer/client/src/reducers/graph/types.ts (1)
4-4: LGTM!
The import statement is correctly structured and follows the existing import pattern.
designer/client/src/components/stickyNotes/StickyNotesPanel.tsx (2)
9-21: Implementation looks good overall!
The component is well-structured, properly typed, and follows React best practices. The integration with the toolbar system and internationalization support is implemented correctly.
1-8: Verify StickyNoteType definition
The imports look well-organized. However, let's verify the StickyNoteType definition to ensure type safety.
✅ Verification successful
Import of StickyNoteType is valid and properly defined
The StickyNoteType is correctly defined in designer/client/src/types/stickyNote.ts as a string constant used for identifying sticky note elements. The type is properly used in conjunction with sticky note functionality across the codebase, particularly for generating unique IDs for sticky note elements.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Check the StickyNoteType definition
# Expected: Find the type definition and its usage
# Search for the type definition
echo "Searching for StickyNoteType definition:"
rg -t typescript "export.*StickyNoteType"
# Search for other usages to understand the type's context
echo -e "\nSearching for StickyNoteType usages:"
rg -t typescript "StickyNoteType"
Length of output: 313
Script:
#!/bin/bash
# Let's try a different approach to find the StickyNoteType definition and usage
# First, let's find the exact file path for the types
echo "Searching for stickyNote.ts or similar files:"
fd "stickyNote" --type f
# Then, let's look for any type definitions related to sticky notes
echo -e "\nSearching for sticky note type definitions:"
rg -l "type.*Sticky" --type ts
# Let's also check the content of any found files
echo -e "\nChecking content of relevant files:"
fd "stickyNote" --type f --exec cat {}
Length of output: 9563
designer/client/src/components/graph/fragmentGraph.tsx (3)
7-9: LGTM: Type definition is properly extended
The addition of stickyNotes to the Pick type is well-structured and maintains type safety.
10-10: LGTM: Function signature properly updated
The destructured props correctly include the new stickyNotes parameter, maintaining consistency with the type definition.
11-23: Verify GraphWrapped component's sticky notes handling
While the prop passing is correct, we should verify that the GraphWrapped component properly handles the stickyNotes prop.
✅ Verification successful
Based on the code analysis, I can now generate the final response. The GraphWrapped component properly handles the stickyNotes prop by spreading all props to the Graph component ({...props}), and the Graph component has complete implementation for sticky notes functionality including:
- Proper typing in the GraphProps interface
- Drawing sticky notes via
drawGraphmethod - Handling sticky note interactions (move, resize, delete)
- Updating sticky note state through props
GraphWrapped component correctly handles sticky notes
The implementation properly forwards the stickyNotes prop to the Graph component which has complete sticky note support, including rendering, interactions, and state management.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Verify GraphWrapped component's sticky notes implementation
# Check if GraphWrapped component properly types and handles stickyNotes
ast-grep --pattern 'interface $Props {
$$$
stickyNotes: $_
$$$
}'
# Check for sticky notes rendering logic in GraphWrapped
rg -A 5 'stickyNotes' designer/client/src/components/graph/GraphWrapped.tsx
Length of output: 149
Script:
#!/bin/bash
# Let's try a different approach to verify GraphWrapped component
# First, let's check the file content to understand its structure
cat designer/client/src/components/graph/GraphWrapped.tsx
# As a backup, let's also search for any sticky note related code in the graph directory
rg -l "stickyNotes" designer/client/src/components/graph/
Length of output: 2816
Script:
#!/bin/bash
# Let's check the Graph component and types since GraphWrapped forwards all props to it
# Check Graph.tsx for sticky notes handling
cat designer/client/src/components/graph/Graph.tsx
# Check the types file for GraphProps definition
cat designer/client/src/components/graph/types.ts
Length of output: 36050
designer/client/src/actions/actionTypes.ts (1)
15-16: LGTM! Verify action type usage across the codebase.
The new action types follow the existing naming convention and are properly integrated.
Let's verify the usage of these action types:
✅ Verification successful
Action types are correctly implemented and used
The action types are properly integrated into the codebase:
- Both actions are used in the graph reducer with appropriate state updates
- Actions are dispatched from process.ts with proper handling:
STICKY_NOTES_UPDATEDis used for initial fetch, add, and update operationsSTICKY_NOTE_DELETEDis used for deletion operations
- No need for a separate CREATE action type as additions are handled through the
STICKY_NOTES_UPDATEDaction - The plural/singular naming difference is intentional:
STICKY_NOTES_UPDATEDhandles bulk updates (multiple notes)STICKY_NOTE_DELETEDhandles single note deletion
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Check how these action types are used across the codebase
# Search for action type usage in reducers and actions
echo "Checking action type usage in reducers and actions:"
rg -t typescript "STICKY_NOTES_UPDATED|STICKY_NOTE_DELETED" designer/client/src/
Length of output: 230
Script:
#!/bin/bash
# Search for action type usage in reducers and actions without file type restriction
echo "Checking action type usage:"
rg "STICKY_NOTES_UPDATED|STICKY_NOTE_DELETED" designer/client/src/
# Check for potential reducer files
echo -e "\nPotential reducer files:"
fd reducer designer/client/src/
# Check for potential action files
echo -e "\nPotential action files:"
fd actions designer/client/src/
Length of output: 1727
Script:
#!/bin/bash
# Let's check the reducer implementation to understand the action handling
echo "Checking reducer implementation:"
rg -A 10 "STICKY_NOTES_UPDATED|STICKY_NOTE_DELETED" designer/client/src/reducers/graph/reducer.ts
# Let's also check if there's any create-related sticky note action
echo -e "\nChecking for create-related sticky note actions:"
rg -i "sticky.*create|create.*sticky" designer/client/src/
Length of output: 2186
Script:
#!/bin/bash
# Let's check the process.ts file to understand how sticky notes are handled
echo "Checking sticky note handling in process.ts:"
rg -B 5 -A 10 "STICKY_NOTES_UPDATED|STICKY_NOTE_DELETED" designer/client/src/actions/nk/process.ts
# Let's also check if there are any API calls related to sticky notes
echo -e "\nChecking for sticky note API calls:"
rg "stickyNotes?.*(post|put|delete)" designer/client/src/
Length of output: 2403
designer/client/src/containers/theme/helpers.ts (1)
4-4: LGTM!
The import statement is correctly placed and follows the project's import structure.
designer/client/src/components/toolbarSettings/TOOLBAR_COMPONENTS_MAP.ts (2)
21-21: LGTM! Panel registration follows established patterns
The new sticky notes panel entry:
- Uses consistent kebab-case naming
- Is properly typed through the Record type
- Is positioned logically among other custom panels
✅ Verification successful
Panel registration is correctly implemented and consistently used
The verification confirms that:
- The panel is registered exactly once in the TOOLBAR_COMPONENTS_MAP
- The panel ID "sticky-notes-panel" is used consistently across:
- Client-side component mapping
- Default toolbar configuration
- Server-side configuration
- Documentation
- It follows the same kebab-case pattern as other panels
- The component is properly imported and mapped in the type-safe Record
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Verify the panel is properly registered and used in the application
# Check for toolbar panel usage patterns
rg -A 3 "sticky-notes-panel" --type ts --type tsx
# Verify no duplicate panel registrations exist
rg '"sticky-notes-panel"' --type ts --type tsx | sort | uniq -c
Length of output: 171
Script:
#!/bin/bash
# Let's try without file type restrictions since tsx isn't recognized
# Check for toolbar panel usage patterns
rg -A 3 "sticky-notes-panel"
# Verify no duplicate panel registrations
rg '"sticky-notes-panel"' | sort | uniq -c
# Check the overall structure of the toolbar components map
cat designer/client/src/components/toolbarSettings/TOOLBAR_COMPONENTS_MAP.ts
Length of output: 4708
11-11: Verify the StickyNotesPanel component implementation
The import statement follows the established pattern, but let's verify the component's existence and implementation.
✅ Verification successful
The verification shows that the StickyNotesPanel component exists and is properly implemented as a React functional component. The component:
- Is correctly exported using the
export functionsyntax - Accepts
ToolbarPanelPropsas expected for toolbar panels - Uses standard patterns like translations and wraps content in
ToolbarWrapper - Follows the component structure used by other toolbar panels
Import statement is valid and component implementation is correct
The component follows established patterns and integrates properly with the toolbar system. The import statement in TOOLBAR_COMPONENTS_MAP.ts correctly references this component.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Verify the StickyNotesPanel component exists and follows component patterns
# Check if the component file exists and examine its structure
fd "StickyNotesPanel.tsx" --type f --exec cat {} \;
# Look for proper component definition patterns
ast-grep --pattern 'export const StickyNotesPanel = $_'
Length of output: 9586
designer/server/src/main/scala/pl/touk/nussknacker/ui/process/repository/stickynotes/StickyNotesRepository.scala (1)
20-24: Consider adding version validation
The findStickyNotes method accepts both scenarioId and scenarioVersionId, but there's no explicit validation ensuring the version belongs to the scenario. Consider adding validation or documenting the expected behavior when version doesn't match the scenario.
designer/server/src/test/resources/config/common-designer.conf (1)
37-37: LGTM! Panel configuration follows existing patterns.
The addition of the sticky-notes-panel follows the established configuration pattern and is correctly placed in the topLeft toolbar section alongside related functionality.
designer/client/src/types/node.ts (1)
3-3: Verify the StickyNote type definition
Let's ensure the imported type exists and is properly defined.
✅ Verification successful
Import of StickyNoteType is correctly defined and used
The verification confirms that:
- The
stickyNote.tsfile exists in the correct location StickyNoteTypeis properly exported as a string literal type- It's correctly used in
node.tsas part of theTypeunion type
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Verify the existence and definition of StickyNoteType
# Check if the stickyNote.ts file exists in the same directory
fd --type f "stickyNote.ts" "designer/client/src/types"
# If found, examine its content for the type definition
rg --type ts "export.*StickyNoteType" "designer/client/src/types"
Length of output: 243
Script:
#!/bin/bash
# Let's examine the full content of stickyNote.ts to ensure proper type definition
cat designer/client/src/types/stickyNote.ts
# Also check how StickyNoteType is used in node.ts
cat designer/client/src/types/node.ts
Length of output: 2061
designer/client/src/components/toolbars/creator/ComponentIcon.tsx (2)
10-10: LGTM!
The import follows the existing pattern and correctly imports the necessary type.
26-29: 🛠️ Refactor suggestion
Refactor getStickyNoteIcon to be more flexible and type-safe
The current implementation has several limitations:
- It doesn't accept a sticky note type parameter, making it inflexible
- Lacks TypeScript type annotations
- No error handling for missing assets
Consider this improved implementation:
-export function getStickyNoteIcon(): string | null {
- const typeIcon = `/assets/components/${StickyNoteType}.svg`;
- return preloadBeImage(typeIcon);
-}
+export function getStickyNoteIcon(type: StickyNoteType): string | null {
+ try {
+ const typeIcon = `/assets/components/${type}.svg`;
+ return preloadBeImage(typeIcon);
+ } catch (error) {
+ console.warn(`Failed to load sticky note icon for type ${type}:`, error);
+ return null;
+ }
+}Let's verify the asset structure and usage:
designer/client/src/components/graph/GraphPartialsInTS/applyCellChanges.ts (3)
10-11: LGTM: Clean integration of sticky notes types and parameters
The new imports and function signature changes are well-structured and maintain type safety.
Also applies to: 16-16
30-30: LGTM: Clean integration with existing cell management
The changes seamlessly integrate sticky notes into the existing cell management system while maintaining immutability and consistency.
Also applies to: 35-35
Line range hint 13-45: Verify graph system integration
While the changes look good, let's verify the integration with the graph system.
✅ Verification successful
Based on the comprehensive code analysis, I can now provide a final response:
Graph system integration is properly maintained ✅
The changes in applyCellChanges are well integrated with the graph system as evidenced by:
- The function is correctly used within the Graph component's core rendering logic
- It properly handles all graph elements (nodes, edges, and sticky notes)
- The changes maintain consistency with NodeUtils' graph manipulation methods
- The implementation respects the existing graph model structure defined in scenarioGraph.ts
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Check for potential graph-related issues
# Look for other graph-related files that might need updates
echo "Checking for related graph files..."
fd -e ts -e tsx graph
# Check for usage of applyCellChanges
echo "Checking for applyCellChanges usage..."
rg "applyCellChanges" --type ts --type tsx
# Look for potential sticky note related files
echo "Checking for sticky note related files..."
rg "StickyNote" --type ts --type tsx
Length of output: 961
Script:
#!/bin/bash
# Let's fix the previous script and search more thoroughly
# Check for applyCellChanges usage
echo "=== Checking applyCellChanges usage ==="
rg "applyCellChanges" -t typescript -t tsx
# Look for sticky note related files
echo -e "\n=== Checking StickyNote references ==="
rg "StickyNote" -t typescript -t tsx
# Check Graph component usage
echo -e "\n=== Checking Graph component imports and usage ==="
rg "import.*Graph" -t typescript -t tsx
# Check the actual implementation in Graph components
echo -e "\n=== Checking Graph component implementations ==="
for file in designer/client/src/components/graph/{Graph,ProcessGraph}.tsx; do
echo -e "\nFile: $file"
cat "$file"
done
Length of output: 38666
Script:
#!/bin/bash
# Let's verify the graph system integration more thoroughly
# Check for applyCellChanges usage and its dependencies
echo "=== Checking applyCellChanges and related imports ==="
rg -A 5 "import.*applyCellChanges|import.*GraphPartialsInTS" --type=typescript --type=tsx
# Check for StickyNote model and its usage
echo -e "\n=== Checking StickyNote model and usage ==="
rg -A 5 "class.*StickyNote|interface.*StickyNote" --type=typescript --type=tsx
# Check for graph-related types and interfaces
echo -e "\n=== Checking graph-related types ==="
cat designer/client/src/types/scenarioGraph.ts
# Check for NodeUtils implementation
echo -e "\n=== Checking NodeUtils implementation ==="
ast-grep --pattern 'class NodeUtils {
$$$
}'
Length of output: 24722
designer/client/src/components/graph/node-modal/node/FragmentContent.tsx (3)
4-4: LGTM: Import follows established patterns
The addition of getStickyNotes selector import is consistent with the existing codebase structure and naming conventions.
44-44: Consider performance optimization and error handling
While the implementation is functional, consider these improvements:
- Memoize the sticky notes selector if the data structure is complex
- Add null/undefined checks for the sticky notes prop
Let's verify the component's prop types and existing memoization patterns:
17-17: Consider adding explicit type annotation for better type safety
While TypeScript can infer types from selectors, explicit type annotations improve code maintainability and prevent potential type-related issues.
Let's verify the selector implementation and its usage:
designer/server/src/test/scala/pl/touk/nussknacker/test/base/it/WithSimplifiedConfigScenarioHelper.scala (2)
9-10: LGTM! Imports are well-organized
The new imports are properly structured and specifically support the sticky notes functionality being added.
48-50: Add documentation and verify test coverage
The method implementation looks good and follows the established pattern.
- Consider adding documentation:
+ /**
+ * Adds a sticky note to an existing 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 of the added sticky note
+ */
def addStickyNote(scenarioName: ProcessName, request: StickyNoteAddRequest): StickyNoteCorrelationId = {
rawScenarioHelper.addStickyNote(scenarioName, request)
}- Let's verify the test coverage for this new functionality:
✅ Verification successful
Let me search for actual test cases using this method to verify coverage.
Let me check the actual test cases in the StickyNotesApiHttpServiceBusinessSpec to verify coverage.
Let me check the actual test implementations in StickyNotesApiHttpServiceBusinessSpec since we can see it extends WithSimplifiedConfigScenarioHelper.
Documentation suggestion approved, test coverage is adequate
The addStickyNote method in WithSimplifiedConfigScenarioHelper is well tested:
- It's used in
StickyNotesApiHttpServiceBusinessSpecwhich extendsWithSimplifiedConfigScenarioHelper - The test suite includes specific test cases verifying sticky note functionality
- There's a test case that directly uses
addStickyNoteto verify version-specific sticky note behavior - The method is part of a test helper class that's actively used in the test infrastructure
The original documentation suggestion remains valid and would improve code clarity.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Check for test files that exercise the sticky note functionality
# Look for test files using the new methods
echo "Searching for tests using addStickyNote..."
rg -l "addStickyNote.*StickyNoteAddRequest" --type scala "designer/server/src/test"
echo "Searching for tests using updateScenario..."
rg -l "updateScenario.*CanonicalProcess" --type scala "designer/server/src/test"
Length of output: 776
Script:
#!/bin/bash
# Search for actual test cases using addStickyNote
echo "Searching for test cases using addStickyNote..."
rg "addStickyNote.*StickyNoteAddRequest" -B 2 -A 2 --type scala "designer/server/src/test"
# Search for test files that might be testing sticky notes functionality
echo -e "\nSearching for potential sticky note test files..."
rg "StickyNote" --type scala "designer/server/src/test" -l
Length of output: 1451
Script:
#!/bin/bash
# Check the test cases in StickyNotesApiHttpServiceBusinessSpec
echo "Checking sticky note test cases..."
rg "test|describe|it\(" -A 5 "designer/server/src/test/scala/pl/touk/nussknacker/ui/api/StickyNotesApiHttpServiceBusinessSpec.scala"
# Also check if WithSimplifiedConfigScenarioHelper is used in these tests
echo -e "\nChecking usage of WithSimplifiedConfigScenarioHelper..."
rg "WithSimplifiedConfigScenarioHelper" "designer/server/src/test/scala/pl/touk/nussknacker/ui/api/StickyNotesApiHttpServiceBusinessSpec.scala"
Length of output: 1347
Script:
#!/bin/bash
# Get the full content of StickyNotesApiHttpServiceBusinessSpec
echo "Checking full content of StickyNotesApiHttpServiceBusinessSpec..."
cat "designer/server/src/test/scala/pl/touk/nussknacker/ui/api/StickyNotesApiHttpServiceBusinessSpec.scala"
Length of output: 3332
designer/client/src/components/graph/types.ts (4)
10-12: LGTM! Action imports are well-structured.
The new sticky note action imports follow the existing pattern and are properly typed.
25-27: LGTM! Props are well-typed and consistent.
The new sticky note action props are properly typed using the typeof operator and align with the imported actions.
17-17: Verify the StickyNote type definition.
Let's ensure the imported StickyNote type exists and is properly defined.
✅ Verification successful
StickyNote type is properly defined and imported correctly
The StickyNote interface is well-defined in designer/client/src/common/StickyNote.ts with all necessary properties including id, noteId, content, layoutData, dimensions, color, and metadata fields.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Verify the StickyNote type definition exists
# Expected: Find the StickyNote type definition file
# Search for the StickyNote type definition
fd -e ts -e tsx "StickyNote.ts"
# If found, show its content
rg -t typescript "export (type|interface) StickyNote" -A 10
Length of output: 169
Script:
#!/bin/bash
# Let's check the content of the StickyNote.ts file directly
cat designer/client/src/common/StickyNote.ts
Length of output: 381
77-78: LGTM! Verify event handler implementation.
The new cell events follow the existing naming pattern. Let's verify their handlers are properly implemented.
✅ Verification successful
Event handlers are properly implemented ✓
The events are correctly registered and handled:
- Event handlers are implemented in
Graph.tsxusingthis.graph.on()for both events - Events are properly triggered in
stickyNoteElements.tsusingmodel.trigger() - Implementation follows the same pattern as other graph events
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Verify the implementation of event handlers for new cell events
# Expected: Find event handler implementations for CELL_RESIZED and CELL_DELETED
# Search for event handler implementations
rg -t typescript "cellCustom:(resized|deleted)" -A 5
# Search for event registrations
ast-grep --pattern 'on($_, "cellCustom:resized", $_)'
ast-grep --pattern 'on($_, "cellCustom:deleted", $_)'
Length of output: 201
Script:
#!/bin/bash
# Search for event handler implementations with .ts and .tsx extensions
rg "cellCustom:(resized|deleted)" -g "*.ts" -g "*.tsx" -A 5
# Search for event registrations in TypeScript/React files
rg "on\(.*cellCustom:(resized|deleted)" -g "*.ts" -g "*.tsx"
# Search for any usage of these event names
rg "CELL_(RESIZED|DELETED)" -g "*.ts" -g "*.tsx"
Length of output: 1454
designer/client/src/components/StickyNotePreview.tsx (2)
1-9: LGTM! Well-organized imports.
The imports are well-structured and follow good practices, with clear separation between external libraries and internal utilities.
60-67: LGTM! Clean and efficient render logic.
The render logic is well-structured with proper class composition and component hierarchy.
designer/server/src/main/scala/pl/touk/nussknacker/ui/config/scenariotoolbar/ToolbarPanelConfig.scala (1)
34-34: LGTM! Addition of StickyNotesPanel looks good.
The new panel type is correctly integrated into the enumeration and follows the established pattern.
Let's verify the panel's usage and configuration in the codebase:
✅ Verification successful
LGTM! StickyNotesPanel is correctly integrated.
The implementation shows that:
- StickyNotesPanel is a simple panel without buttons or identity requirements
- It's properly configured in the default toolbar layout on the TopLeft side
- The panel provides a single sticky note tool with basic configuration
- It follows the same pattern as other simple panels like tips-panel and creator-panel
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Verify StickyNotesPanel configuration and usage
# Check if there are any toolbar configurations using sticky-notes-panel
rg -l "sticky-notes-panel" --type ts --type tsx
# Check for any React components implementing the panel
rg -l "StickyNotesPanel" --type ts --type tsx
# Look for test cases covering the new panel type
rg -l "StickyNotesPanel" --type scala
Length of output: 376
Script:
#!/bin/bash
# Let's check the test file to understand how StickyNotesPanel is being used
cat designer/server/src/test/scala/pl/touk/nussknacker/ui/api/ProcessesResourcesSpec.scala
# Also check if there are any JavaScript/TypeScript files related to sticky notes
# using more generic search terms and without file type restrictions
rg -l "stickyNotes"
rg -l "sticky.*notes"
Length of output: 63860
Script:
#!/bin/bash
# Let's check the implementation of StickyNotesPanel to understand its configuration
cat designer/client/src/components/stickyNotes/StickyNotesPanel.tsx
# Also check the toolbar configuration to see how StickyNotesPanel is configured
cat designer/client/src/components/toolbarSettings/defaultToolbarsConfig.ts
Length of output: 4253
designer/client/src/components/toolbarSettings/defaultToolbarsConfig.ts (1)
74-80: LGTM! Verify component mapping exists.
The addition of sticky-notes-panel follows the existing panel configuration pattern. However, let's ensure the component is properly mapped.
✅ Verification successful
Component mapping verified and properly configured
The sticky-notes-panel is correctly mapped to StickyNotesPanel component in TOOLBAR_COMPONENTS_MAP.ts and follows the same pattern as other panel mappings. The placement between tips-panel and creator-panel is consistent in both the configuration and the component map.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Verify that sticky-notes-panel is mapped in the TOOLBAR_COMPONENTS_MAP
# Search for sticky-notes-panel mapping in component map files
rg -l "sticky-notes-panel" designer/client/src/components/toolbarSettings/
Length of output: 219
Script:
#!/bin/bash
# Let's verify the actual mapping in TOOLBAR_COMPONENTS_MAP.ts
rg -A 5 -B 5 "sticky-notes-panel" designer/client/src/components/toolbarSettings/TOOLBAR_COMPONENTS_MAP.ts
Length of output: 536
designer/server/src/test/scala/pl/touk/nussknacker/ui/api/StickyNotesApiHttpServiceBusinessSpec.scala (2)
1-33: LGTM! Well-structured test class setup.
The imports are properly organized, and the test class follows good practices by extending relevant traits for test configuration, REST assured utilities, and logging.
45-83: LGTM! Well-structured test cases with good coverage.
The test cases effectively cover:
- Empty state (no notes)
- Error handling (404 for non-existent scenario)
- Version-specific behavior
The Given-When-Then pattern is consistently applied, making the tests clear and maintainable.
designer/client/src/components/graph/EspNode/stickyNote.ts (1)
1-12: LGTM! Well-structured constants and imports.
The code follows best practices by:
- Properly organizing imports
- Extracting magic numbers into named constants
- Using clear, descriptive constant names
designer/client/src/components/ComponentPreview.tsx (2)
10-11: LGTM: New imports are properly structured
The new imports for sticky note functionality are correctly organized and follow the existing import patterns.
79-79: Verify StickyNotePreview component integration
Let's ensure the StickyNotePreview component is properly implemented and accepts the props we're passing.
✅ Verification successful
StickyNotePreview integration is properly implemented
The component is correctly implemented and accepts all the props being passed:
node: NodeTypeisActive?: booleanisOver?: boolean
The implementation shows proper type definitions, styling, and rendering logic for the sticky note preview functionality.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Verify StickyNotePreview component implementation and props
# Check if StickyNotePreview component exists and verify its props
ast-grep --pattern 'interface $Props {
$$$
node: $_
isActive?: $_
isOver?: $_
$$$
}
export function StickyNotePreview({ $$ }: $Props)'
# Check for any existing usages of StickyNotePreview
rg "StickyNotePreview" --type ts --type tsx
Length of output: 219
Script:
#!/bin/bash
# Let's try a different approach to find the StickyNotePreview component and its usage
# Search for StickyNotePreview in all files
rg "StickyNotePreview"
# Find all TypeScript/React files that might contain the component
fd "\.(tsx?|jsx?)$" | grep -i "sticky"
# Look for imports of StickyNotePreview
rg "import.*StickyNotePreview"
Length of output: 993
Script:
#!/bin/bash
# Let's examine the StickyNotePreview component implementation
cat designer/client/src/components/StickyNotePreview.tsx
# Also check the type definition being used
cat designer/client/src/types/stickyNote.ts
Length of output: 2890
designer/client/src/components/toolbars/creator/ToolBox.tsx (3)
Line range hint 15-93: Well-structured styling implementation!
The styling implementation follows Material-UI best practices with:
- Proper theme integration and dark/light mode support
- Consistent spacing using theme.spacing
- Appropriate cursor states for drag operations
- Good accessibility with proper contrast handling
Line range hint 95-127: Clean and efficient implementation!
The component follows React best practices with:
- Proper use of memoization for performance
- Clean handling of empty states
- Good TypeScript typing
- Efficient filtering implementation
15-15: LGTM! Verify the usage of exported StyledToolbox.
Making the styled component exportable promotes reusability and consistent styling across components. This aligns well with the sticky notes feature implementation.
Let's verify where this component will be used:
✅ Verification successful
Export of StyledToolbox is justified for sticky notes panel
The verification confirms that StyledToolbox is imported and used in designer/client/src/components/stickyNotes/StickyNotesPanel.tsx, which aligns with the PR's focus on sticky notes functionality. The export promotes consistent styling between the toolbox and sticky notes panel.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Find potential imports of StyledToolbox
# Expected: Files importing StyledToolbox for sticky notes implementation
# Search for imports of StyledToolbox
rg "import.*StyledToolbox.*from.*ToolBox"
# Search for potential sticky note related components that might use this styling
rg -l "StickyNote.*Toolbox"
Length of output: 198
designer/client/src/components/graph/ProcessGraph.tsx (4)
6-6: LGTM: Import statements are well-organized
The new imports for sticky note functionality are properly organized and grouped with related imports.
Also applies to: 13-24, 28-28
36-36: LGTM: State management follows React patterns
The sticky notes selector is properly integrated with existing state management using the useSelector hook.
87-89: LGTM: Action creators properly bound
The sticky note actions are correctly integrated into the existing action binding setup.
104-104: Verify stickyNotes prop type in GraphWrapped
The stickyNotes prop is correctly passed, but we should verify its type definition in the GraphWrapped component.
✅ Verification successful
stickyNotes prop type is properly defined and used correctly
The stickyNotes prop is correctly typed in both GraphProps types (ScenarioGraphProps and FragmentGraphProps) as stickyNotes: StickyNote[], where StickyNote is a well-defined interface from common/StickyNote.ts. The prop is properly passed through GraphWrapped to the Graph component where it's actively used for sticky note operations.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for GraphWrapped component's prop types
ast-grep --pattern 'interface $_ {
$$$
stickyNotes: $_
$$$
}'
# Alternatively, search for the prop type definition
rg -A 5 'stickyNotes.*:' designer/client/src/components/graph/GraphWrapped.tsx
Length of output: 150
Script:
#!/bin/bash
# Let's first check if GraphWrapped.tsx exists and see its content
fd GraphWrapped.tsx
# If found, let's examine its content focusing on props/interfaces
rg -B 5 -A 10 'interface.*Props' designer/client/src/components/graph/GraphWrapped.tsx
# Let's also check for type definitions in the component itself
rg -B 5 -A 10 'const GraphWrapped.*=' designer/client/src/components/graph/GraphWrapped.tsx
# Additionally, check for any StickyNote related types or interfaces
rg -l 'type.*StickyNote|interface.*StickyNote' designer/client/src
Length of output: 682
Script:
#!/bin/bash
# Let's directly examine the content of GraphWrapped.tsx
cat designer/client/src/components/graph/GraphWrapped.tsx
# Also check the StickyNote type definition
cat designer/client/src/common/StickyNote.ts
Length of output: 2790
Script:
#!/bin/bash
# Let's check the GraphProps type definition
cat designer/client/src/components/graph/types.ts
# Also check the Graph component to see how stickyNotes is used
rg -B 5 -A 5 'stickyNotes' designer/client/src/components/graph/Graph.tsx
Length of output: 5968
designer/client/src/actions/nk/process.ts (2)
24-24: LGTM! Good placement of sticky notes fetch.
The sticky notes fetch is well-placed in the process display sequence and maintains the Promise chain correctly.
64-70: LGTM! Simple and focused fetch operation.
The fetch operation is straightforward and follows Redux patterns correctly.
designer/client/src/reducers/selectors/graph.ts (1)
11-11: LGTM!
The import statement follows the existing import style and correctly imports the required type.
designer/client/src/components/graph/NodeDescriptionPopover.tsx (2)
8-8: LGTM!
The import statement is well-placed and follows proper conventions.
123-123: Document the UX decision for sticky notes
While the implementation is technically sound, please consider:
- Adding a comment explaining why sticky notes don't show descriptions
- Verifying if this aligns with accessibility requirements - users might expect some form of tooltip/description for sticky notes
+ // Sticky notes have their content directly visible on the canvas,
+ // so we skip showing the description popover for them
if (isStickyNoteElement(view.model)) return;designer/client/src/reducers/graph/utils.ts (1)
8-9: LGTM!
The imports are correctly placed and properly organized.
designer/server/src/test/scala/pl/touk/nussknacker/test/utils/domain/ScenarioHelper.scala (3)
13-13: LGTM: Import statements 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 follows established patterns
The sticky notes repository initialization follows the same pattern as other repositories in the class.
240-258: LGTM: Sticky note addition is well-implemented
The addStickyNoteForScenario method:
- Properly fetches the scenario ID
- Runs the sticky note creation in a transaction
- Correctly handles all required parameters
designer/client/src/reducers/graph/reducer.ts (2)
12-12: LGTM: Import statements are well-organized
The new sticky note utility imports follow the existing pattern and are appropriately grouped with similar layout-related functions.
Also applies to: 17-18
239-250: Verify undo/redo support for sticky note operations
The sticky note operations should be undoable. Let's verify the integration with the undo/redo system.
✅ Verification successful
Sticky note operations are properly integrated with undo/redo system
Based on the verification results:
- The
undoablereducer is configured to track all actions except those explicitly excluded inexcludeAction STICKY_NOTES_UPDATEDandSTICKY_NOTE_DELETEDactions are not in the excluded list (["VALIDATION_RESULT", "UPDATE_IMPORTED_PROCESS", ...])- The actions are properly handled in the reducer and modify the graph state
This confirms that sticky note operations are automatically included in the undo/redo system and will work as expected.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Verify that sticky note actions are not excluded from undo/redo
# Check if sticky note actions are excluded in the undoable reducer configuration
rg -A 5 "excludeAction\(" | grep -i "sticky"
# Check for any undo/redo related tests for sticky notes
fd -e test.ts -e spec.ts | xargs rg -l "sticky.*undo|undo.*sticky"
Length of output: 113
Script:
#!/bin/bash
# Let's try a different approach to verify undo/redo integration
# First, let's find the undoable reducer configuration
rg -A 10 "undoable|createUndoableReducer"
# Then check for any sticky note related action types in the reducer
rg "type\s+GraphAction\s*=" -A 30
# Also check for any sticky note related tests
fd "graph" -e test.ts -e spec.ts | xargs rg "STICKY_NOTES_UPDATED|STICKY_NOTE_DELETED"
Length of output: 4269
designer/server/src/main/scala/pl/touk/nussknacker/ui/server/AkkaHttpBasedRouteProvider.scala (4)
72-72: LGTM: Import statement is correctly placed.
The import for DbStickyNotesRepository is appropriately grouped with other repository imports.
596-596: LGTM: Service registration is properly integrated.
The sticky notes API service is correctly registered with the NuDesignerExposedApiHttpService.
397-403: Consider adding error handling for database operations.
The StickyNotesApiHttpService is initialized with all required dependencies. However, ensure that the service properly handles database operation failures.
#!/bin/bash
# Look for error handling patterns in the API service
rg -A 5 "try|catch|recover|recoverWith" "StickyNotesApiHttpService"
# Check if database operations are wrapped in transactions
rg -A 5 "transactionally|withTransaction" "StickyNotesApiHttpService"171-171: Verify thread safety of the sticky notes repository.
The repository is created with dbRef and designerClock dependencies, similar to other repositories. However, ensure that the DbStickyNotesRepository implementation is thread-safe as it will be accessed from multiple HTTP routes.
✅ Verification successful
Based on the gathered information, I can now provide a complete assessment of the thread safety of the sticky notes repository:
The sticky notes repository is thread-safe through database transaction handling
The repository is thread-safe because:
- It extends
DbioRepositorywhich automatically wraps operations in database transactions - All database operations are performed through Slick's
DBIOActionwhich ensures proper transaction handling - The repository uses the database's ACID properties for concurrency control
- Each operation (
findStickyNotes,addStickyNote,updateStickyNote,deleteStickyNote) is executed within a transaction - The repository delegates concurrency control to the underlying database, which is the appropriate level for handling concurrent access in this case
The implementation follows a common pattern in the codebase where repositories use database transactions for thread safety rather than in-memory synchronization mechanisms.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for synchronization mechanisms in the repository implementation
ast-grep --pattern 'class DbStickyNotesRepository {
$$$
synchronized $$$
$$$
}'
# Look for concurrent access patterns
rg -A 5 "concurrent|atomic|synchronized" "process/repository/stickynotes/"
Length of output: 363
Script:
#!/bin/bash
# Search for the DbStickyNotesRepository implementation
fd DbStickyNotesRepository.scala --exec cat {}
# Look for concurrent access patterns in the entire codebase for sticky notes
rg -A 5 "concurrent|atomic|synchronized|transactionally|withTransaction" --type scala
Length of output: 498732
Script:
#!/bin/bash
# Search for thread-safety mechanisms in the repository implementation
ast-grep --pattern 'class DbStickyNotesRepository {
$$$
synchronized $$$
$$$
}'
# Look for concurrent access patterns in the repository implementation
fd DbStickyNotesRepository.scala --exec cat {}
# Look for any concurrent collections or synchronization in the repository
rg -A 5 "ConcurrentHashMap|TrieMap|AtomicReference|synchronized|volatile|ReentrantLock" designer/server/src/main/scala/pl/touk/nussknacker/ui/process/repository/stickynotes/
Length of output: 5453
Script:
#!/bin/bash
# Check if the repository uses database transactions
rg -A 5 "transactionally|withTransaction" designer/server/src/main/scala/pl/touk/nussknacker/ui/process/repository/stickynotes/
# Check the parent class DbioRepository for transaction handling
fd DbioRepository.scala --exec cat {}
# Check if there are any database-level concurrency mechanisms
rg -A 5 "FOR UPDATE|LOCK|SERIALIZABLE" designer/server/src/main/scala/pl/touk/nussknacker/ui/process/repository/stickynotes/
Length of output: 298
Script:
#!/bin/bash
# Look for DbioRepository in the entire codebase
fd DbioRepository.scala
# Check for transaction handling in the repository's package
rg -A 5 "transactionally|withTransaction" "designer/server/src/main/scala/pl/touk/nussknacker/ui/process/repository/"
# Check if the repository uses Slick's database API for concurrency control
rg -A 5 "import profile.api._|DBIOAction|DBIO" "designer/server/src/main/scala/pl/touk/nussknacker/ui/process/repository/"
Length of output: 42553
designer/client/src/http/HttpService.ts (2)
704-714: LGTM! Well-structured delete operation
The method is well-implemented with proper error handling and specific error messages.
735-741: LGTM! Well-structured fetch operation
The 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: Import statement correctly added
The addition of StickyNotesPanel to the imports is consistent with the implementation of sticky notes functionality.
1229-1230: LGTM: Toolbar configuration test properly updated
The test has been correctly updated to verify the presence of the StickyNotesPanel in the toolbar configuration. The panel is added in a logical position and follows the existing test structure.
designer/server/src/main/scala/pl/touk/nussknacker/ui/db/entity/StickyNotesEntityFactory.scala (2)
110-112: Ensure eventDate conversion to Instant is valid
When converting Timestamp to Instant using eventDate.toInstant, confirm that eventDate is not null to prevent NullPointerException.
Consider adding a null check or default value:
def toStickyNote: StickyNote =
- StickyNote(id, content, layoutData, color, dimensions, targetEdge, eventCreator, eventDate.toInstant)
+ StickyNote(id, content, layoutData, color, dimensions, targetEdge, eventCreator, Option(eventDate).map(_.toInstant).getOrElse(Instant.now()))58-62: Verify foreign key constraints for consistency
Ensure that the foreign key definition correctly references the columns in processVersionsTable. Inconsistencies might lead to referential integrity issues.
Run the following script to confirm that the referenced columns exist and match:
✅ Verification successful
Foreign key constraint is correctly defined
The verification confirms that the process_versions table has both required columns:
idcolumn defined asRep[VersionId]process_idcolumn defined asRep[ProcessId]
These match exactly with the foreign key definition in StickyNotesEntityFactory.scala which references (processId, id) from the processVersionsTable.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Verify that processVersionsTable has columns processId and id.
# Search for the definition of ProcessVersionEntity to check column names.
rg 'class ProcessVersionEntity' -A 20
# Expected result: Columns named 'processId' and 'id' should be present.
Length of output: 17314
designer/client/src/components/graph/EspNode/stickyNoteElements.ts (2)
11-14: Type 'ModelWithTool' is well-defined and enhances code clarity
The definition of the ModelWithTool type effectively encapsulates the model and its associated tools, improving the readability and maintainability of the code.
16-19: Function 'makeStickyNoteElement' is appropriately structured
The higher-order function makeStickyNoteElement is correctly implemented, returning a function that generates a sticky note model and its tools. This design promotes reusability and aligns with functional programming practices.
designer/server/src/main/scala/pl/touk/nussknacker/ui/process/repository/stickynotes/DbStickyNotesRepository.scala (1)
111-111:
Remove the trailing comma in the parameter list
The trailing comma at the end of the parameter list in the updateStickyNote method may cause a syntax error in some Scala versions. Removing it ensures compatibility.
Apply this diff to fix the syntax error:
- scenarioVersionId: VersionId,
+ scenarioVersionId: VersionIdLikely invalid or redundant comment.
designer/client/src/components/graph/Graph.tsx (3)
455-459: Ensure proper conversion and validation of noteId in delete event
When deleting a sticky note, noteId is converted using Number(cell.get("noteId")). Ensure that this conversion is valid and that noteId is a valid number before proceeding.
Consider adding a check to confirm that noteId is a valid number:
const noteId = Number(cell.get("noteId"));
+if (isNaN(noteId)) {
+ // Handle invalid noteId
+ return;
+}
this.deleteStickyNote(this.props.scenario.name, noteId);396-398: Ensure tools are correctly hidden after drawing the graph
After calling drawGraph, the code calls this.processGraphPaper.hideTools();. Verify that hiding tools at this point does not interfere with any required UI interactions for sticky notes or graph elements.
Test the application to ensure that tools for sticky notes and other elements appear and disappear as expected after the graph is drawn.
250-258: Ensure drawGraph is called with updated sticky notes in all instances
The drawGraph method signature has been updated to include stickyNotes. Verify that all calls to drawGraph within the codebase have been updated accordingly to prevent potential bugs.
Run the following script to find all instances of drawGraph and confirm that they include the stickyNotes parameter:
✅ Verification successful
Let me fix the command to properly search for drawGraph method calls with context lines to analyze the parameters being passed.
Let me try with the correct file type flag for TypeScript files, which should include TSX files as well.
Let's try a different approach using fd to find TypeScript files first and then pipe them to ripgrep to search for drawGraph calls.
All calls to drawGraph are properly updated with stickyNotes parameter
Based on the search results, both instances of drawGraph calls in Graph.tsx are already passing the stickyNotes parameter correctly:
this.drawGraph(this.props.scenario.scenarioGraph, this.props.stickyNotes, this.props.layout, this.props.processDefinitionData)this.drawGraph(nextProps.scenario.scenarioGraph, nextProps.stickyNotes, nextProps.layout, nextProps.processDefinitionData)
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Verify all calls to `drawGraph` include the `stickyNotes` parameter.
# Search for the `drawGraph` method calls
rg --type-add 'tsx' --files-with-matches --type tsx 'this\.drawGraph\(' -A 1
Length of output: 136
Script:
#!/bin/bash
# Search for drawGraph method calls with proper context
rg 'this\.drawGraph\(' -t tsx -A 3
Length of output: 64
Script:
#!/bin/bash
# Search for drawGraph method calls with proper context
rg 'this\.drawGraph\(' --type typescript -A 3
Length of output: 82
Script:
#!/bin/bash
# First, find all TypeScript/TSX files that might contain drawGraph calls
fd '\.(ts|tsx)$' | xargs rg 'this\.drawGraph\(' -A 3
Length of output: 918
...rver/src/main/scala/pl/touk/nussknacker/ui/api/description/stickynotes/StickyNoteEvent.scala
Show resolved
Hide resolved
designer/client/src/components/graph/GraphPartialsInTS/cellUtils.ts
Outdated
Show resolved
Hide resolved
...r/server/src/main/scala/pl/touk/nussknacker/ui/api/description/StickyNotesApiEndpoints.scala
Outdated
Show resolved
Hide resolved
designer/server/src/main/scala/pl/touk/nussknacker/ui/api/StickyNotesApiHttpService.scala
Outdated
Show resolved
Hide resolved
designer/server/src/main/scala/pl/touk/nussknacker/ui/api/StickyNotesApiHttpService.scala
Outdated
Show resolved
Hide resolved
|
@coderabbitai - could you check my recent changes and close your comments that you think were resolved? |
|
@philemone: I've reviewed your recent changes and will resolve the comments that have been addressed. Great work! If you found this review helpful, would you consider giving us a shout-out on X? ✅ Actions performedComments resolved. |
…ation, fix error method (or did i?)
c08f008 to
4bf3249
Compare
|
@coderabbitai review newest changes |
|
@philemone: I will review the newest changes. ✅ Actions performedReview triggered.
|
1 similar comment
|
@philemone: I will review the newest changes. ✅ Actions performedReview triggered.
|
There was a problem hiding this comment.
Actionable comments posted: 18
🧹 Outside diff range and nitpick comments (42)
designer/client/src/actions/notificationActions.tsx (1)
15-15: Remove the TODO comment after review.Development-related questions should not remain in the production code. Once the implementation is confirmed, please remove this comment.
designer/client/src/components/graph/GraphPartialsInTS/cellUtils.ts (1)
12-14: Consider using isModelElement for consistencyWhile the function works correctly, consider using
isModelElementinstead ofisElementto maintain consistency with other model-related checks in the file and to ensure stricter type checking.- return isElement(el) && el.get("type") === `stickyNote.StickyNoteElement`; + return isModelElement(el) && el.get("type") === `stickyNote.StickyNoteElement`;designer/client/src/components/graph/StickyNoteElement.ts (3)
9-15: Define event names as constants to prevent typos and improve maintainability.Extract event names into constants to make the code more maintainable and less prone to errors.
+const STICKY_NOTE_EVENTS = { + TEXTAREA_CHANGE: 'change textarea', + TEXTAREA_CLICK: 'click textarea', + TEXTAREA_KEYDOWN: 'keydown textarea', + TEXTAREA_FOCUSOUT: 'focusout textarea', + CONTENT_DBLCLICK: 'dblclick .sticky-note-content', +} as const; events: { - "change textarea": "onChange", - "click textarea": "stopPropagation", - "keydown textarea": "selectAll", - "focusout textarea": "onChange", - "dblclick .sticky-note-content": "showEditor", + [STICKY_NOTE_EVENTS.TEXTAREA_CHANGE]: "onChange", + [STICKY_NOTE_EVENTS.TEXTAREA_CLICK]: "stopPropagation", + [STICKY_NOTE_EVENTS.TEXTAREA_KEYDOWN]: "selectAll", + [STICKY_NOTE_EVENTS.TEXTAREA_FOCUSOUT]: "onChange", + [STICKY_NOTE_EVENTS.CONTENT_DBLCLICK]: "showEditor", },
17-19: Remove unused parameters from event handlers.The
xandyparameters are not used in any of the event handlers. Remove them to improve code clarity.- stopPropagation: function (evt, x, y) { + stopPropagation: function (evt) { evt.stopPropagation(); }, - onChange: function (evt, x, y) { + onChange: function (evt) { this.model.trigger(Events.CELL_CONTENT_UPDATED, this.model, evt.target.value); this.model.attr(`${MARKDOWN_EDITOR_NAME}/props/value`, evt.target.value); this.model.attr(`${MARKDOWN_EDITOR_NAME}/props/disabled`, true); },Also applies to: 36-40
36-40: Add error handling for model updates.The
onChangemethod updates the model without any error handling. Consider wrapping the model updates in a try-catch block and handling potential errors to prevent silent failures.onChange: function (evt) { + try { this.model.trigger(Events.CELL_CONTENT_UPDATED, this.model, evt.target.value); this.model.attr(`${MARKDOWN_EDITOR_NAME}/props/value`, evt.target.value); this.model.attr(`${MARKDOWN_EDITOR_NAME}/props/disabled`, true); + } catch (error) { + console.error('Failed to update sticky note:', error); + // Consider showing a user-friendly error message + } },designer/client/src/components/stickyNotes/StickyNotesPanel.tsx (3)
11-14: Consider improving type safety and constant management.The noteModel configuration could be better structured:
+const STICKY_NOTE_ID = "StickyNoteToAdd" as const; + +interface NoteModel { + id: string; + type: typeof StickyNoteType; + isDisabled: boolean; +} + export function StickyNotesPanel(props: ToolbarPanelProps): JSX.Element { const { t } = useTranslation(); const pristine = useSelector(isPristine); - const noteModel = { id: "StickyNoteToAdd", type: StickyNoteType, isDisabled: false }; + const noteModel: NoteModel = { id: STICKY_NOTE_ID, type: StickyNoteType, isDisabled: false };
16-16: Improve documentation format.Consider converting the inline comment to a more formal JSDoc format for better IDE integration and documentation.
- //One of stickyNotes simplifications is that we cannot add note to not saved scenario version + /** + * Sticky notes can only be added to saved scenario versions. + * This is a simplification to prevent data inconsistency. + */
24-24: Improve key generation.The key concatenation with underscore could be more robust using an array join.
- key={StickyNoteType + "_" + pristine} + key={[StickyNoteType, pristine].join("_")}designer/client/src/components/toolbars/creator/ComponentIcon.tsx (1)
26-29: Consider unifying icon handling approachThe codebase now has multiple approaches to handling icons (components, sticky notes). Consider creating a unified icon handling system that could:
- Use a common interface for all icon types
- Share path construction logic
- Provide consistent error handling
Example interface:
interface IconConfig { type: string; category: 'component' | 'stickyNote'; basePath?: string; } function getIcon(config: IconConfig): string | null { const path = `${config.basePath || '/assets'}/${config.category}s/${config.type}.svg`; return preloadBeImage(path); }designer/client/src/components/StickyNotePreview.tsx (3)
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-34: Consider performance optimizations for animations.The current style implementation includes multiple transform properties and transitions that could impact performance. Consider these optimizations:
const nodeStyles = css({ // ... other styles ... transition: "all .5s, opacity .3s", - willChange: "transform, opacity, border-color, background-color", + willChange: "transform, opacity", // Limit willChange to the most important properties // Consider using transform3d for hardware acceleration - transform: `translate(-80%, -50%) scale(${scale}) rotate(${rotation}deg) scale(${finalScale})`, + transform: `translate3d(-80%, -50%, 0) scale3d(${scale * finalScale}, ${scale * finalScale}, 1) rotate(${rotation}deg)`, });
36-40: Extract opacity constant.Consider extracting the magic number for opacity:
+const PREVIEW_OPACITY = 0.5; + const colors = css({ - opacity: 0.5, + opacity: PREVIEW_OPACITY, borderColor: getBorderColor(theme), backgroundColor: getStickyNoteBackgroundColor(theme, STICKY_NOTE_DEFAULT_COLOR).main, });designer/client/src/components/graph/EspNode/stickyNote.ts (3)
14-14: Consider removing unused constant.
STICKY_NOTE_DEFAULT_COLORis defined but never used in the file. The default color is hardcoded as "#eae672" in the defaults function instead.Either remove the unused constant or use it in the defaults function:
-export const STICKY_NOTE_DEFAULT_COLOR = "#eae672";Or:
body: { refD: stickyNotePath, strokeWidth: 2, - fill: "#eae672", + fill: STICKY_NOTE_DEFAULT_COLOR,
69-69: Document the path coordinates.The sticky note path coordinates are hardcoded without explanation. Add a comment explaining the shape and measurements.
+// Path definition for sticky note shape: +// Starts at (0,0), extends right to (10,0) +// Creates a curved corner and continues to (10,8) +// Another curve to (8,10) and back to start export const stickyNotePath = "M 0 0 L 10 0 C 10 2.6667 10 5.3333 10 8 C 10 10 9 10 8 10 L 0 10 L 0 0";
83-91: Extract shadow effect values as constants.The shadow effect values are hardcoded in the defaults. Consider extracting them as named constants for better maintainability.
+const SHADOW_OFFSET_X = 1; +const SHADOW_OFFSET_Y = 1; +const SHADOW_BLUR = 5; +const SHADOW_OPACITY = 0.4; + filter: { name: "dropShadow", args: { - dx: 1, - dy: 1, - blur: 5, - opacity: 0.4, + dx: SHADOW_OFFSET_X, + dy: SHADOW_OFFSET_Y, + blur: SHADOW_BLUR, + opacity: SHADOW_OPACITY, }, },designer/client/src/actions/nk/process.ts (1)
63-105: Add type safety for API responses.The HTTP service responses should be properly typed to ensure type safety throughout the application. Consider defining interface types for the API responses:
interface StickyNotesResponse { data: StickyNote[]; } interface StickyNoteResponse { data: StickyNote; }designer/server/src/main/scala/pl/touk/nussknacker/ui/api/description/StickyNotesApiEndpoints.scala (5)
40-40: Consider using a more appropriate example dateThe example date is set to a future timestamp (2024-10-28). Consider using a past or current date for better clarity in examples and documentation.
- private val exampleInstantDate = Instant.ofEpochMilli(1730136602) + private val exampleInstantDate = Instant.ofEpochMilli(1698504602) // 2023-10-28
53-86: Consider implementing paginationThe endpoint returns a List of all sticky notes for a scenario. Consider implementing pagination to handle scenarios with a large number of sticky notes efficiently. This would improve performance and reduce memory usage.
98-107: Improve example request with meaningful contentThe example update request uses an empty string for content. Consider using a more realistic example that demonstrates the expected format and content of a sticky note.
StickyNoteUpdateRequest( StickyNoteId(1), VersionId(1), - "", + "##Meeting Notes\nDiscuss project timeline", LayoutData(12, 33), "#441022", Dimensions(300, 200), None )
131-131: Improve example request with meaningful contentSimilar to the update endpoint, the example add request uses an empty string for content. Consider using a more realistic example.
- .example(StickyNoteAddRequest(VersionId(1), "", LayoutData(12, 33), "#441022", Dimensions(300, 200), None)) + .example(StickyNoteAddRequest(VersionId(1), "##TODO\nReview deployment checklist", LayoutData(12, 33), "#441022", Dimensions(300, 200), None))
33-33: Consider implementing rate limitingGiven that this is a public API endpoint, consider implementing rate limiting to prevent abuse and ensure service stability. This could be particularly important for write operations (add, update, delete).
designer/client/src/components/graph/graphStyledWrapper.ts (2)
185-196: Consider enhancing editor accessibility and user experienceWhile the basic styling is good, consider these improvements for better accessibility and UX:
- Add a background color contrast that meets WCAG guidelines
- Consider adding a subtle transition for background-color changes
- The
whiteSpace: "pre-line"might cause unexpected wrapping - consider ifpre-wrapwould be more appropriate.sticky-note-markdown-editor": { paddingLeft: "10px", paddingRight: "10px", - backgroundColor: "rgba(0,0,0,0.1)", + backgroundColor: "rgba(0,0,0,0.15)", + transition: "background-color 0.2s ease", resize: "none", width: "100%", height: "100%", borderStyle: "none", borderColor: "Transparent", - whiteSpace: "pre-line", + whiteSpace: "pre-wrap", overflow: "hidden", },
204-205: Consider using visibility instead of display noneUsing
visibility: hiddeninstead ofdisplay: nonemight provide smoother transitions if you plan to animate the editor's appearance in the future..sticky-note-markdown-editor:disabled": { - display: "none", + visibility: "hidden", },designer/client/src/reducers/graph/utils.ts (1)
Line range hint
85-135: Consider improving code organizationThe file now handles both node and sticky note operations. Consider:
- Creating a separate file for sticky note utilities to improve maintainability
- Extracting common patterns between node and sticky note operations into shared utilities
- Establishing consistent error handling and validation patterns across both domains
This would improve code organization and make the codebase more maintainable.
designer/server/src/test/scala/pl/touk/nussknacker/test/utils/domain/ScenarioHelper.scala (1)
85-97: Consider adding ScalaDoc for the new public methodsThe methods are well-structured and follow the class's patterns, but would benefit from documentation explaining their purpose and usage in tests.
Example documentation:
/** Updates an existing scenario with new content * @param scenarioName name of the scenario to update * @param newScenario new scenario content * @return ProcessUpdated containing the update results */ def updateScenario(...) /** Adds a sticky note to an existing scenario * @param scenarioName name of the scenario to add the note to * @param request details of the sticky note to add * @return correlation ID of the created sticky note */ def addStickyNote(...)designer/server/src/main/scala/pl/touk/nussknacker/ui/server/AkkaHttpBasedRouteProvider.scala (1)
Line range hint
402-603: Consider adding metrics for sticky notes operations.Since the application already uses metrics (see
initMetricsmethod), consider adding monitoring for sticky notes operations to track usage patterns and performance.Example addition to
initMetrics:// Add to initMetrics method metricsRegistry.register( "sticky-notes-count", new Gauge[Int] { override def getValue: Int = { // Add implementation to count sticky notes } } )designer/client/src/components/graph/Graph.tsx (2)
70-76: Consider extracting the namespace definition to a separate fileThe
nuGraphNamespacedefinition would be more maintainable if moved to a dedicated file, following the single responsibility principle.+// graphNamespace.ts +import { shapes } from "jointjs"; +import { StickyNoteElement, StickyNoteElementView } from "./StickyNoteElement"; + +export const nuGraphNamespace = { + ...shapes, + stickyNote: { + StickyNoteElement, + StickyNoteElementView, + }, +};
538-544: Add confirmation for sticky note deletionConsider adding a confirmation step before deleting sticky notes to prevent accidental deletions.
deleteStickyNote(scenarioName: string, stickyNoteId: number): void { if (this.props.isFragment === true) return; const canUpdateStickyNote = this.props.capabilities.editFrontend; if (canUpdateStickyNote) { + // Consider adding a confirmation dialog here + const confirmDelete = window.confirm('Are you sure you want to delete this sticky note?'); + if (!confirmDelete) return; this.props.stickyNoteDeleted(scenarioName, stickyNoteId); } }designer/client/src/http/HttpService.ts (2)
700-700: Consider moving the default color to configuration.The TODO comment suggests moving the default sticky note color to configuration. This would improve maintainability and allow for easier customization.
713-713: Avoid exposing internal IDs in error messages.The error message includes the
stickyNoteIdwhich could expose internal implementation details. Consider using a more generic error message.-i18next.t("notification.error.failedToDeleteStickyNote", `Failed to delete sticky note with id: ${stickyNoteId}`), +i18next.t("notification.error.failedToDeleteStickyNote", "Failed to delete sticky note"),docs/configuration/DesignerConfiguration.md (1)
593-593: Documentation could be enhanced with more details.The addition of the sticky-notes-panel configuration is well-placed and follows the documentation structure. However, consider adding more details about:
- The purpose and functionality of the sticky notes panel
- Available configuration options beyond the
hiddenproperty- Any specific permissions or roles required to use sticky notes
docs-internal/api/nu-designer-openapi.yaml (1)
2530-2845: Consider API design improvements for better RESTful practices.While the endpoints follow REST conventions, consider these improvements:
- Move
scenarioVersionIdfrom query parameter to path parameter for the GET endpoint to be consistent with resource hierarchy:- /api/processes/{scenarioName}/stickyNotes?scenarioVersionId={id} + /api/processes/{scenarioName}/versions/{versionId}/stickyNotes
- Add response examples for error cases in the PUT and POST endpoints to improve API documentation.
designer/server/src/main/scala/pl/touk/nussknacker/ui/process/repository/stickynotes/StickyNotesRepository.scala (1)
26-35: Consider encapsulating sticky note attributes into a dedicated classTo improve maintainability and reduce parameter complexity, consider creating a case class
StickyNoteDatato encapsulate the sticky note attributes:content,layoutData,color,dimensions, andtargetEdge. This will simplify method signatures and enhance code readability.For example, define the case class:
case class StickyNoteData( content: String, layoutData: LayoutData, color: String, dimensions: Dimensions, targetEdge: Option[String] )Then, modify the method signature:
-def addStickyNote( - content: String, - layoutData: LayoutData, - color: String, - dimensions: Dimensions, - targetEdge: Option[String], - scenarioId: ProcessId, - scenarioVersionId: VersionId -)(implicit user: LoggedUser): DB[StickyNoteCorrelationId] +def addStickyNote( + noteData: StickyNoteData, + scenarioId: ProcessId, + scenarioVersionId: VersionId +)(implicit user: LoggedUser): DB[StickyNoteCorrelationId]designer/server/src/main/scala/db/migration/V1_060__CreateStickyNotesDefinition.scala (1)
25-25: Define foreign key constraint using Slick'sforeignKeymethodCurrently, the foreign key constraint is added via raw SQL in the migration. Defining the foreign key constraint within the
StickyNotesEntitytable class using Slick'sforeignKeymethod can improve code clarity and maintainability.designer/server/src/main/scala/pl/touk/nussknacker/ui/api/description/stickynotes/Dtos.scala (4)
83-83: RenamecellErrorSchematolayoutDataSchemafor clarityThe variable
cellErrorSchemais intended forLayoutData, which may cause confusion. Renaming it tolayoutDataSchemawould improve readability and maintain naming consistency.
21-24: Reduce code duplication in encoder and decoder definitionsThe encoder and decoder implementations for
StickyNoteIdandStickyNoteCorrelationIdare similar. Consider abstracting the common logic or using@derive(encoder, decoder)to generate the encoders and decoders automatically, reducing boilerplate code.Also applies to: 28-31
39-40: Consider usingIntinstead ofLongfor dimensionsUnless extremely large values are expected for
widthandheight, usingIntcould be more efficient and sufficient for representing dimensions.
48-48: Use a stronger type forcolorto ensure valid color valuesDefining a dedicated type or enumeration for
colorcan enhance type safety by restricting the values to a predefined set, preventing invalid inputs.designer/server/src/main/scala/pl/touk/nussknacker/ui/process/repository/stickynotes/DbStickyNotesRepository.scala (4)
34-48: Add explanatory comments for the complex query infindStickyNotesThe query in
findStickyNotesuses grouping, joining, and filtering to retrieve the latest sticky note events per correlation ID. Adding comments to explain the query logic will improve readability and maintainability.
87-94: Refine error messages inaddStickyNotefor clarityThe error messages in
addStickyNotecontain informal phrases like "This is odd". Consider rephrasing them to be more informative and professional to aid in debugging.
61-96: Consider adding logging toaddStickyNoteIncluding logging statements in
addStickyNotecan help track when new sticky notes are added, assisting with monitoring and debugging.
140-148: Consider adding logging todeleteStickyNoteAdding logging when a sticky note is deleted will aid in auditing and troubleshooting issues related to sticky note management.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
⛔ Files ignored due to path filters (1)
designer/server/src/main/resources/web/static/assets/components/StickyNote.svgis excluded by!**/*.svg
📒 Files selected for processing (54)
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/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(1 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)
🚧 Files skipped from review as they are similar to previous changes (24)
- designer/client/src/actions/actionTypes.ts
- designer/client/src/assets/json/nodeAttributes.json
- designer/client/src/common/StickyNote.ts
- designer/client/src/components/graph/EspNode/stickyNoteElements.ts
- designer/client/src/components/graph/ProcessGraph.tsx
- designer/client/src/components/graph/fragmentGraph.tsx
- designer/client/src/components/graph/node-modal/node/FragmentContent.tsx
- designer/client/src/components/graph/types.ts
- designer/client/src/components/toolbarSettings/TOOLBAR_COMPONENTS_MAP.ts
- designer/client/src/components/toolbarSettings/defaultToolbarsConfig.ts
- designer/client/src/components/toolbars/creator/ToolBox.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/types/node.ts
- designer/client/src/types/stickyNote.ts
- 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/scenariotoolbar/ToolbarPanelConfig.scala
- designer/server/src/main/scala/pl/touk/nussknacker/ui/db/NuTables.scala
- 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 comments (57)
designer/client/src/components/graph/GraphPartialsInTS/cellUtils.ts (1)
2-3: LGTM! Imports are appropriate for the new functionality.
The imports of StickyNote type and cloneDeep are well-chosen for handling sticky note operations.
designer/client/src/components/stickyNotes/StickyNotesPanel.tsx (2)
1-10: LGTM! Dependencies are well-organized.
The imports are clean, necessary, and follow React best practices.
25-25: Verify pristine state logic.
The tool is disabled when the state is pristine (!pristine). Please verify if this is the intended behavior as it seems counterintuitive - typically, pristine state would indicate that modifications are allowed.
designer/server/src/test/resources/config/common-designer.conf (2)
37-37: LGTM! Panel configuration follows existing pattern.
The sticky notes panel is appropriately placed in the topLeft toolbar section alongside other tool panels.
58-61: Please verify the sticky notes limits and consider additional settings.
The current limits seem quite restrictive:
- Only 5 notes per process might be insufficient for complex scenarios
- Consider adding configurations for:
- Minimum content length
- Markdown/HTML support
- Note categories/colors
Also, there's a trailing comma after maxContentLength which should be removed for consistency with HOCON style.
stickyNotesSettings: {
- maxContentLength: 5000,
+ maxContentLength: 5000
maxNotesCount: 5
}designer/client/src/components/toolbars/creator/ComponentIcon.tsx (1)
10-10: LGTM: Import statement is properly structured
The import follows the established pattern and is correctly positioned with other type imports.
designer/client/src/components/StickyNotePreview.tsx (3)
1-8: LGTM! Well-organized imports.
The imports are properly structured and all are being utilized in the component.
12-17: Great improvement in transform logic!
The transform calculations are now more maintainable with well-named constants and clear logic.
42-61: LGTM! Clean implementation of styles and render logic.
The image styles and component rendering are well-structured, with proper className composition and icon handling.
designer/client/src/components/graph/GraphPartialsInTS/applyCellChanges.ts (6)
10-11: LGTM! Imports are properly structured
The new imports for sticky note functionality are correctly typed and appropriately scoped.
16-16: LGTM! Function signature updated appropriately
The addition of the stickyNotes parameter is well-typed and maintains compatibility with existing parameters.
23-24: LGTM! Well-structured model creation
Good separation of concerns between models and their tools, with clear and descriptive variable names.
30-30: Verify rendering order of different cell types
While the array combination is correct, consider if the order of elements (nodes, edges, sticky notes) affects the rendering hierarchy in the graph.
35-35: LGTM! Proper filtering of new sticky notes
The filtering logic correctly identifies new sticky notes by checking against existing cell IDs.
46-57: LGTM! Error handling implemented as suggested
The implementation follows the previous review recommendations:
- Uses
forEachinstead ofmap - Includes proper error handling for view finding
- Adds appropriate error logging
designer/client/src/components/ComponentPreview.tsx (2)
10-11: LGTM! Imports are well-organized.
The new imports for sticky note functionality are properly placed and follow the existing import style.
80-82:
Fix missing required prop and improve type safety
- The
nodeprop is missing in theStickyNotePreviewcomponent call - Optional chaining on
node?.typeis unnecessary sincenodeis a required prop
Apply this fix:
- return node?.type === StickyNoteType ? (
- <StickyNotePreview isActive={isActive} isOver={isOver} />
+ return node.type === StickyNoteType ? (
+ <StickyNotePreview node={node} isActive={isActive} isOver={isOver} />Additionally, the previous review comment about improving code structure and type safety is still applicable to this section.
designer/client/src/components/graph/EspNode/stickyNote.ts (2)
42-45: Consider adding default attributes to body markup.
The body markup is very minimal compared to border and icon. Consider adding default attributes like fill, stroke, or opacity to ensure consistent rendering.
108-115: LGTM! Clean implementation of shape creation.
The prototype properties setup and shape export are well-implemented with proper type safety.
designer/client/src/components/graph/utils/graphUtils.ts (1)
130-133: LGTM! Clean implementation of configurable event prevention.
The changes provide a flexible way to control event prevention while maintaining backward compatibility through the default value. The TypeScript typing is properly maintained.
designer/server/src/main/scala/pl/touk/nussknacker/ui/config/FeatureTogglesConfig.scala (3)
10-10: LGTM: Import added correctly
The import for StickyNotesSettings is appropriately placed with other DTO imports.
32-33: Verify default value handling
The stickyNotesSettings field is added as a required parameter. Consider whether it should be optional (Option[StickyNotesSettings]) to maintain backward compatibility with existing configurations.
82-82: LGTM: Constructor parameter added correctly
The stickyNotesSettings parameter is correctly added to the constructor call, maintaining the established parameter ordering pattern.
designer/client/src/actions/nk/process.ts (3)
9-11: LGTM: Imports are properly structured.
The new imports are correctly organized and necessary for the sticky notes functionality.
23-23: LGTM: Sticky notes are properly initialized.
The addition ensures sticky notes are loaded when displaying a process, maintaining data consistency.
71-105: Consider potential race conditions in concurrent updates.
The stickyNoteUpdated and stickyNoteAdded functions fetch the latest notes after their respective operations. This could lead to race conditions if multiple updates occur simultaneously. Consider:
- Using optimistic updates for better UX
- Adding version/timestamp checks
- Implementing proper error recovery for failed operations
designer/server/src/main/scala/pl/touk/nussknacker/ui/api/description/StickyNotesApiEndpoints.scala (1)
164-220: LGTM! Well-structured error handling
The Examples object provides comprehensive error cases with clear descriptions and appropriate HTTP status codes. The implementation follows best practices for API error handling.
designer/client/src/components/graph/graphStyledWrapper.ts (2)
179-184: LGTM: Basic container styles look good
The basic container styles for the markdown component are clean and straightforward. The padding provides good spacing for content readability.
200-203: LGTM: Content container styles are appropriate
The full-width and full-height styles are correct for the content container.
designer/server/src/main/resources/defaultDesignerConfig.conf (2)
124-124: LGTM! Visibility rules properly implemented.
The sticky notes panel configuration follows the established pattern and includes appropriate visibility rules (hidden: { archived: true }), matching other panels like the creator-panel.
214-217: Verify the sticky notes limits.
Please confirm that these limits are appropriate for your use case:
maxContentLength: 5000characters per notemaxNotesCount: 5notes total
Consider:
- Are these limits sufficient for typical user needs?
- Have these values been tested with real usage patterns?
- Should these limits be configurable via environment variables like other settings?
designer/client/src/reducers/graph/utils.ts (4)
8-9: LGTM!
The new imports are appropriately placed and necessary for the sticky notes functionality.
85-96: LGTM!
The function correctly handles sticky note removal and layout updates, incorporating previous feedback.
98-110: Refer to existing comment about validation improvements
The previous review comment about adding validation and handling edge cases is still applicable.
126-135: Refer to existing comment about potential data loss
The previous review comment about fixing potential data loss in sticky notes state update is still applicable.
designer/server/src/test/scala/pl/touk/nussknacker/test/utils/domain/ScenarioHelper.scala (3)
13-13: LGTM: Clean import organization and consistent repository initialization
The new imports and repository initialization follow the existing patterns in the codebase and properly handle dependencies.
Also applies to: 19-23, 26-26, 48-48
222-238: Verify scenario name consistency
The implementation looks good but consider adding validation to ensure that newScenario.name matches the provided scenarioName to prevent accidental mismatches.
240-258: LGTM: Well-structured sticky note implementation
The implementation properly handles the sticky note creation in a transaction and correctly manages all required fields from the request.
designer/client/src/reducers/graph/reducer.ts (3)
12-12: LGTM! Well-structured imports
The new sticky note utility imports follow the existing pattern and maintain good separation of concerns.
Also applies to: 17-18
249-254: LGTM! Clean Redux implementation
The STICKY_NOTES_UPDATED case follows Redux best practices with immutable state updates and proper utility function usage.
255-260: Referring to existing review comment
The previous review comment about enhancing consistency and validation for the STICKY_NOTE_DELETED case is still applicable.
designer/server/src/main/scala/pl/touk/nussknacker/ui/server/AkkaHttpBasedRouteProvider.scala (4)
72-72: LGTM: Import statement is correctly placed.
The import for DbStickyNotesRepository is appropriately placed with other repository imports.
171-171: LGTM: Repository initialization follows existing patterns.
The sticky notes repository is initialized consistently with other repositories, using the same pattern of passing dbRef and designerClock.
402-409: Verify error handling in the service configuration.
The service initialization looks correct, but since this is handling user data:
- Ensure that
stickyNotesSettingsincludes proper validation rules - Verify that
dbioRunnertransactions are properly handled for concurrent updates
603-603: LGTM: Service registration is properly integrated.
The sticky notes service is correctly added to the nuDesignerApi alongside other services.
designer/client/src/http/HttpService.ts (1)
695-746: LGTM! Well-structured HTTP service methods.
The sticky note methods are well-implemented with:
- Consistent error handling using the private
#addErrormethod - Proper TypeScript type definitions and return types
- Clear and consistent API endpoint structure
- Proper parameter encoding using
encodeURIComponent
designer/server/src/test/scala/pl/touk/nussknacker/ui/api/ProcessesResourcesSpec.scala (2)
55-55: LGTM: Import added correctly
The StickyNotesPanel import is properly placed alongside other toolbar panel type imports.
1229-1230: LGTM: StickyNotesPanel integration looks good
The StickyNotesPanel is correctly integrated into the toolbar configuration test case, maintaining consistency with other panel configurations.
designer/server/src/main/scala/pl/touk/nussknacker/ui/process/repository/stickynotes/StickyNotesRepository.scala (4)
21-25: findStickyNotes method is well-defined
The method correctly retrieves sticky notes for a given scenario and version.
36-45: Previous suggestion on improving return type for updateStickyNote still applies
The earlier recommendation to enhance type safety and error handling for the updateStickyNote method is still relevant.
46-47: findStickyNoteById method is correctly implemented
The method appropriately retrieves a sticky note by its ID, returning an option type for null safety.
48-48: Previous suggestion on enhancing delete operation type safety still applies
The prior advice to improve the return type for deleteStickyNote to enhance type safety remains applicable.
designer/server/src/main/scala/pl/touk/nussknacker/ui/db/entity/StickyNotesEntityFactory.scala (4)
66-68: Handle potential exceptions when mapping StickyNoteEvent
Using StickyNoteEvent.withName can throw a NoSuchElementException if the provided string does not match any known event names. To prevent runtime errors, consider handling this case more gracefully.
Apply this diff to safely map strings to StickyNoteEvent:
implicit def stickyNoteEventColumnTyped: BaseColumnType[StickyNoteEvent] =
- MappedColumnType.base[StickyNoteEvent, String](_.toString, StickyNoteEvent.withName)
+ MappedColumnType.base[StickyNoteEvent, String](
+ _.toString,
+ str => StickyNoteEvent.values.find(_.toString == str)
+ .getOrElse(throw new IllegalArgumentException(s"Unknown StickyNoteEvent: $str"))
+ )77-80: Improve error handling for JSON parsing in layoutDataColumnTyped
Throwing exceptions directly upon parsing errors can lead to unexpected crashes. It's better to wrap the error in a domain-specific exception to provide more context.
Apply this diff to enhance error handling:
parser.parse(jsonStr).flatMap(Decoder[LayoutData].decodeJson) match {
case Right(layoutData) => layoutData
- case Left(error) => throw error
+ case Left(error) => throw new InvalidLayoutDataException(s"Invalid LayoutData JSON: $jsonStr", error)
}Also, ensure that InvalidLayoutDataException is defined appropriately in your codebase.
86-89: Improve error handling for JSON parsing in dimensionsColumnTyped
Similar to the previous comment, enhance the error handling when parsing Dimensions JSON data to avoid unhandled exceptions.
Apply this diff:
parser.parse(jsonStr).flatMap(Decoder[Dimensions].decodeJson) match {
case Right(dimensions) => dimensions
- case Left(error) => throw error
+ case Left(error) => throw new InvalidDimensionsException(s"Invalid Dimensions JSON: $jsonStr", error)
}Ensure that InvalidDimensionsException is defined to provide meaningful context when an error occurs.
110-112: Verify that toStickyNote includes all necessary fields
The toStickyNote method does not include noteCorrelationId, eventType, scenarioId, or scenarioVersionId in the StickyNote object. Please verify that this omission is intentional and that these fields are not required in the StickyNote domain model.
designer/server/src/main/scala/pl/touk/nussknacker/ui/process/repository/stickynotes/DbStickyNotesRepository.scala (1)
50-59: Verify the necessity of the user parameter in findStickyNoteById
The implicit user: LoggedUser parameter in findStickyNoteById is not used within the method. If it's not required, consider removing it to simplify the method signature.
designer/client/src/components/graph/GraphPartialsInTS/cellUtils.ts
Outdated
Show resolved
Hide resolved
|
@coderabbitai can you close comments which you think have been resolved by my recent changes and do a review one more time? |
|
@philemone: I've closed the comments that have been resolved by your recent changes and will review the updated code. ✅ Actions performedComments resolved. Review triggered.
|
There was a problem hiding this comment.
Actionable comments posted: 5
🧹 Outside diff range and nitpick comments (16)
designer/client/src/components/graph/StickyNoteElement.ts (2)
4-4: Remove redundant type alias import.The
MarkupNodeJSONtype alias is redundant sincedia.MarkupNodeJSONis already available. You can usedia.MarkupNodeJSONdirectly.-import MarkupNodeJSON = dia.MarkupNodeJSON;
48-52: Consider debouncing the onChange handler.The current implementation triggers model updates and events on every change. For better performance, especially with rapid typing, consider debouncing the onChange handler.
+import { debounce } from 'lodash'; + export const StickyNoteElementView = dia.ElementView.extend({ events: { - "change textarea": "onChange", + "change textarea": "onChangeDebounced", - "focusout textarea": "onChange", + "focusout textarea": "onChangeDirect", }, + + initialize: function() { + this.onChangeDebounced = debounce(this.onChangeDirect.bind(this), 300); + }, - onChange: function (evt) { + onChangeDirect: function (evt) { this.model.trigger(Events.CELL_CONTENT_UPDATED, this.model, evt.target.value); this.model.attr(`${MARKDOWN_EDITOR_NAME}/props/value`, evt.target.value); this.model.attr(`${MARKDOWN_EDITOR_NAME}/props/disabled`, true); }, });designer/client/src/components/graph/EspNode/stickyNote.ts (1)
90-98: Consider making shadow parameters theme-configurable.The shadow effect parameters are currently hardcoded. Consider making these configurable through the theme to ensure consistency across different visual themes.
filter: { name: "dropShadow", args: { - dx: 1, - dy: 1, - blur: 5, - opacity: 0.4, + dx: theme.shadows?.[1]?.dx ?? 1, + dy: theme.shadows?.[1]?.dy ?? 1, + blur: theme.shadows?.[1]?.blur ?? 5, + opacity: theme.shadows?.[1]?.opacity ?? 0.4, }, },designer/server/src/main/scala/pl/touk/nussknacker/ui/api/description/stickynotes/Dtos.scala (3)
19-36: Consider adding validation for StickyNoteIdWhile the implementation is good, consider adding validation to ensure
StickyNoteIdcannot be negative.final case class StickyNoteId(value: Long) extends AnyVal object StickyNoteId { + def apply(value: Long): Either[IllegalArgumentException, StickyNoteId] = + if (value > 0) Right(new StickyNoteId(value)) + else Left(new IllegalArgumentException("StickyNoteId must be positive")) + implicit val encoder: Encoder[StickyNoteId] = Encoder.encodeLong.contramap(_.value) - implicit val decoder: Decoder[StickyNoteId] = Decoder.decodeLong.map(StickyNoteId(_)) + implicit val decoder: Decoder[StickyNoteId] = Decoder.decodeLong.emap(value => + apply(value).left.map(_.getMessage) + ) }
43-74: Document validation rules and add color format validationConsider the following improvements:
- Add scaladoc comments documenting the validation rules for content length
- Add color format validation (e.g., hex color codes)
@derive(encoder, decoder, schema) +/** Represents a sticky note with the following constraints: + * - content: length must not exceed maxContentLength (see StickyNotesSettings) + * - color: must be a valid hex color code (#RRGGBB) + */ case class StickyNote( noteId: StickyNoteId, content: String, layoutData: LayoutData, color: String, dimensions: Dimensions, targetEdge: Option[String], editedBy: String, editedAt: Instant -) +) { + require(color.matches("^#[0-9A-Fa-f]{6}$"), "Color must be a valid hex color code (#RRGGBB)") +}
76-83: Add validation and documentation for settingsConsider adding validation and documentation for the settings class.
+/** Settings for sticky notes with the following constraints: + * - maxContentLength: maximum allowed length for sticky note content + * - maxNotesCount: maximum number of sticky notes per scenario + */ case class StickyNotesSettings( maxContentLength: Int, maxNotesCount: Int -) +) { + require(maxContentLength > 0, "maxContentLength must be positive") + require(maxNotesCount > 0, "maxNotesCount must be positive") +}designer/client/src/components/graph/EspNode/stickyNoteElements.ts (4)
34-35: Extract magic numbers into named constants.The height calculation uses magic numbers (4) which could be better expressed as named constants for improved maintainability.
+ const ICON_PADDING = 4; foreignObject: { width: stickyNote.dimensions.width, - height: stickyNote.dimensions.height - ICON_SIZE - CONTENT_PADDING * 4, + height: stickyNote.dimensions.height - ICON_SIZE - CONTENT_PADDING * ICON_PADDING, },
53-54: Consider moving constants to a shared location.These minimum dimension constants might be needed elsewhere in the application. Consider moving them to a shared constants file or the
stickyNote.tsfile where other constants are defined.
56-65: Add type safety to the remove button action.Consider adding explicit typing for the action function parameter to improve type safety.
- action: function () { + action: function (this: elementTools.Remove) {
1-129: Consider adding unit tests for the sticky note functionality.This file contains complex logic for sticky note manipulation and rendering. Consider adding comprehensive unit tests to cover:
- Sticky note creation with various dimensions and themes
- Resize tool behavior and constraints
- Remove tool functionality
- Event handling
This will help ensure the reliability of the sticky notes feature.
designer/client/src/components/graph/NodeDescriptionPopover.tsx (2)
123-123: Improve comment clarity and style.The current comment could be more descriptive and follow proper formatting.
- if (isStickyNoteElement(view.model)) return; //Dont use it for stickyNotes + if (isStickyNoteElement(view.model)) return; // Skip description popover for sticky notes
Line range hint
123-128: Consider adding null check for defensive programming.The
view.modelaccess should be protected against potential undefined values.- if (isStickyNoteElement(view.model)) return; + if (!view?.model || isStickyNoteElement(view.model)) return;designer/client/src/components/graph/graphStyledWrapper.ts (1)
179-184: Use theme spacing for consistent paddingConsider using MUI's theme spacing instead of hardcoded pixel values for padding to maintain consistency across the application.
".sticky-note-markdown": { width: "100%", height: "100%", - paddingLeft: "10px", - paddingRight: "10px", + paddingLeft: theme.spacing(1), + paddingRight: theme.spacing(1), },designer/client/package.json (1)
24-24: Consider consolidating XSS prevention librariesThe project already includes
xss@1.0.10in devDependencies. Consider either:
- Moving the existing
xsspackage to production dependencies if it meets your needs, or- Removing the
xsspackage if DOMPurify will be the standard solution for XSS preventionThis would help maintain consistency in the codebase and reduce bundle size.
designer/client/src/components/graph/Graph.tsx (2)
372-379: Add error handling for tool operationsConsider adding try-catch blocks around tool operations to gracefully handle potential errors.
const showStickyNoteTools = (cellView: dia.CellView) => { + try { cellView.showTools(); + } catch (error) { + console.error('Failed to show sticky note tools:', error); + } }; const hideToolsOnBlankClick = (evt: dia.Event) => { evt.preventDefault(); + try { this.processGraphPaper.hideTools(); + } catch (error) { + console.error('Failed to hide tools:', error); + } };
485-493: Optimize content update handlerThe content equality check could be moved before the sticky note copy operation to improve performance.
this.graph.on(Events.CELL_CONTENT_UPDATED, (cell: dia.Element, content: string) => { if (isStickyNoteElement(cell)) { + const currentContent = cell.get("content"); + if (currentContent === content) return; const updatedStickyNote = getStickyNoteCopyFromCell(this.props.stickyNotes, cell); if (!updatedStickyNote) return; - if (updatedStickyNote.content == content) return; updatedStickyNote.content = content; this.updateStickyNote(this.props.scenario.name, this.props.scenario.processVersionId, updatedStickyNote); } });
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
⛔ Files ignored due to path filters (1)
designer/client/package-lock.jsonis excluded by!**/package-lock.json
📒 Files selected for processing (14)
designer/client/package.json(1 hunks)designer/client/src/actions/notificationActions.tsx(1 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/cellUtils.ts(2 hunks)designer/client/src/components/graph/NodeDescriptionPopover.tsx(2 hunks)designer/client/src/components/graph/StickyNoteElement.ts(1 hunks)designer/client/src/components/graph/graphStyledWrapper.ts(1 hunks)designer/client/src/components/stickyNotes/StickyNotesPanel.tsx(1 hunks)designer/client/src/components/toolbars/creator/ComponentIcon.tsx(2 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/config/FeatureTogglesConfig.scala(4 hunks)
🚧 Files skipped from review as they are similar to previous changes (6)
- designer/client/src/actions/notificationActions.tsx
- designer/client/src/components/StickyNotePreview.tsx
- designer/client/src/components/graph/GraphPartialsInTS/cellUtils.ts
- designer/client/src/components/stickyNotes/StickyNotesPanel.tsx
- designer/client/src/components/toolbars/creator/ComponentIcon.tsx
- designer/server/src/main/scala/pl/touk/nussknacker/ui/config/FeatureTogglesConfig.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 (16)
designer/client/src/components/graph/StickyNoteElement.ts (2)
17-18: LGTM! Clean element definition.
The element definition is well-structured and properly typed using the defined interfaces.
36-37: LGTM! Previous review comments addressed.
The code now correctly:
- Uses proper DOM API with
querySelector - Supports both Ctrl+A and Cmd+A for text selection
Also applies to: 41-42
designer/client/src/components/graph/EspNode/stickyNote.ts (5)
1-17: LGTM! Well-structured imports and constants.
The imports are comprehensive and include security measures (DOMPurify). Constants are well-named and provide clear configuration for sticky note dimensions and styling.
18-46: LGTM with existing href issue noted.
The markup definitions for border, icon, and body are well-structured. Note that there's an existing comment about adding the missing href attribute for the icon.
48-55: LGTM! Secure and user-friendly markdown rendering.
The renderer implementation properly handles security concerns for links and provides a good fallback for images in SVG context.
57-74: LGTM! Robust error handling and content sanitization.
The implementation properly handles markdown parsing errors and sanitizes content using DOMPurify, addressing previous security concerns.
115-122: LGTM! Clean shape implementation.
The proto props and shape export follow JointJS patterns correctly and combine the components effectively.
designer/server/src/main/scala/pl/touk/nussknacker/ui/api/description/stickynotes/Dtos.scala (2)
1-15: LGTM! Well-organized imports and package structure.
The imports are clean, relevant, and properly organized.
89-115: LGTM! Well-structured error handling
The error hierarchy and messages are well-defined and comprehensive.
designer/client/src/components/graph/EspNode/stickyNoteElements.ts (3)
1-14: LGTM! Clean imports and clear type definition.
The imports are well-organized and the ModelWithTool type clearly defines the structure needed for the sticky note implementation.
67-111: LGTM with previous comments.
The resize tool implementation looks functional, but please address the scope issues mentioned in the previous review comments.
126-126: Add content validation before setting the value.
Consider adding validation for the content value before setting it to prevent potential issues with invalid or malformed content.
- stickyNoteModel.attr(`${MARKDOWN_EDITOR_NAME}/props/value`, stickyNote.content);
+ const sanitizedContent = stickyNote.content ?? '';
+ stickyNoteModel.attr(`${MARKDOWN_EDITOR_NAME}/props/value`, sanitizedContent);designer/client/src/components/graph/NodeDescriptionPopover.tsx (1)
8-8: LGTM! Import statement is properly placed.
The import of isStickyNoteElement is correctly added and follows the existing import style.
designer/client/package.json (1)
24-24: Add @types/dompurify for TypeScript support
The addition of DOMPurify is appropriate for sanitizing HTML content in sticky notes. However, to ensure proper TypeScript support, please add the corresponding type definitions:
"dependencies": {
"dompurify": "3.2.0",
+ "@types/dompurify": "3.0.5",
}designer/client/src/components/graph/Graph.tsx (2)
49-50: LGTM: Clean imports and namespace setup
The imports and namespace setup for sticky notes are well-organized and follow best practices.
Also applies to: 70-76
524-547: LGTM: Well-structured CRUD operations
The sticky note CRUD operations are well-implemented with proper permission checks. As confirmed by the retrieved learning, frontend validation for content length is correctly omitted since it's handled by the backend.
|
|
||
| sealed trait StickyNotesError | ||
|
|
||
| implicit lazy val cellErrorSchema: Schema[LayoutData] = Schema.derived |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Fix incorrect schema name
The variable name cellErrorSchema seems incorrect for a LayoutData schema.
- implicit lazy val cellErrorSchema: Schema[LayoutData] = Schema.derived
+ implicit lazy val layoutDataSchema: Schema[LayoutData] = Schema.derived📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| implicit lazy val cellErrorSchema: Schema[LayoutData] = Schema.derived | |
| implicit lazy val layoutDataSchema: Schema[LayoutData] = Schema.derived |
| @derive(encoder, decoder, schema) | ||
| case class Dimensions( | ||
| width: Long, | ||
| height: Long | ||
| ) |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Add validation for Dimensions
The Dimensions case class should ensure positive values for width and height.
@derive(encoder, decoder, schema)
case class Dimensions(
width: Long,
height: Long
-)
+) {
+ require(width > 0 && height > 0, "Dimensions must be positive")
+}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| @derive(encoder, decoder, schema) | |
| case class Dimensions( | |
| width: Long, | |
| height: Long | |
| ) | |
| @derive(encoder, decoder, schema) | |
| case class Dimensions( | |
| width: Long, | |
| height: Long | |
| ) { | |
| require(width > 0 && height > 0, "Dimensions must be positive") | |
| } |
| ".sticky-note-markdown-editor": { | ||
| paddingLeft: "10px", | ||
| paddingRight: "10px", | ||
| backgroundColor: "rgba(0,0,0,0.1)", | ||
| resize: "none", | ||
| width: "100%", | ||
| height: "100%", | ||
| borderStyle: "none", | ||
| borderColor: "Transparent", | ||
| whiteSpace: "pre-line", | ||
| overflow: "hidden", | ||
| }, |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Enhance editor styling and consistency
Several improvements can be made to the editor styling:
- Use theme spacing for padding
- Use theme colors with alpha instead of direct rgba
- Add text color from theme
- Add font properties for consistency
".sticky-note-markdown-editor": {
- paddingLeft: "10px",
- paddingRight: "10px",
- backgroundColor: "rgba(0,0,0,0.1)",
+ paddingLeft: theme.spacing(1),
+ paddingRight: theme.spacing(1),
+ backgroundColor: alpha(theme.palette.common.black, 0.1),
+ color: theme.palette.text.primary,
+ fontFamily: theme.typography.fontFamily,
+ fontSize: theme.typography.body1.fontSize,
resize: "none",
width: "100%",
height: "100%",
borderStyle: "none",
borderColor: "Transparent",
whiteSpace: "pre-line",
overflow: "hidden",
},Don't forget to import the alpha utility:
import { alpha } from '@mui/material/styles';| // TODO move max width and height to some config? | ||
| const width = Math.max(100, Math.min(3000, Math.round(size.width))); | ||
| const height = Math.max(100, Math.min(3000, Math.round(size.height))); |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Move hardcoded values to configuration
The TODO comment suggests moving width/height constraints to config. This should be prioritized for better maintainability.
Consider creating a configuration object:
const STICKY_NOTE_CONSTRAINTS = {
MIN_WIDTH: 100,
MAX_WIDTH: 3000,
MIN_HEIGHT: 100,
MAX_HEIGHT: 3000,
} as const;
Describe your changes
This is just a draft with stickyNotes functionality added
Checklist before merge
Summary by CodeRabbit
Release Notes
New Features
StickyNotesPanelto the toolbar for easy access to sticky note features.Enhancements
Documentation