-
Notifications
You must be signed in to change notification settings - Fork 94
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Sticky notes #7277
base: staging
Are you sure you want to change the base?
Sticky notes #7277
Conversation
📝 Walkthrough📝 WalkthroughWalkthroughThe pull request introduces a comprehensive set of changes focused on enhancing the sticky notes functionality within the Nussknacker application. Key modifications include the addition of new action types, components, and API endpoints for managing sticky notes. In the client-side code, new types and interfaces are defined to represent sticky notes, and various components are updated to support their display and interaction. The Possibly related PRs
Suggested labels
Suggested reviewers
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actionable comments posted: 22
🧹 Outside diff range and nitpick comments (67)
docs/Changelog.md (1)
13-16
: LGTM with a minor suggestion.The changelog entry for the StickyNotes feature is well-structured and informative. Consider adding a brief example of the
stickyNotesSettings
configuration parameters (likemaxContentLength
andmaxNotesCount
) to make it more helpful for users.* [#7181](https://github.com/TouK/nussknacker/pull/7181) StickyNotes feature * sticky notes are designed to store information inside scenario/fragment, they are separate from graph nodes and do not take part in scenario logic * new API available under `processes/{scenarioName}/stickyNotes` - * configuration `stickyNotesSettings` allowing to hide/show stickyNotes, set sticky notes max content length or its max number on a graph + * configuration `stickyNotesSettings` allowing to: + * hide/show stickyNotes + * set max content length via `maxContentLength` + * set max number of notes on a graph via `maxNotesCount`designer/server/src/test/scala/pl/touk/nussknacker/test/utils/domain/ScenarioHelper.scala (2)
85-97
: Consider adding documentation for version handlingWhile the methods are well-structured, the
updateScenario
method would benefit from documentation explaining its version handling behavior, particularly regarding theincreaseVersionWhenJsonNotChanged
parameter used in the implementation.Consider adding ScalaDoc:
/** * Updates the scenario with new content. * @param scenarioName name of the scenario to update * @param newScenario new scenario content * @return ProcessUpdated containing information about the update including version changes */ def updateScenario(
222-258
: Consider improving error handling for scenario lookupThe
.get
operation on the Option returned byfetchProcessId
could be unsafe if the scenario doesn't exist. Consider using a more explicit error handling approach.Consider this pattern:
- scenarioId <- futureFetchingScenarioRepository.fetchProcessId(scenarioName).map(_.get) + scenarioId <- futureFetchingScenarioRepository.fetchProcessId(scenarioName).flatMap { + case Some(id) => Future.successful(id) + case None => Future.failed(new NoSuchElementException(s"Scenario not found: $scenarioName")) + }docs/MigrationGuide.md (3)
7-14
: Consider adding version release datesThe version headers would be more helpful if they included release dates. This helps users understand the timeline of changes.
-## In version 1.19.0 (Not released yet) +## In version 1.19.0 (Not released yet, last updated: December 2024)
Line range hint
15-24
: Consider grouping related changesThe "Other changes" section contains multiple Kafka-related changes. Consider grouping them under a "Kafka Changes" subsection for better organization.
Line range hint
1-3754
: Consider adding a table of contentsFor better navigation in such a long document, consider adding a table of contents at the beginning that links to each version section.
Example addition at the top of the file:
# Table of Contents - [Version 1.19.0](#in-version-1190-not-released-yet) - [Version 1.18.0](#in-version-1180) - [Version 1.17.0](#in-version-1170) ...designer/server/src/main/scala/pl/touk/nussknacker/ui/api/SettingsResources.scala (1)
Test coverage needs improvement for sticky notes settings
The current test coverage for sticky notes settings is insufficient:
SettingsResourcesSpec
doesn't verify the sticky notes settings in the response- Test configuration in
common-designer.conf
defines sticky notes settings but they aren't validatedStickyNotesApiHttpServiceBusinessSpec
only tests the API endpoints but not the settings integrationRequired test improvements:
- Add assertions in
SettingsResourcesSpec
to verify sticky notes settings are correctly included in the response- Verify the settings match the configuration values (maxContentLength: 5000, maxNotesCount: 5, enabled: true)
- Add test cases for disabled sticky notes feature
🔗 Analysis chain
Line range hint
1-170
: Verify test coverage for sticky notes settings.While the code changes look good, please ensure comprehensive test coverage for the sticky notes settings integration:
- Unit tests for serialization/deserialization of
ToggleFeaturesOptions
with sticky notes- Integration tests for the
/settings
endpoint response🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Check for test coverage of sticky notes settings echo "Checking for settings-related test files..." fd -e scala "Settings.*Test|Settings.*Spec" echo "Checking for sticky notes test coverage..." rg "stickyNotesSettings" --type scala -g "*Test*.scala" -g "*Spec*.scala"Length of output: 510
Script:
#!/bin/bash # Let's examine the content of the test file to understand the coverage echo "Checking SettingsResourcesSpec content..." cat designer/server/src/test/scala/pl/touk/nussknacker/ui/api/SettingsResourcesSpec.scala echo -e "\nChecking for any sticky notes related tests in the entire codebase..." rg -l "StickyNotes.*Test|StickyNotes.*Spec" --type scalaLength of output: 3328
Script:
#!/bin/bash # Let's check the content of the sticky notes specific test file echo "Checking StickyNotesApiHttpServiceBusinessSpec content..." cat designer/server/src/test/scala/pl/touk/nussknacker/ui/api/StickyNotesApiHttpServiceBusinessSpec.scala # Let's also check if there are any test configurations for sticky notes settings echo -e "\nChecking test configurations..." rg "stickyNotesSettings" designer/server/src/test/resources/Length of output: 3531
Script:
#!/bin/bash # Let's check the test configuration content echo "Checking test configuration content..." cat designer/server/src/test/resources/config/common-designer.conf # Let's also check if there are any assertions related to sticky notes in SettingsResourcesSpec echo -e "\nChecking for sticky notes assertions in settings tests..." rg -A 10 -B 10 "responseAs\[UISettings\]" designer/server/src/test/scala/pl/touk/nussknacker/ui/api/SettingsResourcesSpec.scalaLength of output: 2984
designer/client/src/components/graph/GraphPartialsInTS/applyCellChanges.ts (2)
35-35
: Optimize ID lookup by using aSet
forcurrentIds
The use of
Array.includes
in filters can become inefficient with large arrays due to linear search time. ConvertcurrentIds
to aSet
to improve lookup performance when checking for the existence of IDs.Apply this diff to optimize the lookup:
-const currentIds = currentCells.map((c) => c.id); +const currentIds = new Set(currentCells.map((c) => c.id));Then, update the usage in filters:
-const newCells = cells.filter((cell) => !currentIds.includes(cell.id)); +const newCells = cells.filter((cell) => !currentIds.has(cell.id)); -const newStickyNotesModelsWithTools = stickyNotesModelsWithTools.filter((s) => !currentIds.includes(s.model.id)); +const newStickyNotesModelsWithTools = stickyNotesModelsWithTools.filter((s) => !currentIds.has(s.model.id));
46-57
: Handle errors more gracefully when adding tools to sticky note viewsWhile the try-catch block captures errors during the addition of tools to sticky note views, consider providing more detailed error information or user feedback. This can aid in debugging and improve user experience.
For example, you could include the sticky note ID in the error message:
console.error(`Failed to add tools to stickyNote view (ID: ${m.model.id}):`, error);designer/server/src/main/scala/db/migration/V1_060__CreateStickyNotesDefinition.scala (1)
23-26
: Add index to improve query performance on foreign keysConsider adding indexes on the foreign key columns
scenario_id
andscenario_version_id
in thesticky_notes
table to improve query performance, especially for join operations.You can modify the migration to include the index creation:
_ <- definitions.stickyNotesEntityTable.schema.create _ <- sqlu""" CREATE INDEX idx_sticky_notes_scenario ON "sticky_notes" ("scenario_id", "scenario_version_id"); """ _ <- sqlu""" ALTER TABLE "sticky_notes" ADD CONSTRAINT "sticky_notes_scenario_version_fk" FOREIGN KEY ("scenario_id", "scenario_version_id") REFERENCES "process_versions" ("process_id", "id") ON DELETE CASCADE; """designer/client/src/components/graph/EspNode/stickyNote.ts (1)
55-62
: Optimize custom Markdown renderer for better maintainabilityThe custom renderer overrides for links and images are necessary but might benefit from additional comments or using more secure default options provided by
marked
.Consider adding comments to explain the customization rationale or exploring configuration options that might simplify the renderer customization.
designer/client/src/components/graph/EspNode/stickyNoteElements.ts (2)
105-112
: Avoid hardcoded constants for size constraintsThe size constraints in the
ResizeTool
are hardcoded. Consider referencing the constants defined inSTICKY_NOTE_CONSTRAINTS
to ensure consistency across the codebase.Modify the code to use the constants:
Math.max( Math.min(STICKY_NOTE_CONSTRAINTS.MAX_WIDTH, Math.round(coordinates.x - 10)), STICKY_NOTE_CONSTRAINTS.MIN_WIDTH, ), Math.max( Math.min(STICKY_NOTE_CONSTRAINTS.MAX_HEIGHT, Math.round(coordinates.y - 10)), STICKY_NOTE_CONSTRAINTS.MIN_HEIGHT, ),Ensure that
-10
offset is correctly handled or replaced with a defined constant if necessary.
133-133
: Update sticky note content upon resizingAfter resizing the sticky note, the content's layout may be affected. Consider triggering a re-render or updating the content display to adjust to the new dimensions.
Ensure that any UI components within the sticky note adjust their size or layout appropriately after a resize event.
designer/server/src/main/scala/pl/touk/nussknacker/ui/process/repository/stickynotes/DbStickyNotesRepository.scala (2)
104-110
: Refine Exception Messages for ClarityThe exception messages starting with "This is odd..." may not provide clear information for debugging purposes. It's advisable to use more informative and professional messages to aid in troubleshooting.
Apply this diff to improve the exception messages:
case 0 => DBIOAction.failed(new IllegalStateException(s"This is odd, no sticky note was added")) case 1 => DBIOAction.successful(entity.noteCorrelationId) case n => - DBIOAction.failed( - new IllegalStateException(s"This is odd, more than one sticky note were added (added $n records).") - ) + DBIOAction.failed( + new IllegalStateException(s"Unexpected number of records added ($n) when adding a sticky note.") + )
25-29
: Organize Imports for ReadabilityThe inheritance chain in the class definition is lengthy and can reduce readability. Consider placing each trait on a new line for better clarity.
Apply this diff:
class DbStickyNotesRepository private (override protected val dbRef: DbRef, override val clock: Clock)( implicit executionContext: ExecutionContext - ) extends DbioRepository - with NuTables - with StickyNotesRepository - with LazyLogging { + ) extends DbioRepository with NuTables with StickyNotesRepository with LazyLogging {Or alternatively:
class DbStickyNotesRepository private ( override protected val dbRef: DbRef, override val clock: Clock )(implicit executionContext: ExecutionContext) extends DbioRepository with NuTables with StickyNotesRepository with LazyLogging {designer/server/src/main/scala/pl/touk/nussknacker/ui/api/description/StickyNotesApiEndpoints.scala (2)
93-115
: Standardize Error Responses instickyNotesUpdateEndpoint
In the
stickyNotesUpdateEndpoint
, consider adding thestickyNoteCountLimitReachedExample
to the error outputs for consistency with other endpoints. This ensures that all relevant error scenarios are covered.Apply this diff:
.errorOut( oneOf[StickyNotesError]( noScenarioExample, noStickyNoteExample, stickyNoteContentTooLongExample + stickyNoteCountLimitReachedExample ) )
163-216
: Avoid Redundant Definitions in Examples ObjectThe
Examples
object contains repetitive code for defining variants of error responses. Consider abstracting the common patterns to reduce duplication and enhance maintainability.Refactor the code to use a helper method for creating variants:
private def createVariant[T](statusCode: StatusCode, example: Example[T]): EndpointOutput.OneOfVariant[T] = { oneOfVariantFromMatchType( statusCode, plainBody[T].example(example) ) } val noScenarioExample: EndpointOutput.OneOfVariant[NoScenario] = createVariant(NotFound, Example.of(...)) val noStickyNoteExample: EndpointOutput.OneOfVariant[NoStickyNote] = createVariant(NotFound, Example.of(...)) // And so on for other examples...designer/server/src/main/scala/pl/touk/nussknacker/ui/api/StickyNotesApiHttpService.scala (2)
66-71
: Validate Sticky Note Content Before Count CheckIn the
stickyNotesAddEndpoint
, the validation for sticky note content occurs after checking the sticky notes count. If the content is invalid, there's no need to check the count. Consider reordering the validations for efficiency.Swap the order of validations:
for { scenarioId <- getScenarioIdByName(scenarioName) _ <- isAuthorized(scenarioId, Permission.Write) + _ <- validateStickyNoteContent(requestBody.content, stickyNotesSettings) count <- getStickyNotesCount(scenarioId, requestBody.scenarioVersionId) _ <- validateStickyNotesCount(count, stickyNotesSettings) - _ <- validateStickyNoteContent(requestBody.content, stickyNotesSettings) processActivity <- addStickyNote(scenarioId, requestBody) } yield processActivity
205-228
: Handle Unauthorized Updates GracefullyIn the
updateStickyNote
method, if a user lacks permission to update a sticky note, the error returned isNoPermission
. Ensure that this is communicated clearly to the user, and consider logging such attempts for security auditing.Implement logging for unauthorized access attempts:
private def isAuthorized(scenarioId: ProcessId, permission: Permission)( implicit loggedUser: LoggedUser ): EitherT[Future, StickyNotesError, Unit] = { val isAuthorized = scenarioAuthorizer.check(scenarioId, permission, loggedUser) if (!isAuthorized) { logger.warn(s"User '${loggedUser.id}' attempted unauthorized access to scenario '${scenarioId.value}'.") } EitherT( isAuthorized.map[Either[StickyNotesError, Unit]] { case true => Right(()) case false => Left(NoPermission) } ) }designer/client/src/components/graph/Graph.tsx (4)
234-242
: Avoid Redundant Null ChecksIn the
CELL_MOVED
event handler, the code checks ifupdatedStickyNote
is falsy before proceeding. SincegetStickyNoteCopyFromCell
should return an object orundefined
, consider simplifying the null checks for readability.Simplify the null checks:
if (updatedStickyNote) { const position = cell.model.get("position"); updatedStickyNote.layoutData = { x: position.x, y: position.y }; this.updateStickyNote(this.props.scenario.name, this.props.scenario.processVersionId, updatedStickyNote); }
375-379
: Prevent Default Behavior on Blank ClickIn the
hideToolsOnBlankClick
function, you callevt.preventDefault();
, but this might not be necessary and could interfere with default behaviors like deselecting text. Consider removing it unless it's explicitly needed.Review if
preventDefault
is necessary, and remove it if not required.
470-499
: Consolidate Event Handling for Sticky NotesMultiple event handlers for sticky notes (
CELL_RESIZED
,CELL_CONTENT_UPDATED
) perform similar update operations. Consider abstracting the common logic into a separate method to adhere to the DRY (Don't Repeat Yourself) principle.Create a helper function to handle sticky note updates:
private handleStickyNoteUpdate(cell: dia.Element, updates: Partial<StickyNote>): void { const updatedStickyNote = getStickyNoteCopyFromCell(this.props.stickyNotes, cell); if (!updatedStickyNote) return; Object.assign(updatedStickyNote, updates); this.updateStickyNote(this.props.scenario.name, this.props.scenario.processVersionId, updatedStickyNote); }Then use this function in your event handlers.
541-545
: Remove Unnecessary Fragment ChecksThe methods
updateStickyNote
anddeleteStickyNote
check ifthis.props.isFragment === true
before proceeding. Since sticky notes are unlikely to be edited in fragments, consider removing these checks unless they serve a specific purpose.Review the necessity of these fragment checks and remove them if they are redundant.
designer/client/src/types/stickyNote.ts (1)
2-4
: Add Type Annotation forcreateStickyNoteId
FunctionThe function
createStickyNoteId
lacks a return type annotation. Adding it enhances code clarity and helps with TypeScript's type checking.Update the function signature:
export function createStickyNoteId(noteId: number): string { return StickyNoteType + "_" + noteId; }designer/client/src/common/StickyNote.ts (2)
3-3
: Add JSDoc documentation for the Dimensions type.Adding documentation will improve code maintainability and help other developers understand the purpose and constraints of this type.
+/** + * 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 };
5-15
: Enhance type safety and add documentation for the StickyNote interface.Consider the following improvements:
- Add JSDoc documentation for the interface and its properties
- Consider using more specific types for validation
+/** + * Represents a sticky note in the system. + */ export interface StickyNote { + /** Unique identifier for the sticky note */ id?: string; + /** Sequential identifier for the note */ noteId: number; + /** Content of the sticky note */ content: string; layoutData: LayoutData; dimensions: Dimensions; + /** Color of the sticky note in hex or named format */ color: string; + /** Optional edge that this note is targeting */ targetEdge?: string; + /** Username of the last editor */ editedBy: string; + /** ISO timestamp of the last edit */ editedAt: string; }Consider adding validation types:
// Add at the top of the file export const MAX_CONTENT_LENGTH = 1000; export const VALID_COLORS = ['#ffeb3b', '#fff176', '#fff59d'] as const; export interface StickyNote { // ... other fields ... content: string & { length: number extends infer L ? L extends 0..typeof MAX_CONTENT_LENGTH ? L : never : never }; color: typeof VALID_COLORS[number]; // ... other fields ... }designer/server/src/main/scala/pl/touk/nussknacker/ui/db/NuTables.scala (1)
14-15
: Consider documenting trait dependencies and ordering.The addition of
StickyNotesEntityFactory
follows the existing pattern. However, it would be helpful to:
- Document any initialization dependencies between traits
- Consider grouping related traits together (e.g., all note-related traits)
designer/server/src/main/scala/pl/touk/nussknacker/ui/api/description/stickynotes/StickyNoteEvent.scala (1)
1-14
: Add documentation and consider error handling.The event system would benefit from:
- ScalaDoc documentation explaining the purpose and usage of these events
- Explicit error handling in the decoder
/** * Represents events that can occur on sticky notes. * These events are used for tracking changes and managing sticky note lifecycle. */designer/client/src/components/toolbars/creator/StickyNoteComponent.tsx (2)
4-4
: Add explicit type annotation for noteModelThe
noteModel
constant would benefit from an explicit type annotation for better type safety and documentation.-const noteModel = { id: "StickyNoteToAdd", type: StickyNoteType, isDisabled: false }; +const noteModel: { id: string; type: typeof StickyNoteType; isDisabled: boolean } = { + id: "StickyNoteToAdd", + type: StickyNoteType, + isDisabled: false, +};
5-19
: Add JSDoc documentation for the component group functionThe
stickyNoteComponentGroup
function would benefit from documentation explaining its purpose and thepristine
parameter's role.+/** + * Creates a component group for sticky notes + * @param pristine - Indicates whether the graph is in a pristine state, controlling note creation + * @returns Array of ComponentGroup containing sticky note configuration + */ export const stickyNoteComponentGroup = (pristine: boolean) => {designer/client/src/reducers/graph/types.ts (1)
17-17
: Add TSDoc comment for the stickyNotes propertyThe new property would benefit from documentation explaining its purpose and optional nature.
+ /** Collection of sticky notes associated with the graph. Undefined when notes are not loaded. */ stickyNotes?: StickyNote[];
designer/client/src/components/graph/fragmentGraph.tsx (2)
7-10
: Enhance type safety with proper interface namingConsider extracting the props type to a named interface for better maintainability and documentation.
+interface FragmentGraphPreviewProps extends Pick<GraphProps, "processCounts" | "scenario" | "stickyNotes" | "nodeIdPrefixForFragmentTests"> {} + export const FragmentGraphPreview = forwardRef< Graph, - Pick<GraphProps, "processCounts" | "scenario" | "stickyNotes" | "nodeIdPrefixForFragmentTests"> + FragmentGraphPreviewProps
11-23
: Consider adding prop validationThe component would benefit from runtime prop validation, especially for the scenario prop which is used to access scenarioGraph.
if (!scenario?.scenarioGraph) { console.warn('FragmentGraphPreview: scenario or scenarioGraph is missing'); return null; }designer/client/cypress/e2e/creatorToolbar.cy.ts (1)
29-29
: LGTM! Consider expanding test coverage.The new stickyNotes click interaction follows the established pattern. However, consider adding specific test cases for:
- Filtering sticky notes
- Verifying sticky note creation from toolbar
- Testing sticky note visibility states
designer/client/src/containers/theme/helpers.ts (1)
19-26
: Consider adding color validation testsThe color validation logic is well-implemented using
CSS.supports
. However, this is a critical piece of UI functionality that should be covered by unit tests to ensure proper fallback behavior.Consider adding tests for:
- Valid color handling
- Invalid color fallback
- Theme color augmentation
designer/client/src/actions/notificationActions.tsx (2)
15-16
: Remove TODO comment after confirmationThe TODO comment should be addressed and removed. The changes appear to be an improvement to the error handling capability.
16-21
: Consider standardizing notification function signaturesWhile the enhanced error handling is valuable, consider:
- The error function now has a different signature pattern compared to success/info
- The conditional operator could be simplified
Consider this refactor for consistency and clarity:
-export function error(message: string, error?: string, showErrorText?: boolean): Action { +export function error(message: string, options?: { technicalDetails?: string, showTechnicalDetails?: boolean }): Action { return Notifications.error({ autoDismiss: 10, - children: <Notification type={"error"} icon={<InfoOutlinedIcon />} message={showErrorText && error ? error : message} />, + children: <Notification + type={"error"} + icon={<InfoOutlinedIcon />} + message={options?.showTechnicalDetails && options.technicalDetails || message} + />, }); }designer/client/src/components/graph/GraphPartialsInTS/cellUtils.ts (1)
16-21
: Consider performance optimization for large datasetsThe implementation is correct and safe. However, for large datasets, consider:
- The deep clone operation could be expensive
- The array find operation could be optimized
Consider this optimization if performance becomes a concern:
export function getStickyNoteCopyFromCell(stickyNotes: StickyNote[], el: dia.Cell): StickyNote | undefined { const noteId = el.get("noteId"); if (!isStickyNoteElement(el) || !noteId) return undefined; - const stickyNote = stickyNotes.find((note) => note.noteId == noteId); - return stickyNote ? cloneDeep(stickyNote) : undefined; + // Use loose equality check for consistency with existing code + const stickyNote = stickyNotes.find((note) => note.noteId == noteId); + // Only clone if found to avoid unnecessary operation + return stickyNote && { ...stickyNote }; }designer/server/src/main/scala/pl/touk/nussknacker/ui/process/repository/stickynotes/StickyNotesRepository.scala (3)
17-19
: Document the purpose of the clock fieldThe
clock
field's purpose and usage should be documented. Is it used for audit trails, timestamps on sticky notes, or something else?
41-50
: Add optimistic locking for updateStickyNoteThe update operation should include version checking to prevent concurrent modifications of the same note.
Consider adding a version field to the update parameters:
def updateStickyNote( noteId: StickyNoteId, + version: Long, content: String, ... )(implicit user: LoggedUser): DB[Int]
51-51
: Consider returning full StickyNote instead of EntityDataThe
findStickyNoteById
method returnsStickyNoteEventEntityData
which seems to be an internal database representation. Consider mapping it to the domain modelStickyNote
for better encapsulation.designer/client/src/components/graph/StickyNoteElement.ts (1)
1-15
: LGTM! Consider adding JSDoc comments.The interfaces are well-defined with proper TypeScript types. Consider adding JSDoc comments to document the purpose of each interface and their properties, especially since this is a new feature.
+/** + * Default configuration options for sticky note elements + */ export interface StickyNoteDefaults { + /** Initial position of the sticky note */ position?: { x: number; y: number }; + /** Dimensions of the sticky note */ size?: { width: number; height: number }; + /** Additional attributes for customization */ attrs?: Record<string, unknown>; }designer/client/src/components/toolbars/creator/ComponentIcon.tsx (1)
26-26
: Consider making the path configurableThe hardcoded asset path
/assets/components/${StickyNoteType}.svg
should ideally be configurable to support different deployment scenarios.Consider extracting this to a configuration constant:
-export const stickyNoteIconSrc = `/assets/components/${StickyNoteType}.svg`; +const COMPONENT_ASSETS_PATH = '/assets/components'; +export const stickyNoteIconSrc = `${COMPONENT_ASSETS_PATH}/${StickyNoteType}.svg`;designer/client/src/components/graph/node-modal/node/FragmentContent.tsx (1)
44-44
: Consider adding TypeScript prop typesThe
stickyNotes
prop is passed without explicit type information.Consider adding proper TypeScript types:
interface FragmentGraphPreviewProps { processCounts: Record<string, any>; // Replace 'any' with proper type scenario: Scenario; stickyNotes: StickyNote[]; // Add proper type nodeIdPrefixForFragmentTests: string; }designer/client/src/components/StickyNotePreview.tsx (3)
9-11
: Consider using TypeScript constants with descriptive namesThe magic numbers could be more descriptively named to better convey their purpose:
-const PREVIEW_SCALE = 0.9; -const ACTIVE_ROTATION = 2; -const INACTIVE_SCALE = 1.5; +const PREVIEW_SCALE_FACTOR = 0.9; +const ACTIVE_ROTATION_DEGREES = 2; +const INACTIVE_SCALE_MULTIPLIER = 1.5;
13-18
: Add JSDoc documentation for the componentThe component's purpose and props should be documented for better maintainability.
+/** + * Renders a preview of a sticky note with hover and active state animations. + * @param isActive - Whether the note is in active state + * @param isOver - Whether the mouse is over the note + */ export function StickyNotePreview({ isActive, isOver }: { isActive?: boolean; isOver?: boolean }): JSX.Element {
19-34
: Consider extracting animation properties to a separate constantThe transition and transform properties could be extracted for better maintainability.
+const ANIMATION_PROPERTIES = { + transition: "all .5s, opacity .3s", + willChange: "transform, opacity, border-color, background-color", +} as const; const nodeStyles = css({ ... - transition: "all .5s, opacity .3s", - willChange: "transform, opacity, border-color, background-color", + ...ANIMATION_PROPERTIES, });designer/client/src/components/graph/types.ts (3)
10-12
: Group related action types togetherThe sticky note actions should be grouped together in the imports for better organization.
import { injectNode, Layout, layoutChanged, nodeAdded, nodesConnected, nodesDisconnected, resetSelection, + // Sticky note actions stickyNoteAdded, stickyNoteDeleted, stickyNoteUpdated, toggleSelection, } from "../../actions/nk";
Also applies to: 25-27
31-31
: Document the stickyNotes propertyAdd TSDoc comments to explain the purpose and usage of the stickyNotes array.
+/** Collection of sticky notes associated with the scenario/fragment */ stickyNotes: StickyNote[];
Also applies to: 49-49
77-79
: Add JSDoc comments for new event typesThe new cell-related events should be documented to explain their purpose.
+ /** Fired when a cell's size is changed */ CELL_RESIZED = "cellCustom:resized", + /** Fired when a cell's content is modified */ CELL_CONTENT_UPDATED = "cellCustom:contentUpdated", + /** Fired when a cell is removed from the graph */ CELL_DELETED = "cellCustom:deleted",designer/server/src/test/scala/pl/touk/nussknacker/ui/api/StickyNotesApiHttpServiceBusinessSpec.scala (2)
84-84
: Implement additional test scenariosThe TODO comment indicates missing test coverage. Consider adding tests for:
- Adding sticky notes
- Updating sticky notes
- Deleting sticky notes
- Concurrent modifications
- Permission checks
Would you like me to help generate these additional test cases or create a GitHub issue to track this task?
45-82
: Add test description commentsEach test case should have a descriptive comment explaining the test scenario and expectations.
+"The GET stickyNotes for scenario" - { + // Test: Verify empty response when no sticky notes exist "return no notes if nothing was created" in { ... } + // Test: Verify 404 response for non-existent scenarios "return 404 if no scenario with given name exists" in { ... } + // Test: Verify version-specific sticky note retrieval "return zero notes for scenarioVersion=1 if notes were added in scenarioVersion=2" in { ... } }designer/client/src/components/ComponentPreview.tsx (1)
80-82
: Consider enhancing type safety for node type checkWhile the conditional rendering works, we could improve type safety by using a type guard function.
Consider this refactor:
- return node?.type === StickyNoteType ? ( + const isStickyNote = (node: NodeType): node is StickyNoteType => node?.type === StickyNoteType; + return isStickyNote(node) ? (designer/client/src/components/graph/ProcessGraph.tsx (1)
49-54
: Simplify drop handling logicThe drop handling logic could be more maintainable by extracting it into a separate function.
Consider this refactor:
+ const handleItemDrop = (item: NodeType, position: Point) => { + if (item?.type === StickyNoteType) { + return graph.current.addStickyNote(scenario.name, scenario.processVersionId, position); + } + graph.current.addNode(item, position); + setLinksHovered(graph.current.graph); + }; + drop: (item: NodeType, monitor) => { const clientOffset = monitor.getClientOffset(); const relOffset = graph.current.processGraphPaper.clientToLocalPoint(clientOffset); const nodeInputRelOffset = relOffset.offset(RECT_WIDTH * -0.8, RECT_HEIGHT * -0.5); - 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); - } + handleItemDrop(item, mapValues(nodeInputRelOffset, Math.round)); },designer/server/src/main/scala/pl/touk/nussknacker/ui/api/description/stickynotes/Dtos.scala (2)
44-54
: Consider adding field documentation.The case class is well-structured with appropriate types. Consider adding ScalaDoc comments to document the purpose of each field, especially
targetEdge
which might not be immediately obvious to other developers.
56-75
: Consider extracting common fields to a base trait.The
StickyNoteAddRequest
andStickyNoteUpdateRequest
classes share many fields. Consider extracting these to a base trait to follow DRY principles:+@derive(encoder, decoder, schema) +trait BaseStickyNoteRequest { + def scenarioVersionId: VersionId + def content: String + def layoutData: LayoutData + def color: String + def dimensions: Dimensions + def targetEdge: Option[String] +} @derive(encoder, decoder, schema) -case class StickyNoteAddRequest( +case class StickyNoteAddRequest( scenarioVersionId: VersionId, content: String, layoutData: LayoutData, color: String, dimensions: Dimensions, targetEdge: Option[String] -) +) extends BaseStickyNoteRequest @derive(encoder, decoder, schema) case class StickyNoteUpdateRequest( noteId: StickyNoteId, scenarioVersionId: VersionId, content: String, layoutData: LayoutData, color: String, dimensions: Dimensions, targetEdge: Option[String] -) +) extends BaseStickyNoteRequestdesigner/client/src/actions/nk/process.ts (1)
76-92
: Consider implementing optimistic updates.The current implementation waits for the server response before updating the UI. Consider implementing optimistic updates to improve user experience:
export function stickyNoteUpdated(scenarioName: string, scenarioVersionId: number, stickyNote: StickyNote): ThunkAction { return (dispatch) => { + // Optimistically update UI + dispatch({ type: "STICKY_NOTES_UPDATED", stickyNotes: [stickyNote] }); + dispatch(layoutChanged()); + HttpService.updateStickyNote(scenarioName, scenarioVersionId, stickyNote).then((_) => { - refreshStickyNotes(dispatch, scenarioName, scenarioVersionId); + // Refresh to ensure consistency + refreshStickyNotes(dispatch, scenarioName, scenarioVersionId); + }).catch((error) => { + console.error("Failed to update sticky note:", error); + // Revert optimistic update + refreshStickyNotes(dispatch, scenarioName, scenarioVersionId); }); }; }designer/server/src/main/scala/pl/touk/nussknacker/ui/config/FeatureTogglesConfig.scala (1)
61-63
: Consider adding validation for StickyNotesSettingsThe configuration parsing looks good with proper fallback to default settings. However, consider adding validation similar to how
deploymentCommentSettings
is validated to ensure the settings are within acceptable bounds.designer/client/src/components/toolbars/creator/ToolBox.tsx (1)
124-128
: Consider extracting the groups logic to a custom hookWhile the implementation is correct, consider extracting this logic into a custom hook (e.g.,
useToolboxGroups
) to improve reusability and testability.+const useToolboxGroups = (componentGroups, filters, pristine, stickyNotesSettings) => { + const stickyNoteToolGroup = useMemo( + () => stickyNoteComponentGroup(pristine), + [pristine] + ); + + return useMemo(() => { + const allComponentGroups = stickyNotesSettings.enabled + ? concat(componentGroups, stickyNoteToolGroup) + : componentGroups; + return allComponentGroups + .map(filterComponentsByLabel(filters)) + .filter((g) => g.components.length > 0); + }, [componentGroups, filters, stickyNoteToolGroup, stickyNotesSettings]); +};designer/client/src/reducers/selectors/graph.ts (1)
75-77
: Consider adding type annotation for selector return typeWhile the implementation is correct, consider adding an explicit return type annotation for better type safety and documentation.
-export const getStickyNotes = createSelector([getGraph, getStickyNotesSettings], (g, settings) => +export const getStickyNotes = createSelector([getGraph, getStickyNotesSettings], (g, settings): StickyNote[] => settings.enabled ? g.stickyNotes || ([] as StickyNote[]) : ([] as StickyNote[]), );designer/client/src/components/graph/NodeDescriptionPopover.tsx (1)
8-8
: LGTM! Consider grouping related imports.The changes correctly prevent the description popover from appearing for sticky notes. However, consider grouping the
isStickyNoteElement
import with other graph-related imports for better code organization.import { Graph } from "./Graph"; +import { isStickyNoteElement } from "./GraphPartialsInTS"; import { MarkdownStyled } from "./node-modal/MarkdownStyled"; import { Events } from "./types"; -import { isStickyNoteElement } from "./GraphPartialsInTS";Also applies to: 123-123
designer/server/src/main/resources/defaultDesignerConfig.conf (1)
213-217
: LGTM! Consider adding documentation for the settings.The sticky notes configuration looks good with reasonable limits. Consider adding comments to document:
- The purpose of each setting
- The implications of these limits on user experience
- The impact of the
enabled
flag on existing notesstickyNotesSettings: { + # Maximum length of sticky note content in characters maxContentLength: 5000, + # Maximum number of sticky notes per scenario maxNotesCount: 5 + # Global feature toggle for sticky notes functionality enabled: true }designer/client/src/components/graph/graphStyledWrapper.ts (1)
206-216
: LGTM! Consider adding transition for focus state.The focus and disabled states are well-defined. Consider adding a smooth transition for the focus outline.
".sticky-note-markdown-editor:focus": { outline: "none", + transition: "box-shadow 0.2s ease-in-out", boxShadow: `0 0 0 2px ${theme.palette.primary.main}`, },
designer/client/src/reducers/graph/utils.ts (2)
82-93
: LGTM! Consider adding type annotations for better maintainability.The function correctly handles the removal of sticky notes while maintaining immutability. Consider adding explicit return type annotation for better code maintainability.
-export function removeStickyNoteFromLayout(state: GraphState, stickyNoteId: number): { layout: NodePosition[]; stickyNotes: StickyNote[] } { +export function removeStickyNoteFromLayout(state: GraphState, stickyNoteId: number): { + layout: NodePosition[]; + stickyNotes: StickyNote[]; +} {
95-107
: LGTM! Consider adding JSDoc documentation.The function correctly prepares new sticky notes while maintaining immutability. Consider adding JSDoc documentation to describe the purpose, parameters, and return value.
+/** + * Prepares new sticky notes with their corresponding layout positions + * @param state - Current graph state + * @param stickyNotes - Array of sticky notes to be added + * @returns Object containing updated layout and sticky notes + */ export function prepareNewStickyNotesWithLayout(designer/client/src/http/HttpService.ts (2)
695-707
: Consider making the default sticky note color configurable.The hardcoded default color should be moved to a configuration file for better maintainability and customization.
Consider:
- Moving
STICKY_NOTE_DEFAULT_COLOR
to a configuration file- Allowing customization through environment variables or UI settings
740-746
: Enhance type safety for sticky notes retrieval.Consider adding:
- Runtime validation of the response data
- Query parameter validation
getStickyNotes(scenarioName: string, scenarioVersionId: number): Promise<AxiosResponse<StickyNote[]>> { + if (scenarioVersionId < 0) { + return Promise.reject(new Error("Invalid scenario version ID")); + } const promise = api.get(...); + return promise.then(response => { + // Validate response data + if (!Array.isArray(response.data)) { + throw new Error("Invalid response format"); + } + return response; + });
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
⛔ Files ignored due to path filters (3)
designer/client/cypress/e2e/__image_snapshots__/electron/Linux/Creator toolbar should allow collapse (persist) and filtering #0.png
is excluded by!**/*.png
designer/client/package-lock.json
is excluded by!**/package-lock.json
designer/server/src/main/resources/web/static/assets/components/StickyNote.svg
is excluded by!**/*.svg
📒 Files selected for processing (59)
designer/client/cypress/e2e/creatorToolbar.cy.ts
(1 hunks)designer/client/package.json
(1 hunks)designer/client/src/actions/actionTypes.ts
(1 hunks)designer/client/src/actions/nk/assignSettings.ts
(1 hunks)designer/client/src/actions/nk/process.ts
(3 hunks)designer/client/src/actions/notificationActions.tsx
(1 hunks)designer/client/src/assets/json/nodeAttributes.json
(1 hunks)designer/client/src/common/StickyNote.ts
(1 hunks)designer/client/src/components/ComponentPreview.tsx
(2 hunks)designer/client/src/components/StickyNotePreview.tsx
(1 hunks)designer/client/src/components/graph/EspNode/stickyNote.ts
(1 hunks)designer/client/src/components/graph/EspNode/stickyNoteElements.ts
(1 hunks)designer/client/src/components/graph/Graph.tsx
(13 hunks)designer/client/src/components/graph/GraphPartialsInTS/applyCellChanges.ts
(2 hunks)designer/client/src/components/graph/GraphPartialsInTS/cellUtils.ts
(2 hunks)designer/client/src/components/graph/NodeDescriptionPopover.tsx
(2 hunks)designer/client/src/components/graph/ProcessGraph.tsx
(4 hunks)designer/client/src/components/graph/StickyNoteElement.ts
(1 hunks)designer/client/src/components/graph/fragmentGraph.tsx
(1 hunks)designer/client/src/components/graph/graphStyledWrapper.ts
(2 hunks)designer/client/src/components/graph/node-modal/node/FragmentContent.tsx
(3 hunks)designer/client/src/components/graph/types.ts
(3 hunks)designer/client/src/components/graph/utils/graphUtils.ts
(1 hunks)designer/client/src/components/toolbars/creator/ComponentIcon.tsx
(2 hunks)designer/client/src/components/toolbars/creator/StickyNoteComponent.tsx
(1 hunks)designer/client/src/components/toolbars/creator/ToolBox.tsx
(2 hunks)designer/client/src/components/toolbars/creator/ToolboxComponentGroup.tsx
(1 hunks)designer/client/src/containers/theme/helpers.ts
(2 hunks)designer/client/src/http/HttpService.ts
(3 hunks)designer/client/src/reducers/graph/reducer.ts
(2 hunks)designer/client/src/reducers/graph/types.ts
(2 hunks)designer/client/src/reducers/graph/utils.ts
(3 hunks)designer/client/src/reducers/selectors/graph.ts
(2 hunks)designer/client/src/reducers/selectors/settings.ts
(1 hunks)designer/client/src/types/component.ts
(1 hunks)designer/client/src/types/node.ts
(1 hunks)designer/client/src/types/stickyNote.ts
(1 hunks)designer/server/src/main/resources/defaultDesignerConfig.conf
(1 hunks)designer/server/src/main/scala/db/migration/V1_060__CreateStickyNotesDefinition.scala
(1 hunks)designer/server/src/main/scala/db/migration/hsql/V1_060__CreateStickyNotes.scala
(1 hunks)designer/server/src/main/scala/db/migration/postgres/V1_060__CreateStickyNotes.scala
(1 hunks)designer/server/src/main/scala/pl/touk/nussknacker/ui/api/SettingsResources.scala
(3 hunks)designer/server/src/main/scala/pl/touk/nussknacker/ui/api/StickyNotesApiHttpService.scala
(1 hunks)designer/server/src/main/scala/pl/touk/nussknacker/ui/api/description/StickyNotesApiEndpoints.scala
(1 hunks)designer/server/src/main/scala/pl/touk/nussknacker/ui/api/description/stickynotes/Dtos.scala
(1 hunks)designer/server/src/main/scala/pl/touk/nussknacker/ui/api/description/stickynotes/StickyNoteEvent.scala
(1 hunks)designer/server/src/main/scala/pl/touk/nussknacker/ui/config/FeatureTogglesConfig.scala
(4 hunks)designer/server/src/main/scala/pl/touk/nussknacker/ui/db/NuTables.scala
(1 hunks)designer/server/src/main/scala/pl/touk/nussknacker/ui/db/entity/StickyNotesEntityFactory.scala
(1 hunks)designer/server/src/main/scala/pl/touk/nussknacker/ui/process/repository/stickynotes/DbStickyNotesRepository.scala
(1 hunks)designer/server/src/main/scala/pl/touk/nussknacker/ui/process/repository/stickynotes/StickyNotesRepository.scala
(1 hunks)designer/server/src/main/scala/pl/touk/nussknacker/ui/server/AkkaHttpBasedRouteProvider.scala
(4 hunks)designer/server/src/test/resources/config/common-designer.conf
(1 hunks)designer/server/src/test/scala/pl/touk/nussknacker/test/base/it/WithSimplifiedConfigScenarioHelper.scala
(2 hunks)designer/server/src/test/scala/pl/touk/nussknacker/test/utils/domain/ScenarioHelper.scala
(4 hunks)designer/server/src/test/scala/pl/touk/nussknacker/ui/api/StickyNotesApiHttpServiceBusinessSpec.scala
(1 hunks)docs-internal/api/nu-designer-openapi.yaml
(3 hunks)docs/Changelog.md
(1 hunks)docs/MigrationGuide.md
(1 hunks)
🧰 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 (55)
designer/server/src/test/scala/pl/touk/nussknacker/test/utils/domain/ScenarioHelper.scala (2)
13-13
: LGTM: Well-organized imports
The new imports are properly organized and specifically scoped to support the sticky notes functionality.
Also applies to: 19-23, 26-26
48-48
: LGTM: Repository field follows established patterns
The sticky notes repository field is properly initialized with required dependencies and follows the class's established patterns for repository declarations.
docs/MigrationGuide.md (3)
Line range hint 1-2
: LGTM: Clear and concise title
The title and initial description are clear and appropriate for a migration guide.
10-14
: LGTM: Well-structured configuration changes section
The configuration changes section is well-organized and clearly explains the sticky notes feature configuration parameters.
Line range hint 1-3754
: Verify all version numbers follow semantic versioning
Please verify that all version numbers in the document follow semantic versioning (MAJOR.MINOR.PATCH) consistently.
✅ Verification successful
Based on the output from the version number check, I can verify that all version numbers in the migration guide follow semantic versioning (MAJOR.MINOR.PATCH) format. The versions range from 0.0.8 to 1.19.0 and include both released versions and unreleased ones (marked with "Not released yet").
All version numbers follow semantic versioning pattern correctly
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Check for version numbers that don't follow semantic versioning pattern
echo "Checking for version numbers that don't follow semantic versioning..."
rg -A 1 "## In version \d+\.\d+\.\d+" || echo "No version numbers found or all follow semantic versioning"
Length of output: 2536
designer/server/src/main/scala/pl/touk/nussknacker/ui/api/SettingsResources.scala (3)
10-10
: LGTM!
The import statement for StickyNotesSettings
is correctly placed and follows the project's package structure conventions.
46-47
: LGTM!
The changes maintain good code style with the trailing comma and correctly integrate the sticky notes settings from the configuration.
139-139
: Verify documentation and migration requirements.
The addition of stickyNotesSettings
to ToggleFeaturesOptions
looks correct, but please ensure:
- A changelog entry is added as per PR requirements
- If this change affects JSON serialization of existing configurations, document it in the migration guide
✅ Verification successful
Documentation requirements are properly addressed
The changes are well-documented:
- Changelog entry exists with detailed feature description (Sticky notes #7181)
- Migration guide includes configuration changes with clear parameters explanation
- The change is non-breaking as it's an addition of an optional feature with configuration
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Check for changelog and migration guide entries
echo "Checking for changelog entry..."
rg -l "sticky.*notes" docs/Changelog.md || echo "No changelog entry found"
echo "Checking for migration guide entry..."
rg -l "sticky.*notes" docs/MigrationGuide.md || echo "No migration guide entry found"
# Check for existing usages of ToggleFeaturesOptions in tests
echo "Checking test coverage..."
rg "ToggleFeaturesOptions" --type scala -g "*Test*.scala" -g "*Spec*.scala"
Length of output: 424
Script:
#!/bin/bash
# Let's check the actual content of changelog and migration guide entries
echo "=== Changelog entry content ==="
rg -A 5 -B 5 "sticky.*notes" docs/Changelog.md
echo "=== Migration guide entry content ==="
rg -A 5 -B 5 "sticky.*notes" docs/MigrationGuide.md
# Let's also check the ToggleFeaturesOptions class usage in tests to understand if JSON serialization is covered
echo "=== ToggleFeaturesOptions test coverage details ==="
rg -A 5 "ToggleFeaturesOptions" --type scala -g "*Test*.scala" -g "*Spec*.scala"
Length of output: 1877
designer/client/package.json (1)
25-25
: Good choice of DOMPurify for XSS prevention
The addition of DOMPurify is a solid security measure for sanitizing HTML content in sticky notes. Consider documenting the following security guidelines for the team:
- Always sanitize HTML content before rendering
- Apply sanitization as close to the rendering point as possible
- Consider creating a wrapper component to enforce consistent sanitization
Let's verify if there are any other HTML sanitization libraries that might conflict:
designer/client/src/components/graph/GraphPartialsInTS/applyCellChanges.ts (1)
16-16
: Verify all invocations of applyCellChanges
are updated with the new stickyNotes
parameter
The applyCellChanges
function signature has been modified to include a new parameter stickyNotes: StickyNote[]
. Ensure that all calls to this function throughout the codebase have been updated to pass the stickyNotes
argument to prevent potential runtime errors.
Run the following script to find all usages of applyCellChanges
and check if they include the new parameter:
✅ Verification successful
Based on the search results, I can generate the final response as we have found all the invocations of applyCellChanges
:
All invocations of applyCellChanges
are properly updated with the new parameter
The only invocation of applyCellChanges
in the codebase is in designer/client/src/components/graph/Graph.tsx
, and it correctly passes the stickyNotes
parameter along with other required arguments.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Find all calls to `applyCellChanges` and display them with context.
rg 'applyCellChanges\(' -A 3 -B 1
Length of output: 973
designer/server/src/main/scala/db/migration/V1_060__CreateStickyNotesDefinition.scala (1)
25-25
: Review the use of ON DELETE CASCADE
in foreign key constraint
Applying ON DELETE CASCADE
to the foreign key constraint on the sticky_notes
table means that deleting a record from process_versions
will automatically delete related sticky_notes
. Ensure this behavior is intended, as it may lead to unintended data loss.
Would you like to confirm if cascading deletes are appropriate for sticky notes associated with scenario versions?
designer/client/src/components/graph/EspNode/stickyNote.ts (1)
67-71
: Ensure safe sanitization and error handling in Markdown parsing
The use of DOMPurify.sanitize
with ADD_ATTR: ["target"]
allows target
attributes, which is necessary for opening links in new tabs. Confirm that this does not introduce any security vulnerabilities.
Additionally, consider displaying a user-friendly error message within the sticky note UI when Markdown parsing fails, rather than a generic error string.
Would you like to review the sanitization settings to ensure they comply with security best practices?
designer/server/src/main/scala/pl/touk/nussknacker/ui/api/description/StickyNotesApiEndpoints.scala (1)
129-131
: Validate Examples Provided in stickyNotesAddEndpoint
The example for StickyNoteAddRequest
might be missing necessary fields or contain incorrect values. Ensure that the example reflects a valid request that can be used as a reference.
Please verify the example and update it if needed.
designer/server/src/main/scala/pl/touk/nussknacker/ui/api/StickyNotesApiHttpService.scala (1)
197-203
: Consistent Error Handling in validateStickyNoteContent
The validateStickyNoteContent
method performs a length check on the content. Ensure that the error message provides clear and consistent information, including the actual content length and the maximum allowed length.
designer/client/src/components/graph/Graph.tsx (2)
382-386
: Ensure Tools Visibility for Sticky Notes
When selecting sticky notes, the tools are shown by calling showStickyNoteTools
. Ensure that tools are hidden for other elements to prevent overlapping or visual clutter.
563-563
: Adjust Indentation for Readability
There's a missing indentation on line 563, which affects the readability of the code.
[typo]
Apply this diff to fix the indentation:
- this.drawGraph(nextProps.scenario.scenarioGraph, nextProps.stickyNotes, nextProps.layout, nextProps.processDefinitionData);
+ this.drawGraph(nextProps.scenario.scenarioGraph, nextProps.stickyNotes, nextProps.layout, nextProps.processDefinitionData);
designer/server/src/main/scala/db/migration/hsql/V1_060__CreateStickyNotes.scala (1)
6-8
: LGTM! Clean and focused implementation.
The class correctly extends the base migration definition and properly overrides the profile for HSQL database.
Let's verify the base class existence and implementation:
✅ Verification successful
Base class implementation verified and looks good!
The HSQL migration class correctly extends the base trait V1_060__CreateStickyNotesDefinition
which contains proper implementation for creating sticky notes table with:
- All required columns and their constraints
- Foreign key relationship to process_versions table
- Appropriate logging
- Well-structured table definition using Slick
The implementation follows the standard migration pattern and the HSQL-specific class correctly provides the required database profile.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Verify base migration class existence and implementation
# Test: Search for the base class definition
rg -t scala "class V1_060__CreateStickyNotesDefinition"
# Test: Check the actual migration SQL
ast-grep --pattern 'class V1_060__CreateStickyNotesDefinition {
$$$
sqlResource = $$$
$$$
}'
Length of output: 158
Script:
#!/bin/bash
# Let's try a broader search to find the base class definition
# First, search for any file containing CreateStickyNotes in the filename
fd "CreateStickyNotes"
# Then search for any class definition related to sticky notes migration
rg -t scala "class.*CreateStickyNotes"
# Also search for trait or abstract class definitions that might be related
rg -t scala "(trait|abstract class).*CreateStickyNotes"
Length of output: 865
Script:
#!/bin/bash
# Let's check the content of the base trait to verify the implementation
cat designer/server/src/main/scala/db/migration/V1_060__CreateStickyNotesDefinition.scala
# Also check if there's a corresponding SQL file for this migration
fd -e sql "V1_060"
Length of output: 3224
designer/server/src/main/scala/db/migration/postgres/V1_060__CreateStickyNotes.scala (1)
6-8
: LGTM! Consistent with HSQL implementation.
The class correctly implements the Postgres-specific migration profile.
Let's verify schema consistency across database implementations:
✅ Verification successful
These commands will help us locate and analyze the migration files to verify schema consistency between HSQL and Postgres implementations.
Schema consistency verified across database implementations
Both HSQL and Postgres migrations extend the same base definition (V1_060__CreateStickyNotesDefinition
), which contains the complete table schema and foreign key constraints. The base definition ensures identical schema across both databases, with the only difference being the database-specific profile (PostgresProfile vs HSQLProfile). The schema includes:
- All required columns with proper types and constraints
- Foreign key relationship to
process_versions
table - Consistent auto-incrementing primary key
- Identical table name and structure
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Verify schema consistency between HSQL and Postgres migrations
# Test: Compare migration implementations
rg -t scala "CREATE TABLE sticky_notes" -A 15
Length of output: 47
Script:
#!/bin/bash
# Let's try to find the migration definition files first
fd -e scala "V1_060"
Length of output: 272
Script:
#!/bin/bash
# Search for any SQL-like table creation patterns in these files
ast-grep --pattern 'sql"""$$$"""'
Length of output: 35
Script:
#!/bin/bash
# Look for the base definition class
rg -t scala "CreateStickyNotesDefinition"
Length of output: 1449
Script:
#!/bin/bash
# Let's examine the content of the base definition file to see the schema
cat designer/server/src/main/scala/db/migration/V1_060__CreateStickyNotesDefinition.scala
Length of output: 3205
designer/client/src/types/component.ts (1)
8-8
: Add JSDoc documentation for the disabled property.
The disabled
property would benefit from documentation explaining its purpose and usage.
Apply this diff to add documentation:
+ /** Function that determines if the component should be disabled.
+ * @returns {boolean} True if the component should be disabled, false otherwise.
+ */
disabled?: () => boolean;
Let's verify the implementation of this property in component rendering:
✅ Verification successful
Let me try a different approach to find the usage of the disabled property in the codebase. I've modified the search patterns to be more inclusive and removed the typescript file type restriction since it wasn't recognized. This should help us verify how this property is being used in component rendering.
Based on the search results, I can now generate the final response:
The disabled
property is correctly implemented and used throughout the codebase
The property is used consistently in component rendering:
- In
ToolboxComponentGroup.tsx
, it's evaluated during render:disabled={component.disabled ? component.disabled() : false}
- In
StickyNoteComponent.tsx
, it's implemented as a function:disabled: () => !pristine
- The property is properly typed and integrated with React's component system
The implementation follows the TypeScript type definition and is used to dynamically control component states across the application.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Verify usage of disabled property in component rendering
# Test: Search for usage of disabled property
rg -t typescript "disabled\(\)" -A 5
# Test: Search for component rendering logic
ast-grep --pattern 'const $component = { disabled: $_() }'
Length of output: 134
Script:
#!/bin/bash
# Search for disabled property usage with ripgrep
rg "disabled\?:" -A 5
# Search for component rendering with disabled property using ast-grep
ast-grep --pattern 'disabled: $_()'
# Find TypeScript files that might contain component rendering
fd "\.tsx?$" | xargs rg "disabled"
Length of output: 48041
designer/client/src/assets/json/nodeAttributes.json (1)
41-43
: LGTM! Verify integration with node type system.
The new StickyNote entry follows the established pattern and maintains consistency with other node types.
Let's verify that this node type is properly integrated:
✅ Verification successful
LGTM! StickyNote node type is properly integrated.
The StickyNote type is well integrated across the codebase with comprehensive implementation:
- Core type definitions in
designer/client/src/types/stickyNote.ts
anddesigner/client/src/types/node.ts
- UI components in
designer/client/src/components/graph/StickyNoteElement.ts
and related files - Toolbox integration in
designer/client/src/components/toolbars/creator/StickyNoteComponent.tsx
- Backend support with proper repository and API endpoints
- Full feature implementation including database migrations
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Check if the StickyNote type is referenced in component definitions and handlers
rg -l "StickyNote" --type ts --type tsx
Length of output: 67
Script:
#!/bin/bash
# Let's try different approaches to find StickyNote references
# 1. Search in all files (not just ts/tsx) for StickyNote
rg -l "StickyNote"
# 2. Find relevant JavaScript/TypeScript files that might contain the implementation
fd -e js -e jsx -e ts
Length of output: 21503
designer/client/src/actions/actionTypes.ts (1)
13-14
: Consider adding STICKY_NOTE_CREATED action type.
While the update and delete actions are present, a dedicated creation action type might be beneficial for clarity and consistency with Redux patterns.
Let's verify the action types usage:
designer/client/src/containers/theme/helpers.ts (1)
4-4
: LGTM!
Clean import addition for the default color constant.
designer/client/src/components/graph/GraphPartialsInTS/cellUtils.ts (2)
2-3
: LGTM!
Clean import additions with clear purpose.
12-14
: LGTM!
Well-implemented type guard using TypeScript's type predicate pattern.
designer/client/src/types/node.ts (2)
3-5
: LGTM on the type extension
The addition of StickyNoteType
to the Type
union is clean and well-integrated.
Line range hint 1-55
: Address technical debt in node.ts
The file contains several issues that should be addressed:
- Untyped
$TodoType
placeholders - TODO comment about removing properties
- Index signature with
any
type
Let's scan for similar patterns across the codebase:
designer/server/src/test/resources/config/common-designer.conf (1)
57-61
: Document and validate sticky notes configuration
- Consider adding comments explaining the rationale behind the limits:
stickyNotesSettings: {
# Maximum length of note content to prevent performance issues
maxContentLength: 5000,
# Limit notes per scenario to maintain UI clarity
maxNotesCount: 5,
# Feature toggle for sticky notes functionality
enabled: true
}
- Verify if these limits are sufficient:
- 5000 characters might be restrictive for complex notes
- 5 notes per scenario seems very limiting
Let's check if these limits align with similar features:
✅ Verification successful
Sticky notes configuration limits are consistent with other features
The limits in stickyNotesSettings
align well with other configuration limits in the codebase:
maxContentLength: 5000
is reasonable when compared totestDataMaxLength: 10000
maxNotesCount: 5
is conservative but proportional tomaxSamplesCount: 20
in test data settings
The suggested documentation improvements are still valuable for maintainability, but the actual limits are well-balanced with other features in the system.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for similar length/count limits in configs
echo "Searching for length limits:"
rg "maxLength|Length:" "designer/server/src/**/resources/**/*.conf"
echo -e "\nSearching for count limits:"
rg "maxCount|Count:" "designer/server/src/**/resources/**/*.conf"
Length of output: 441
Script:
#!/bin/bash
# Let's try searching without the glob pattern and use fd to find conf files first
echo "Finding config files:"
fd -e conf . designer/server/src/test/resources/
echo -e "\nSearching for length limits:"
fd -e conf . designer/server/src/test/resources/ -x rg -i "max.*length|length:"
echo -e "\nSearching for count limits:"
fd -e conf . designer/server/src/test/resources/ -x rg -i "max.*count|count:"
# Let's also check if there are any other similar settings blocks
echo -e "\nSearching for other settings blocks:"
fd -e conf . designer/server/src/test/resources/ -x rg "Settings\s*:\s*\{" -A 5
Length of output: 1986
designer/client/src/components/graph/StickyNoteElement.ts (2)
17-18
: LGTM! Clean factory function implementation.
The element definition follows JointJS patterns and provides good type safety through TypeScript interfaces.
38-45
: Review keyboard shortcut implementation.
The current implementation of Ctrl+A/Cmd+A might conflict with browser/OS shortcuts. Consider using a different shortcut or adding a check for edit mode.
✅ Verification successful
Let me analyze the findings from the search results. The codebase shows several keyboard shortcut handlers:
- The
StickyNoteElement.ts
implementation for Ctrl+A/Cmd+A to select text BindKeyboardShortcuts.tsx
has a global Ctrl+A/Cmd+A handler for selecting all nodes- Other modifier key (Ctrl/Cmd) usages are for different purposes (pan-zoom, range select, undo/redo)
Let me verify if the StickyNoteElement
is used in an editable context.
Keyboard shortcut implementation is correctly implemented.
The Ctrl+A/Cmd+A implementation in StickyNoteElement.ts
is specifically scoped to textarea elements within sticky notes, as evidenced by the event binding "keydown textarea": "selectAll"
. This is a standard text selection behavior expected in editable fields and won't conflict with the global node selection shortcut because:
- It's only triggered when focus is within the textarea
- It uses
preventDefault()
appropriately - It implements the standard text selection behavior users expect in text input fields
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Check for other keyboard shortcut handlers that might conflict
rg -A 2 "KeyA|ctrlKey|metaKey" --type ts
Length of output: 4290
Script:
#!/bin/bash
# Check how StickyNoteElement is used
ast-grep --pattern 'class StickyNoteElement {
$$$
}'
# Also check for any textarea/input related code
rg -A 2 "textarea|contenteditable" --type ts
Length of output: 9545
designer/client/src/reducers/selectors/settings.ts (1)
17-17
: LGTM! Verify selector usage.
The new selector follows the established pattern and properly uses memoization. Let's verify its usage across the codebase.
✅ Verification successful
Selector usage verified and properly integrated
The getStickyNotesSettings
selector is correctly used across the codebase:
- Used in
ToolBox.tsx
component withuseSelector
hook - Composed with other selectors in
graph.ts
to creategetStickyNotes
selector - Implementation in
settings.ts
follows the established pattern
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Check usage of the new selector
rg "getStickyNotesSettings" --type ts
Length of output: 734
designer/client/src/actions/nk/assignSettings.ts (1)
42-42
: Verify settings initialization.
Ensure that the sticky notes settings are properly initialized with default values when not provided in the configuration.
designer/client/src/components/toolbars/creator/ComponentIcon.tsx (1)
27-29
: LGTM! Clean implementation of sticky note icon
The implementation follows the established pattern of icon preloading and reuses existing functionality.
designer/client/src/components/graph/node-modal/node/FragmentContent.tsx (1)
17-17
: LGTM! Proper Redux selector usage
The sticky notes selector is correctly integrated using the useSelector hook.
designer/server/src/test/scala/pl/touk/nussknacker/test/base/it/WithSimplifiedConfigScenarioHelper.scala (1)
44-46
: LGTM! Clean implementation of scenario update helper
The method properly delegates to rawScenarioHelper while maintaining the trait's pattern.
designer/client/src/components/toolbars/creator/ToolboxComponentGroup.tsx (1)
76-82
: LGTM! Consider verifying disabled state handling
The implementation of the disabled state for Tool components looks correct. The existence check for the disabled function is properly implemented.
Let's verify the disabled state handling implementation:
✅ Verification successful
Based on the search results, I can now generate the final response since we found the relevant disabled prop usage in the Tool component showing it's properly handled in the drag functionality.
LGTM! Disabled state is properly implemented
The disabled prop is correctly used in the Tool component to prevent dragging when disabled is true via the canDrag
option. This ensures disabled components cannot be dragged into the form builder.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for other usages of the disabled property to ensure consistent handling
rg -l "disabled\(\)" --type ts --type tsx
# Look for any disabled state unit tests
rg -l "disabled.*test" --type ts --type tsx
Length of output: 145
Script:
#!/bin/bash
# Search for other usages of the disabled property without file type restrictions
rg "disabled\(\)"
# Look for any disabled state unit tests
rg "disabled.*test"
# Search for Tool component usage and implementation
ast-grep --pattern 'interface ToolProps {
$$$
}'
ast-grep --pattern 'const Tool = $$$'
Length of output: 65855
Script:
#!/bin/bash
# Search for Tool component definition and props
rg -A 10 "interface ToolProps" designer/client/src/components/toolbars/creator/
# Search for disabled prop usage in Tool component
rg -A 10 "disabled.*prop" designer/client/src/components/toolbars/creator/
# Search for Tool component tests
rg "Tool.*test" designer/client/src/components/toolbars/creator/
Length of output: 1210
designer/client/src/components/graph/ProcessGraph.tsx (1)
87-89
: Add error handling for sticky note operations
The sticky note operations (add/update/delete) should include error handling to maintain application stability.
Let's verify the error handling implementation:
designer/server/src/main/scala/pl/touk/nussknacker/ui/api/description/stickynotes/Dtos.scala (3)
20-42
: LGTM! Well-structured value classes and data types.
The implementation follows Scala best practices:
- Value classes extend AnyVal to avoid runtime overhead
- Proper JSON serialization support via Circe
- Clear and concise data structures
87-117
: LGTM! Comprehensive error handling.
The error hierarchy is well-designed with:
- Clear error cases
- User-friendly error messages
- Proper integration with the authorization system
77-85
: Add validation and document default values.
Consider adding validation for the settings values and documenting the rationale for the default values:
- Why 5000 characters for maxContentLength?
- Why 5 notes maximum?
designer/client/src/components/graph/utils/graphUtils.ts (1)
130-134
: LGTM! Good improvement to event handling flexibility.
The addition of the preventDefault
parameter with a default value that maintains existing behavior is a clean way to add flexibility while ensuring backward compatibility.
designer/server/src/main/scala/pl/touk/nussknacker/ui/config/FeatureTogglesConfig.scala (2)
9-10
: LGTM: Clean import organization
The import for StickyNotesSettings is properly placed with other DTO imports.
32-33
: LGTM: Clean case class field addition
The stickyNotesSettings field is properly added with correct comma separation.
designer/client/src/components/toolbars/creator/ToolBox.tsx (2)
5-15
: LGTM: Clean import organization
Imports are well-organized and properly grouped by functionality.
118-119
: LGTM: Proper use of selectors
Good use of useSelector hooks for accessing sticky notes settings and pristine state.
designer/client/src/reducers/selectors/graph.ts (1)
11-12
: LGTM: Clean import organization
The imports for sticky notes types are properly placed.
designer/server/src/main/resources/defaultDesignerConfig.conf (1)
Line range hint 219-229
: Verify the impact of label validation rules.
The validation rules for scenario labels look good. Let's verify if existing scenarios comply with these rules to ensure we don't break anything.
✅ Verification successful
Let me try a different approach to verify the scenario labels, as the previous script had some issues with parsing JSON files.
This script should help us:
- Find actual scenario files by looking for ones containing "scenarioGraph"
- Extract label values from those files
- Also check test files that might define scenario labels in test cases
Let me try one more specific search to find actual scenario labels in test files:
This should help us find test cases that specifically deal with scenario labels and their validation, which would give us insight into what labels are currently in use and tested.
Based on the gathered information, I can now provide a final response:
The new validation rules for scenario labels are safe to implement.
The code analysis reveals that:
- The new rules (
^[a-zA-Z0-9-_]+$
and^.{1,20}$
) are more permissive than existing test cases which show stricter validation (e.g., 5-character limit in test cases). - The validation system is well-tested with proper error handling for invalid labels.
- The new rules allow common characters (alphanumeric, hyphen, underscore) which are standard for labels.
- The 20-character limit is reasonable and more generous than previous constraints seen in tests.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Check existing scenario labels against new validation rules
# Expected results: List of any scenario labels that don't match the new rules
# Search for scenario label patterns that don't match the new rules
rg -l '"label":\s*"[^"]*"' | while read -r file; do
echo "Checking $file"
# Extract labels and check against rules
jq -r '.label // empty' "$file" | grep -vE '^[a-zA-Z0-9-_]{1,20}$' || true
done
Length of output: 7511
Script:
#!/bin/bash
# Search for scenario files and check their content for labels
rg -g '*.json' -l '"scenarioGraph"' | while read -r file; do
echo "=== Checking $file ==="
# Use rg to extract label values directly from the file
rg -o '"label":\s*"([^"]*)"' "$file" --replace '$1' || true
done
# Also check test files that might contain scenario labels
rg -g '*.scala' -A 1 'label\s*=' || true
Length of output: 26764
Script:
#!/bin/bash
# Search for scenario label test cases
rg -g '*.scala' -A 2 'ScenarioLabel\(' || true
# Also check for any test files with label validation
rg -g '*.scala' -A 3 'validateScenarioLabel|ScenarioLabelValidation' || true
Length of output: 11315
designer/client/src/components/graph/graphStyledWrapper.ts (1)
184-189
: LGTM! Well-structured sticky note styles.
The markdown container styles are clean and use appropriate spacing.
designer/client/src/reducers/graph/utils.ts (1)
136-145
: LGTM! The implementation is clean and maintains immutability.
The function correctly integrates sticky notes and their layout into the state.
designer/client/src/reducers/graph/reducer.ts (1)
19-24
: LGTM! All necessary sticky notes utility functions are imported.
designer/server/src/main/scala/pl/touk/nussknacker/ui/server/AkkaHttpBasedRouteProvider.scala (4)
72-72
: LGTM! Import is correctly placed with other repository imports.
171-171
: LGTM! Repository initialization follows the established pattern.
402-409
: LGTM! Service initialization is complete with all required dependencies.
603-603
: LGTM! Service is correctly integrated into nuDesignerApi.
designer/client/src/http/HttpService.ts (1)
709-719
: LGTM!
The implementation is secure and handles errors appropriately.
docs-internal/api/nu-designer-openapi.yaml (1)
2530-2929
: LGTM! The API specification is comprehensive and well-structured.
The sticky notes API endpoints and schemas are properly defined with:
- Clear authentication requirements
- Proper error responses
- Well-documented request/response schemas
Also applies to: 5492-7267
designer/server/src/main/scala/pl/touk/nussknacker/ui/db/entity/StickyNotesEntityFactory.scala
Show resolved
Hide resolved
designer/server/src/main/scala/pl/touk/nussknacker/ui/db/entity/StickyNotesEntityFactory.scala
Show resolved
Hide resolved
...in/scala/pl/touk/nussknacker/ui/process/repository/stickynotes/DbStickyNotesRepository.scala
Show resolved
Hide resolved
...in/scala/pl/touk/nussknacker/ui/process/repository/stickynotes/DbStickyNotesRepository.scala
Show resolved
Hide resolved
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actionable comments posted: 2
🧹 Outside diff range and nitpick comments (2)
designer/server/src/main/scala/db/migration/V1_060__CreateStickyNotesDefinition.scala (2)
40-44
: Add length constraints for string columns.String columns should have maximum length constraints to prevent excessive data storage and ensure consistent behavior across different database systems.
Consider adding length constraints:
-def content = column[String]("content", NotNull) -def layoutData = column[String]("layout_data", NotNull) -def color = column[String]("color", NotNull) -def dimensions = column[String]("dimensions", NotNull) +def content = column[String]("content", O.Length(1000), NotNull) +def layoutData = column[String]("layout_data", O.Length(500), NotNull) +def color = column[String]("color", O.Length(50), NotNull) +def dimensions = column[String]("dimensions", O.Length(100), NotNull)
74-87
: Add documentation and validation for the entity class.The case class would benefit from:
- ScalaDoc comments explaining the purpose of each field
- Validation annotations for field constraints
Consider adding documentation and validation:
/** Represents a sticky note event in the database * * @param id Unique identifier for the event * @param noteCorrelationId UUID correlating related sticky note events * @param content The text content of the sticky note * ... */ @validate final case class StickyNoteEventEntityData( id: Long, @size(max=36) noteCorrelationId: UUID, @size(max=1000) content: String, // ... rest of the fields with appropriate validation )
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (1)
designer/server/src/main/scala/db/migration/V1_060__CreateStickyNotesDefinition.scala
(1 hunks)
🔇 Additional comments (1)
designer/server/src/main/scala/db/migration/V1_060__CreateStickyNotesDefinition.scala (1)
24-24
: Verify the CASCADE delete behavior impact.
The foreign key constraint uses ON DELETE CASCADE
, which means deleting a process version will automatically delete all associated sticky notes. Ensure this is the intended behavior as it could lead to unintended data loss.
Run the following script to check if this cascading behavior is consistent with other related tables:
✅ Verification successful
Let me gather more information about the relationships between these tables to better understand the cascade behavior.
Let me try a different approach to check the table structures:
Let me try one final search with a simpler pattern to understand the table relationships:
ON DELETE CASCADE is consistent with the existing schema design
The cascading delete behavior is consistent with the schema design pattern used in related tables:
sticky_notes
cascades deletions fromprocess_versions
scenario_activities
cascades deletions fromprocesses
This indicates a deliberate architectural decision where child entities are automatically cleaned up when their parent entities are removed, maintaining referential integrity and preventing orphaned records.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Check for CASCADE delete behavior in other migration files
rg -A 2 "ON DELETE CASCADE" --type scala
Length of output: 1164
Script:
#!/bin/bash
# Check table creation details for related tables
rg -A 10 'CREATE TABLE.*process_versions|CREATE TABLE.*sticky_notes|CREATE TABLE.*scenario_activities|CREATE TABLE.*processes' --type scala
Length of output: 141
Script:
#!/bin/bash
# Search for table creation statements with more flexible pattern
rg -A 10 'CREATE TABLE.*"(sticky_notes|process_versions|scenario_activities|processes)"' --type scala
Length of output: 103
Script:
#!/bin/bash
# Search for any mentions of these tables in migration files
rg '"sticky_notes"|"process_versions"|"scenario_activities"|"processes"' --type scala designer/server/src/main/scala/db/migration/
Length of output: 1296
designer/server/src/main/scala/db/migration/V1_060__CreateStickyNotesDefinition.scala
Show resolved
Hide resolved
designer/server/src/main/scala/db/migration/V1_060__CreateStickyNotesDefinition.scala
Show resolved
Hide resolved
7fd180e
to
c76fbc4
Compare
…ation, fix error method (or did i?)
Co-authored-by: philemone <[email protected]>
c76fbc4
to
170c47d
Compare
created: #7311 |
@@ -26,6 +26,7 @@ describe("Creator toolbar", () => { | |||
cy.contains(/^types$/i).click(); | |||
cy.contains(/^services$/i).click(); | |||
cy.contains(/^sinks$/i).click(); | |||
cy.contains(/^stickyNotes$/i).click(); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Label looks like technical name of variable. Can you change it to sth like "sticky notes" ?
Describe your changes
Checklist before merge
Summary by CodeRabbit
Release Notes
New Features
GET /api/processes/{scenarioName}/stickyNotes
PUT /api/processes/{scenarioName}/stickyNotes
POST /api/processes/{scenarioName}/stickyNotes
DELETE /api/processes/{scenarioName}/stickyNotes/{noteId}
Bug Fixes
Documentation