Preview/notes #7278
Preview/notes #7278
Conversation
…ation, fix error method (or did i?)
Co-authored-by: philemone <[email protected]>
📝 WalkthroughWalkthroughThe pull request introduces significant enhancements related to sticky notes within the Nussknacker Designer application. It adds a new Cypress end-to-end test for the Creator toolbar, specifically including interactions with a new "stickyNotes" element. Additionally, a new dependency, "dompurify", is introduced to the project's package.json for HTML sanitization. The action types for sticky notes are defined, including "STICKY_NOTES_UPDATED" and "STICKY_NOTE_DELETED". New settings for sticky notes are defined, allowing for configuration of maximum content length, note count, and enabling/disabling functionality. The implementation includes CRUD operations for sticky notes in the backend, with corresponding API endpoints for managing these notes. Furthermore, the database schema is updated to accommodate sticky notes, and various UI components are modified to support sticky note interactions. Overall, the changes enhance the application's capability to manage sticky notes effectively, integrating them into both the frontend and backend systems. Possibly related PRs
Suggested labels
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 13
🧹 Outside diff range and nitpick comments (58)
docs/Changelog.md (1)
13-16: LGTM! Consider adding migration notes if needed.The changelog entries for the StickyNotes feature are well documented and follow the established format. The description clearly explains the feature's purpose and configuration options.
Consider adding a note about any migration steps needed if this feature requires changes to existing configurations or database schema updates.
designer/server/src/main/scala/pl/touk/nussknacker/ui/api/description/stickynotes/Dtos.scala (4)
20-25: Ensure Consistent Use of Derivation for Encoders and DecodersFor
StickyNoteIdandStickyNoteCorrelationId, encoders and decoders are manually implemented. In contrast, other case classes use@derive(encoder, decoder, schema)to auto-generate these instances. For consistency and to reduce boilerplate, consider using@derivefor these classes unless custom logic is necessary.Apply this change:
+import derevo.circe.{decoder, encoder} ... @derive(encoder, decoder) final case class StickyNoteId(value: Long) extends AnyVal @derive(encoder, decoder) final case class StickyNoteCorrelationId(value: UUID) extends AnyVal - object StickyNoteId { - implicit val encoder: Encoder[StickyNoteId] = Encoder.encodeLong.contramap(_.value) - implicit val decoder: Decoder[StickyNoteId] = Decoder.decodeLong.map(StickyNoteId(_)) - } - object StickyNoteCorrelationId { - implicit val encoder: Encoder[StickyNoteCorrelationId] = Encoder.encodeUUID.contramap(_.value) - implicit val decoder: Decoder[StickyNoteCorrelationId] = Decoder.decodeUUID.map(StickyNoteCorrelationId(_)) - }Also applies to: 27-32
57-64: Abstract Common Fields in Request Classes to Reduce DuplicationBoth
StickyNoteAddRequestandStickyNoteUpdateRequestshare several common fields. To enhance maintainability and adhere to the DRY (Don't Repeat Yourself) principle, consider abstracting the shared fields into a base trait or class.Example refactoring:
trait StickyNoteRequestBase { 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( scenarioVersionId: VersionId, content: String, layoutData: LayoutData, color: String, dimensions: Dimensions, targetEdge: Option[String] ) extends StickyNoteRequestBase @derive(encoder, decoder, schema) case class StickyNoteUpdateRequest( noteId: StickyNoteId, scenarioVersionId: VersionId, content: String, layoutData: LayoutData, color: String, dimensions: Dimensions, targetEdge: Option[String] ) extends StickyNoteRequestBaseAlso applies to: 67-75
83-85: Externalize Default Settings for ConfigurabilityThe default values for
StickyNotesSettingsare hardcoded, which can limit flexibility. To allow easy adjustments without changing the codebase, consider externalizing these defaults to a configuration file.Example:
Define the default settings in a configuration file (e.g., application.conf):
stickyNotesSettings { maxContentLength = 5000 maxNotesCount = 5 enabled = true }Modify
StickyNotesSettingsto load values from the configuration:import com.typesafe.config.ConfigFactory object StickyNotesSettings { private val config = ConfigFactory.load().getConfig("stickyNotesSettings") val default: StickyNotesSettings = StickyNotesSettings( maxContentLength = config.getInt("maxContentLength"), maxNotesCount = config.getInt("maxNotesCount"), enabled = config.getBoolean("enabled") ) }
99-116: Refactor Error Codecs to Eliminate Repetitive PatternsThe implicit codecs for the error cases in
StickyNotesErrorexhibit repetitive code structures. To enhance readability and reduce duplication, consider introducing a helper method to generate these codecs.Refactored code:
private def createErrorCodec[T](message: T => String): Codec[String, T, CodecFormat.TextPlain] = BaseEndpointDefinitions.toTextPlainCodecSerializationOnly[T](message) implicit val noScenarioCodec: Codec[String, NoScenario, CodecFormat.TextPlain] = createErrorCodec(e => s"No scenario ${e.scenarioName} found") implicit val noStickyNoteCodec: Codec[String, NoStickyNote, CodecFormat.TextPlain] = createErrorCodec(e => s"No sticky note with id: ${e.noteId} was found") implicit val stickyNoteContentTooLongCodec: Codec[String, StickyNoteContentTooLong, CodecFormat.TextPlain] = createErrorCodec(e => s"Provided note content is too long (${e.count} characters). Max content length is ${e.max}.") implicit val stickyNoteCountLimitReachedCodec: Codec[String, StickyNoteCountLimitReached, CodecFormat.TextPlain] = createErrorCodec(e => s"Cannot add another sticky note, since max number of sticky notes was reached: ${e.max} (see configuration).")designer/server/src/main/scala/db/migration/V1_060__CreateStickyNotesDefinition.scala (1)
21-26: Ensure proper logging outside of database actions.Logging inside the
forcomprehension may not execute as intended within the database action. Consider moving the logging statements outside the database actions for clarity and to ensure they execute correctly.Apply this diff to adjust the logging:
logger.info("Starting migration V1_060__CreateStickyNotesDefinition") - for { + val actions = for { _ <- definitions.stickyNotesEntityTable.schema.create _ <- 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;""" } yield () - } yield logger.info("Execution finished for migration V1_060__CreateStickyNotesDefinition") + actions.andThen(DBIO.successful(logger.info("Execution finished for migration V1_060__CreateStickyNotesDefinition")))designer/server/src/main/scala/pl/touk/nussknacker/ui/db/entity/StickyNotesEntityFactory.scala (2)
77-80: Handle JSON parsing errors more gracefully.Throwing exceptions upon JSON parsing failures can lead to unhandled exceptions. Consider handling errors more gracefully by logging the error and providing a default value or informative message.
Apply this diff to improve error handling:
jsonStr => parser.parse(jsonStr).flatMap(Decoder[LayoutData].decodeJson) match { case Right(layoutData) => layoutData - case Left(error) => throw error + case Left(error) => + logger.error("Failed to parse LayoutData JSON", error) + throw new RuntimeException("Invalid LayoutData format", error) }
86-89: Handle JSON parsing errors more gracefully.Similar to
layoutData, improve error handling fordimensionsto avoid unhandled exceptions.Apply this diff to improve error handling:
jsonStr => parser.parse(jsonStr).flatMap(Decoder[Dimensions].decodeJson) match { case Right(dimensions) => dimensions - case Left(error) => throw error + case Left(error) => + logger.error("Failed to parse Dimensions JSON", error) + throw new RuntimeException("Invalid Dimensions format", error) }designer/client/src/components/graph/EspNode/stickyNote.ts (1)
96-96: Use the defined default color constant instead of hard-coded value.Replace the hard-coded color
"#eae672"with theSTICKY_NOTE_DEFAULT_COLORconstant for consistency and ease of maintenance.Apply this diff to use the constant:
fill: "#eae672", + // fill: STICKY_NOTE_DEFAULT_COLOR, + fill: STICKY_NOTE_DEFAULT_COLOR,designer/client/src/components/graph/EspNode/stickyNoteElements.ts (1)
17-17: Remove unused parameterprocessDefinitionData.The parameter
processDefinitionDatais not used within themakeStickyNoteElementfunction. Consider removing it to clean up the code.Apply this diff to remove the unused parameter:
-export function makeStickyNoteElement( - processDefinitionData: ProcessDefinitionData, +export function makeStickyNoteElement( theme: Theme, ): (stickyNote: StickyNote) => ModelWithTool {designer/server/src/main/scala/pl/touk/nussknacker/ui/process/repository/stickynotes/DbStickyNotesRepository.scala (2)
34-64: Refactor duplicated query logic infindStickyNotesandcountStickyNotesThe methods
findStickyNotesandcountStickyNoteshave similar query logic. To adhere to the DRY (Don't Repeat Yourself) principle, consider extracting the common parts into a private helper method or function.
103-110: Simplify error handling inaddStickyNotemethodIn the
addStickyNotemethod, the insert operation checks for the number of affected rows and throws anIllegalStateExceptionfor unexpected cases. Since the database constraints should ensure data integrity, consider simplifying this logic or handling failures without throwing generic exceptions.designer/server/src/main/scala/pl/touk/nussknacker/ui/api/description/StickyNotesApiEndpoints.scala (1)
170-215: UsejsonBodyinstead ofplainBodyfor error responsesIn the error variants such as
stickyNoteContentTooLongExample, you are usingplainBodyto serialize error responses. For consistency and better client handling, consider usingjsonBodyto return error details in JSON format.designer/server/src/main/scala/pl/touk/nussknacker/ui/api/StickyNotesApiHttpService.scala (2)
114-124: Check user authorization before fetching sticky noteIn the
deleteStickyNotemethod, consider checking user authorization before fetching the sticky note from the database. This can improve performance by avoiding unnecessary database access when the user lacks the necessary permissions.
205-228: Simplify error handling inupdateStickyNotemethodThe
updateStickyNotemethod includes nested error handling withEitherTmonads. To enhance readability, consider simplifying the error handling logic or refactoring the method to reduce complexity.designer/server/src/test/scala/pl/touk/nussknacker/test/utils/domain/ScenarioHelper.scala (2)
85-90: Add explicit return type forupdateScenariomethodFor clarity and maintainability, consider adding an explicit return type to the
updateScenariomethod. This practice enhances code readability and helps with future modifications.
92-97: Add explicit return type foraddStickyNotemethodSimilarly, consider specifying the return type for the
addStickyNotemethod to improve code clarity and prevent potential misunderstandings about the method's return value.designer/client/src/components/graph/Graph.tsx (2)
495-495: Use strict equality operator for content comparisonIn the content comparison, replace
==with===to ensure strict equality checking without type coercion. This prevents unexpected behavior if types differ.
478-484: Consider centralizing sticky note dimension constraintsThe dimension constraints for sticky notes are applied in multiple places. To adhere to the DRY principle, consider centralizing these constraints in a utility function or constant to ensure consistency and ease of maintenance.
designer/client/src/types/stickyNote.ts (2)
1-1: Add JSDoc documentation for exported constantConsider adding documentation to describe the purpose and usage of this exported constant.
+/** Constant representing the type identifier for sticky notes in the system */ export const StickyNoteType = "StickyNote";
2-4: Add input validation and documentation for createStickyNoteId functionThe function should validate that noteId is non-negative and include JSDoc documentation explaining its purpose and parameters.
+/** + * Creates a unique identifier for a sticky note by combining the type and note ID + * @param noteId - The numeric identifier of the sticky note (must be non-negative) + * @returns A string in the format "StickyNote_<noteId>" + * @throws {Error} If noteId is negative + */ export function createStickyNoteId(noteId: number) { + if (noteId < 0) { + throw new Error("Sticky note ID must be non-negative"); + } return StickyNoteType + "_" + noteId; }designer/server/src/main/scala/db/migration/hsql/V1_060__CreateStickyNotes.scala (1)
6-8: Add Scaladoc documentation for the migration classConsider adding documentation to describe the purpose of this migration class and its relationship to sticky notes functionality.
+/** + * HSQLDB-specific implementation of sticky notes migration. + * Creates necessary database structures for storing sticky notes in HSQLDB. + */ class V1_060__CreateStickyNotes extends V1_060__CreateStickyNotesDefinition { override protected lazy val profile: JdbcProfile = HsqldbProfile }designer/server/src/main/scala/db/migration/postgres/V1_060__CreateStickyNotes.scala (1)
6-8: Add Scaladoc documentation for the PostgreSQL migration classConsider adding documentation to describe the purpose of this migration class and its relationship to sticky notes functionality.
+/** + * PostgreSQL-specific implementation of sticky notes migration. + * Creates necessary database structures for storing sticky notes in PostgreSQL. + */ class V1_060__CreateStickyNotes extends V1_060__CreateStickyNotesDefinition { override protected lazy val profile: JdbcProfile = PostgresProfile }designer/client/src/types/component.ts (1)
8-8: Add JSDoc comment to document the disabled property.The optional disabled property is well-typed and allows for dynamic component disabling. Consider adding documentation to explain when/why a component would be disabled.
+ /** Function that determines if the component should be disabled. Return true to disable the component. */ disabled?: () => boolean;designer/client/src/common/StickyNote.ts (1)
3-3: Consider using readonly properties for immutability.The Dimensions type could benefit from being readonly to prevent accidental modifications.
-export type Dimensions = { width: number; height: number }; +export type Dimensions = { readonly width: number; readonly height: number };designer/server/src/main/scala/pl/touk/nussknacker/ui/api/description/stickynotes/StickyNoteEvent.scala (1)
6-7: Consider adding custom error handling for JSON decoding.While the current implementation is correct, consider adding custom error handling for invalid event types during JSON decoding to provide more meaningful error messages.
implicit val typeDecoder: Decoder[StickyNoteEvent.Value] = Decoder.decodeString.emap { str => StickyNoteEvent.values.find(_.toString == str).toRight(s"Invalid StickyNoteEvent: $str") }designer/client/src/components/toolbars/creator/StickyNoteComponent.tsx (1)
4-4: Add type annotation for better type safety.The
noteModelconstant would benefit from explicit typing to ensure type safety and improve code maintainability.-const noteModel = { id: "StickyNoteToAdd", type: StickyNoteType, isDisabled: false }; +const noteModel: { id: string; type: typeof StickyNoteType; isDisabled: boolean } = { + id: "StickyNoteToAdd", + type: StickyNoteType, + isDisabled: false, +};designer/client/src/assets/json/nodeAttributes.json (1)
41-43: Consider maintaining alphabetical order for node typesThe StickyNote node type addition is correct and follows the established pattern. However, consider maintaining alphabetical order for better maintainability and easier navigation.
Consider moving the StickyNote entry to maintain alphabetical order with other node types:
"Split": { "name": "Split" }, + "StickyNote": { + "name": "StickyNote" + }, "Switch": { "name": "Choice" }, - "StickyNote": { - "name": "StickyNote" - },designer/client/cypress/e2e/creatorToolbar.cy.ts (1)
29-29: Consider expanding sticky notes test coverageWhile the basic toolbar interaction test is good, consider adding more comprehensive test cases for sticky notes functionality:
- Test sticky note creation
- Verify maximum notes limit
- Test content length restrictions
- Verify persistence of sticky notes
- Test deletion functionality
Example test structure:
it("should handle sticky notes operations", () => { // Test creation cy.contains(/^stickyNotes$/i).click(); cy.get("[data-testid=add-sticky-note]").click(); // Test content cy.get("[data-testid=sticky-note-content]") .type("Test note content") .should("have.value", "Test note content"); // Test persistence cy.reload(); cy.get("[data-testid=sticky-note-content]") .should("have.value", "Test note content"); // Test deletion cy.get("[data-testid=delete-sticky-note]").click(); cy.get("[data-testid=sticky-note-content]").should("not.exist"); });designer/client/src/containers/theme/helpers.ts (1)
19-26: Consider enhancing type safety and documentationWhile the implementation is solid, consider these improvements:
- Add JSDoc documentation to explain the function's purpose and parameters
- Consider using a more specific type for the color parameter (e.g., CSS color string)
Here's a suggested improvement:
+/** + * Gets the background color for a sticky note, with validation and fallback + * @param theme - The current theme object + * @param color - A valid CSS color string + * @returns An augmented color object from the theme's palette + */ -export function getStickyNoteBackgroundColor(theme: Theme, color: string) { +export function getStickyNoteBackgroundColor(theme: Theme, color: CSSColorString) {designer/client/src/actions/notificationActions.tsx (1)
15-15: Remove TODO comment from production codeDevelopment notes/TODOs should be tracked in issues rather than in the production codebase.
Would you like me to create a GitHub issue to track this discussion about the error function's completeness?
designer/client/src/components/graph/GraphPartialsInTS/cellUtils.ts (1)
16-21: Consider adding error handling for invalid noteId.The function handles the happy path well with proper null checks and immutable copies. However, consider adding error logging when noteId exists but no matching note is found, as this could indicate data inconsistency.
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); + if (noteId && !stickyNote) { + console.warn(`Sticky note with id ${noteId} not found in available notes`); + } return stickyNote ? cloneDeep(stickyNote) : undefined; }designer/client/src/components/graph/StickyNoteElement.ts (2)
17-18: Consider adding JSDoc documentationThe
StickyNoteElementfactory function would benefit from JSDoc documentation explaining its purpose and parameters.+/** + * Creates a new sticky note element type using JointJS's dia.Element.define + * @param defaults - Optional positioning, sizing, and attribute defaults + * @param protoProps - Optional prototype properties including markup + * @returns A new JointJS element type for sticky notes + */ export const StickyNoteElement = (defaults?: StickyNoteDefaults, protoProps?: StickyNoteProtoProps) => dia.Element.define("stickyNote.StickyNoteElement", defaults, protoProps);
38-45: Add keyboard event type and consider additional shortcutsThe keyboard event handling could be improved:
- selectAll: function (evt) { + selectAll: function (evt: KeyboardEvent) { if (evt.code === "KeyA") { if (evt.ctrlKey || evt.metaKey) { evt.preventDefault(); evt.target.select(); } } + // Consider adding Escape key handler to blur the textarea + if (evt.code === "Escape") { + (evt.target as HTMLTextAreaElement).blur(); + } },designer/client/src/actions/nk/assignSettings.ts (1)
42-42: Document the settings integrationAdd JSDoc comment to explain the sticky notes feature configuration.
+ /** Configuration for sticky notes feature including content length and count limits */ stickyNotesSettings: StickyNotesSettings;designer/client/src/components/graph/node-modal/node/FragmentContent.tsx (1)
17-17: Add TypeScript types and error handling for sticky notesWhile the implementation is functionally correct, consider these improvements:
- Add TypeScript type for the
stickyNotesprop- Add error handling or fallback for when sticky notes data is unavailable
Consider adding types and error handling:
+ import { StickyNote } from "../../../../types/stickyNote"; - const stickyNotes = useSelector(getStickyNotes); + const stickyNotes: StickyNote[] = useSelector(getStickyNotes) ?? []; <FragmentGraphPreview processCounts={fragmentCounts} scenario={fragmentContent} - stickyNotes={stickyNotes} + stickyNotes={stickyNotes} nodeIdPrefixForFragmentTests={getFragmentNodesPrefix(fragmentContent)} />Also applies to: 44-44
designer/server/src/test/scala/pl/touk/nussknacker/test/base/it/WithSimplifiedConfigScenarioHelper.scala (1)
44-50: Add test coverage and error handling documentationWhile the implementation is clean and follows the existing pattern, consider:
- Adding test cases demonstrating the usage of these new methods
- Documenting possible error scenarios and how they're handled
Consider adding ScalaDoc with error handling details:
+ /** + * Updates the scenario with a new version + * @param scenarioName name of the scenario to update + * @param newVersion new version of the scenario + * @return ProcessUpdated with update details + * @throws ProcessNotFoundError if scenario doesn't exist + */ def updateScenario(scenarioName: ProcessName, newVersion: CanonicalProcess): ProcessUpdated = { rawScenarioHelper.updateScenario(scenarioName, newVersion) } + /** + * Adds a sticky note to the scenario + * @param scenarioName name of the scenario + * @param request sticky note details + * @return correlation ID of the created sticky note + * @throws ProcessNotFoundError if scenario doesn't exist + */ def addStickyNote(scenarioName: ProcessName, request: StickyNoteAddRequest): StickyNoteCorrelationId = { rawScenarioHelper.addStickyNote(scenarioName, request) }designer/client/src/components/StickyNotePreview.tsx (2)
9-11: Consider documenting magic numbersWhile these constants are used for visual effects, adding comments explaining the visual impact of these specific values would improve maintainability.
+// Scale factor for the preview state of sticky notes const PREVIEW_SCALE = 0.9; +// Rotation angle (in degrees) when the note is active const ACTIVE_ROTATION = 2; +// Scale factor for inactive state const INACTIVE_SCALE = 1.5;
13-62: Add accessibility attributes to interactive elementsThe component might be interactive but lacks accessibility attributes. Consider adding appropriate ARIA attributes and keyboard interactions.
export function StickyNotePreview({ isActive, isOver }: { isActive?: boolean; isOver?: boolean }): JSX.Element { // ... existing code ... return ( - <div className={cx(colors, nodeStyles)}> + <div + className={cx(colors, nodeStyles)} + role="button" + tabIndex={0} + aria-label="Sticky note preview" + aria-pressed={isActive} + > <div className={cx(imageStyles, colors)}> <PreloadedIcon src={stickyNoteIconSrc} /> </div> </div> ); }designer/client/src/components/graph/GraphPartialsInTS/applyCellChanges.ts (1)
46-57: Replace console statements with proper loggingConsider using a proper logging service instead of direct console calls. Also, the error handling block could be extracted into a separate function for better maintainability.
+const addToolsToStickyNote = (model: ModelWithTool, paper: dia.Paper) => { + const view = model.model.findView(paper); + if (!view) { + logger.warn(`View not found for stickyNote model: ${model.model.id}`); + return; + } + view.addTools(model.tools); +}; - newStickyNotesModelsWithTools.forEach((m) => { - try { - const view = m.model.findView(paper); - if (!view) { - console.warn(`View not found for stickyNote model: ${m.model.id}`); - return; - } - view.addTools(m.tools); - } catch (error) { - console.error(`Failed to add tools to stickyNote view:`, error); - } - }); + newStickyNotesModelsWithTools.forEach((model) => { + try { + addToolsToStickyNote(model, paper); + } catch (error) { + logger.error(`Failed to add tools to stickyNote view:`, error); + } + });designer/client/src/components/graph/types.ts (2)
77-79: Consider grouping related eventsThe Events enum is growing large. Consider grouping related events (like cell events) into separate enums or using a namespace pattern for better organization.
-export enum Events { +export namespace Events { + export enum Cell { + RESIZED = "cellCustom:resized", + CONTENT_UPDATED = "cellCustom:contentUpdated", + DELETED = "cellCustom:deleted", + // ... other cell events + } + + export enum Link { + CONNECT = "link:connect", + // ... other link events + } + + export enum Blank { + POINTERCLICK = "blank:pointerclick", + // ... other blank events + } +}
Line range hint
19-41: Consider splitting ScenarioGraphProps into smaller interfacesThe ScenarioGraphProps type is growing with many responsibilities. Consider splitting it into smaller, focused interfaces for better maintainability.
interface StickyNoteActions { stickyNoteAdded: typeof stickyNoteAdded; stickyNoteUpdated: typeof stickyNoteUpdated; stickyNoteDeleted: typeof stickyNoteDeleted; } interface NodeActions { nodesConnected: typeof nodesConnected; nodesDisconnected: typeof nodesDisconnected; nodeAdded: typeof nodeAdded; // ... other node actions } interface ScenarioGraphProps extends StickyNoteActions, NodeActions { // ... remaining props }designer/server/src/test/scala/pl/touk/nussknacker/ui/api/StickyNotesApiHttpServiceBusinessSpec.scala (1)
42-43: Enhance test data helper method.The
stickyNoteToAddhelper method uses hardcoded values which might not cover all edge cases. Consider:
- Adding parameters for content and color
- Including validation boundary cases
- Adding documentation
- private def stickyNoteToAdd(versionId: VersionId): StickyNoteAddRequest = - StickyNoteAddRequest(versionId, "", LayoutData(0, 1), "#aabbcc", Dimensions(300, 200), None) + /** Creates a StickyNoteAddRequest with customizable parameters for testing + * @param versionId The version ID of the scenario + * @param content Note content (defaults to empty string) + * @param color Note color in hex format (defaults to "#aabbcc") + * @return StickyNoteAddRequest instance + */ + private def stickyNoteToAdd( + versionId: VersionId, + content: String = "", + color: String = "#aabbcc" + ): StickyNoteAddRequest = + StickyNoteAddRequest( + versionId, + content, + LayoutData(0, 1), + color, + Dimensions(300, 200), + None + )designer/client/src/components/toolbars/creator/ToolboxComponentGroup.tsx (1)
76-82: Consider enhancing type safety for the disabled prop.While the implementation is functional, consider these improvements:
- Add TypeScript type guard for the disabled function
- Add unit tests for the disabled state
- disabled={component.disabled ? component.disabled() : false} + disabled={typeof component.disabled === 'function' ? component.disabled() : false}designer/server/src/main/scala/pl/touk/nussknacker/ui/config/FeatureTogglesConfig.scala (2)
32-33: Consider grouping related configuration fields togetherThe
stickyNotesSettingsfield seems arbitrarily placed. Consider grouping it with other UI-related settings likecommentSettingsanddeploymentCommentSettingsfor better code organization.final case class FeatureTogglesConfig( development: Boolean, metrics: Option[MetricsSettings], remoteEnvironment: Option[HttpRemoteEnvironmentConfig], counts: Option[Config], environmentAlert: Option[EnvironmentAlert], commentSettings: Option[CommentSettings], deploymentCommentSettings: Option[DeploymentCommentSettings], + stickyNotesSettings: StickyNotesSettings, scenarioLabelConfig: Option[ScenarioLabelConfig], scenarioStateTimeout: Option[FiniteDuration], surveySettings: Option[SurveySettings], tabs: Option[List[TopTab]], intervalTimeSettings: IntervalTimeSettings, testDataSettings: TestDataSettings, enableConfigEndpoint: Boolean, redirectAfterArchive: Boolean, - componentDefinitionExtractionMode: ComponentDefinitionExtractionMode, - stickyNotesSettings: StickyNotesSettings + componentDefinitionExtractionMode: ComponentDefinitionExtractionMode )
61-62: Add documentation for the default settings behaviorConsider adding a comment explaining the default behavior when
stickyNotesSettingsis not provided in the configuration.+ // If stickyNotesSettings is not provided in config, use default settings + // Default settings can be found in StickyNotesSettings.default val stickyNotesSettings = config.getAs[StickyNotesSettings]("stickyNotesSettings").getOrElse(StickyNotesSettings.default)designer/client/src/components/toolbars/creator/ToolBox.tsx (1)
124-128: Verify memoization dependencies and add type annotationsThe memoization implementation looks good, but consider these improvements:
- Add type annotation for
stickyNoteToolGroup- Verify if
t(translation function) is needed in the dependency array- const stickyNoteToolGroup = useMemo(() => stickyNoteComponentGroup(pristine), [pristine, props, t]); + const stickyNoteToolGroup = useMemo(() => stickyNoteComponentGroup(pristine), [pristine]); - const groups = useMemo(() => { + const groups: ComponentGroup[] = 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 (2)
75-77: Consider grouping related selectors and adding explicit return typeThe sticky notes selector implementation is correct, but consider these improvements:
- Group it with other UI-related selectors for better code organization
- Add explicit return type annotation for better type safety
-export const getStickyNotes = createSelector([getGraph, getStickyNotesSettings], (g, settings) => - settings.enabled ? g.stickyNotes || ([] as StickyNote[]) : ([] as StickyNote[]), -); +export const getStickyNotes = createSelector( + [getGraph, getStickyNotesSettings], + (g, settings): StickyNote[] => settings.enabled ? g.stickyNotes || [] : [], +);
11-12: Group related imports togetherConsider grouping the sticky notes related imports with other feature-specific imports for better code organization.
import { ProcessCounts } from "../graph"; import { RootState } from "../index"; import { getProcessState } from "./scenarioState"; import { TestFormParameters } from "../../common/TestResultUtils"; +// Feature-specific imports import { StickyNote } from "../../common/StickyNote"; import { getStickyNotesSettings } from "./settings";designer/server/src/main/scala/pl/touk/nussknacker/ui/api/SettingsResources.scala (1)
46-47: Consider adding validation for stickyNotesSettingsWhile the configuration is correctly passed through, consider adding validation to ensure stickyNotesSettings is properly initialized with valid values.
// Add validation similar to DeploymentCommentSettings object StickyNotesSettings { def create( maxContentLength: Int, maxNotesCount: Int, enabled: Boolean ): Validated[InvalidStickyNotesSettingsError, StickyNotesSettings] = { Validated.cond( maxContentLength > 0 && maxNotesCount > 0, new StickyNotesSettings(maxContentLength, maxNotesCount, enabled), InvalidStickyNotesSettingsError("maxContentLength and maxNotesCount must be positive") ) } }designer/client/src/components/graph/NodeDescriptionPopover.tsx (1)
123-123: Enhance the comment for better clarityWhile the logic is correct, the comment could be more descriptive.
- if (isStickyNoteElement(view.model)) return; //Dont use it for stickyNotes + if (isStickyNoteElement(view.model)) return; // Skip description popover for sticky notes as they have their own display mechanismdesigner/client/src/components/graph/graphStyledWrapper.ts (1)
184-216: Consider enhancing the sticky note stylesA few suggestions to improve the styling:
- Add transitions for smoother interactions
- Extract magic numbers into theme spacing units
- Consider adding hover states for better UX
Consider applying these improvements:
".sticky-note-markdown": { width: "100%", height: "100%", - paddingLeft: "10px", - paddingRight: "10px", + paddingLeft: theme.spacing(1.25), + paddingRight: theme.spacing(1.25), + transition: theme.transitions.create(['background-color', 'box-shadow']), },designer/client/src/reducers/graph/utils.ts (1)
82-93: Consider optimizing layout updatesThe current implementation creates new arrays for both layout and sticky notes. For large datasets, this could be optimized.
Consider using Set or Map for O(1) lookups:
export function removeStickyNoteFromLayout(state: GraphState, stickyNoteId: number): { layout: NodePosition[]; stickyNotes: StickyNote[] } { const { layout } = state; const stickyNoteLayoutId = createStickyNoteId(stickyNoteId); + const layoutMap = new Map(layout.map(l => [l.id, l])); + layoutMap.delete(stickyNoteLayoutId); const updatedStickyNotes = state.stickyNotes.filter((n) => n.noteId !== stickyNoteId); - const updatedLayout = updatedStickyNotes.map((stickyNote) => { - return { id: createStickyNoteId(stickyNote.noteId), position: stickyNote.layoutData }; - }); + const updatedLayout = Array.from(layoutMap.values()); return { stickyNotes: [...updatedStickyNotes], - layout: [...layout.filter((l) => l.id !== stickyNoteLayoutId), ...updatedLayout], + layout: updatedLayout, }; }designer/client/src/http/HttpService.ts (2)
721-738: Add validation for sticky note content length.The
updateStickyNotemethod should validate the content length before making the API call to ensure it meets any backend restrictions.Consider adding validation:
updateStickyNote(scenarioName: string, scenarioVersionId: number, stickyNote: StickyNote) { + const maxContentLength = 1000; // Get from config + if (stickyNote.content && stickyNote.content.length > maxContentLength) { + return Promise.reject(new Error(`Content exceeds maximum length of ${maxContentLength} characters`)); + } const promise = api.put(...); ... }
740-746: Consider adding caching for sticky notes.The
getStickyNotesmethod could benefit from caching to reduce API calls, especially if sticky notes are frequently accessed but rarely updated.Consider implementing a caching mechanism with a short TTL to improve performance.
docs-internal/api/nu-designer-openapi.yaml (2)
2530-2843: REST API endpoints for sticky notes look good but could use additional documentation.The implementation follows REST conventions and includes proper authentication, authorization, and error handling. However, consider the following improvements:
- Add description for why
scenarioVersionIdis required in the GET endpoint- Document specific validation error scenarios in the 400 response examples, such as:
- Maximum content length exceeded
- Maximum notes count reached
- Invalid color format
- Invalid dimensions
Example documentation to add for the GET endpoint's
scenarioVersionIdparameter:- name: scenarioVersionId in: query required: true + description: Version ID of the scenario to retrieve sticky notes from. This ensures notes are retrieved for the correct version as they may differ between versions. schema: type: integer format: int64
7182-7267: Schema definitions could be enhanced with additional constraints.The schema definitions are well-structured but could benefit from additional validation constraints:
- Add format validation for the color field (e.g., hex color)
- Add min/max constraints for dimensions
- Add maxLength constraint for content field
Example improvements:
StickyNote: properties: content: type: string + maxLength: 5000 color: type: string + pattern: '^#[0-9A-Fa-f]{6}$' Dimensions: properties: width: type: integer format: int64 + minimum: 50 + maximum: 1000 height: type: integer format: int64 + minimum: 50 + maximum: 1000Also applies to: 5492-5504
docs/MigrationGuide.md (1)
7-14: Consider adding version compatibility matrixThe migration guide would benefit from having a compatibility matrix at the top showing which versions are compatible with different Flink versions, Scala versions, and other major dependencies. This would help users quickly determine the right upgrade path.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
⛔ Files ignored due to path filters (3)
designer/client/cypress/e2e/__image_snapshots__/electron/Linux/Creator toolbar should allow collapse (persist) and filtering #0.pngis excluded by!**/*.pngdesigner/client/package-lock.jsonis excluded by!**/package-lock.jsondesigner/server/src/main/resources/web/static/assets/components/StickyNote.svgis excluded by!**/*.svg
📒 Files selected for processing (59)
designer/client/cypress/e2e/creatorToolbar.cy.ts(1 hunks)designer/client/package.json(1 hunks)designer/client/src/actions/actionTypes.ts(1 hunks)designer/client/src/actions/nk/assignSettings.ts(1 hunks)designer/client/src/actions/nk/process.ts(3 hunks)designer/client/src/actions/notificationActions.tsx(1 hunks)designer/client/src/assets/json/nodeAttributes.json(1 hunks)designer/client/src/common/StickyNote.ts(1 hunks)designer/client/src/components/ComponentPreview.tsx(2 hunks)designer/client/src/components/StickyNotePreview.tsx(1 hunks)designer/client/src/components/graph/EspNode/stickyNote.ts(1 hunks)designer/client/src/components/graph/EspNode/stickyNoteElements.ts(1 hunks)designer/client/src/components/graph/Graph.tsx(13 hunks)designer/client/src/components/graph/GraphPartialsInTS/applyCellChanges.ts(2 hunks)designer/client/src/components/graph/GraphPartialsInTS/cellUtils.ts(2 hunks)designer/client/src/components/graph/NodeDescriptionPopover.tsx(2 hunks)designer/client/src/components/graph/ProcessGraph.tsx(4 hunks)designer/client/src/components/graph/StickyNoteElement.ts(1 hunks)designer/client/src/components/graph/fragmentGraph.tsx(1 hunks)designer/client/src/components/graph/graphStyledWrapper.ts(2 hunks)designer/client/src/components/graph/node-modal/node/FragmentContent.tsx(3 hunks)designer/client/src/components/graph/types.ts(3 hunks)designer/client/src/components/graph/utils/graphUtils.ts(1 hunks)designer/client/src/components/toolbars/creator/ComponentIcon.tsx(2 hunks)designer/client/src/components/toolbars/creator/StickyNoteComponent.tsx(1 hunks)designer/client/src/components/toolbars/creator/ToolBox.tsx(2 hunks)designer/client/src/components/toolbars/creator/ToolboxComponentGroup.tsx(1 hunks)designer/client/src/containers/theme/helpers.ts(2 hunks)designer/client/src/http/HttpService.ts(3 hunks)designer/client/src/reducers/graph/reducer.ts(2 hunks)designer/client/src/reducers/graph/types.ts(2 hunks)designer/client/src/reducers/graph/utils.ts(3 hunks)designer/client/src/reducers/selectors/graph.ts(2 hunks)designer/client/src/reducers/selectors/settings.ts(1 hunks)designer/client/src/types/component.ts(1 hunks)designer/client/src/types/node.ts(1 hunks)designer/client/src/types/stickyNote.ts(1 hunks)designer/server/src/main/resources/defaultDesignerConfig.conf(1 hunks)designer/server/src/main/scala/db/migration/V1_060__CreateStickyNotesDefinition.scala(1 hunks)designer/server/src/main/scala/db/migration/hsql/V1_060__CreateStickyNotes.scala(1 hunks)designer/server/src/main/scala/db/migration/postgres/V1_060__CreateStickyNotes.scala(1 hunks)designer/server/src/main/scala/pl/touk/nussknacker/ui/api/SettingsResources.scala(3 hunks)designer/server/src/main/scala/pl/touk/nussknacker/ui/api/StickyNotesApiHttpService.scala(1 hunks)designer/server/src/main/scala/pl/touk/nussknacker/ui/api/description/StickyNotesApiEndpoints.scala(1 hunks)designer/server/src/main/scala/pl/touk/nussknacker/ui/api/description/stickynotes/Dtos.scala(1 hunks)designer/server/src/main/scala/pl/touk/nussknacker/ui/api/description/stickynotes/StickyNoteEvent.scala(1 hunks)designer/server/src/main/scala/pl/touk/nussknacker/ui/config/FeatureTogglesConfig.scala(4 hunks)designer/server/src/main/scala/pl/touk/nussknacker/ui/db/NuTables.scala(1 hunks)designer/server/src/main/scala/pl/touk/nussknacker/ui/db/entity/StickyNotesEntityFactory.scala(1 hunks)designer/server/src/main/scala/pl/touk/nussknacker/ui/process/repository/stickynotes/DbStickyNotesRepository.scala(1 hunks)designer/server/src/main/scala/pl/touk/nussknacker/ui/process/repository/stickynotes/StickyNotesRepository.scala(1 hunks)designer/server/src/main/scala/pl/touk/nussknacker/ui/server/AkkaHttpBasedRouteProvider.scala(4 hunks)designer/server/src/test/resources/config/common-designer.conf(1 hunks)designer/server/src/test/scala/pl/touk/nussknacker/test/base/it/WithSimplifiedConfigScenarioHelper.scala(2 hunks)designer/server/src/test/scala/pl/touk/nussknacker/test/utils/domain/ScenarioHelper.scala(4 hunks)designer/server/src/test/scala/pl/touk/nussknacker/ui/api/StickyNotesApiHttpServiceBusinessSpec.scala(1 hunks)docs-internal/api/nu-designer-openapi.yaml(3 hunks)docs/Changelog.md(1 hunks)docs/MigrationGuide.md(1 hunks)
🧰 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 (37)
designer/server/src/main/scala/pl/touk/nussknacker/ui/process/repository/stickynotes/StickyNotesRepository.scala (1)
48-48:
Remove unnecessary trailing comma in method parameters.
The trailing comma after scenarioVersionId in the updateStickyNote method may cause a syntax error.
Apply this diff to fix the syntax error:
scenarioVersionId: VersionId
- )(implicit user: LoggedUser): DB[Int]
+ )(implicit user: LoggedUser): DB[Int]Likely invalid or redundant comment.
designer/client/src/components/graph/EspNode/stickyNote.ts (1)
67-71: Ensure proper sanitization and error handling in Markdown parsing.
While using DOMPurify and marked is good for sanitization, consider handling errors in a user-friendly way and ensure all potential security concerns are addressed.
Run the following script to check for any vulnerabilities in the usage of marked and DOMPurify:
✅ Verification successful
Let me try a different approach to analyze the code and its dependencies.
Let me check the renderer configuration and any additional security settings.
Current implementation has proper security measures in place
The code implements adequate security measures:
- Uses DOMPurify to sanitize HTML output with specific allowance for "target" attribute
- Custom renderer properly handles links with security attributes (
target="_blank"andrel="noopener noreferrer") - Error handling provides user feedback without exposing sensitive information
- Markdown parsing is isolated to sticky note content only
- No other instances of marked/DOMPurify usage that could create inconsistencies
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Check for known vulnerabilities in 'marked' and 'dompurify' packages.
# Check 'marked' package for vulnerabilities
npm audit --json | jq '.advisories[] | select(.module_name=="marked")'
# Check 'dompurify' package for vulnerabilities
npm audit --json | jq '.advisories[] | select(.module_name=="dompurify")'
Length of output: 962
Script:
#!/bin/bash
# Check how marked and DOMPurify are imported and configured
rg -A 5 "import.*\{?(marked|DOMPurify)\}?" designer/client/src/
# Look for any custom renderer configurations
ast-grep --pattern 'const renderer = new marked.Renderer()'
# Check for any security-related configurations
rg -B 2 -A 5 "DOMPurify.sanitize"
Length of output: 1894
Script:
#!/bin/bash
# Check the renderer configuration and any security-related settings
rg -B 10 -A 10 "const renderer = new marked.Renderer\(\)" designer/client/src/components/graph/EspNode/stickyNote.ts
# Look for any marked configuration
rg "marked\.setOptions|marked\.defaults" designer/client/src/
# Check if there are any other uses of marked or DOMPurify in the codebase
rg "marked\.parse|DOMPurify\.sanitize" designer/client/src/
Length of output: 1027
designer/server/src/main/scala/db/migration/hsql/V1_060__CreateStickyNotes.scala (1)
1-8: Verify migration script compatibility
The migration script introduces sticky notes functionality. We should verify its compatibility with existing data and rollback capabilities.
designer/server/src/main/scala/db/migration/postgres/V1_060__CreateStickyNotes.scala (1)
1-8: Verify PR checklist requirements
Based on the PR objectives, please ensure:
- Documentation is added to
_MigrationGuide.md_as this is a database schema change - An entry is added to
_Changelog.md_describing the sticky notes feature - The changes are covered by automated tests
designer/client/src/common/StickyNote.ts (1)
5-15: 🛠️ Refactor suggestion
Consider type improvements for better type safety.
A few suggestions to enhance type safety and validation:
- Use Date type for editedAt
- Add color type validation
- Consider adding content length validation based on maxContentLength setting
export interface StickyNote {
id?: string;
noteId: number;
content: string;
layoutData: LayoutData;
dimensions: Dimensions;
- color: string;
+ color: `#${string}` | string;
targetEdge?: string;
editedBy: string;
- editedAt: string;
+ editedAt: Date;
}Let's verify if there are any color validation patterns used elsewhere:
designer/server/src/main/scala/pl/touk/nussknacker/ui/db/NuTables.scala (1)
14-15: Verify initialization order of entity factories.
The addition of StickyNotesEntityFactory follows the established pattern. However, let's verify there are no initialization order dependencies between the entity factories.
✅ Verification successful
Let me gather more specific information about these entity factories.
Let me check for any initialization methods or dependencies between these entity factories.
Based on the analysis of the codebase, I can now generate the final response:
No initialization order concerns with StickyNotesEntityFactory
The entity factories in this codebase follow a simple trait-based composition pattern where:
- All entity factories extend
BaseEntityFactory - None of the factories have initialization methods that could cause order dependencies
- StickyNotesEntityFactory only has a dependency on ProcessVersionEntityFactory's table, which is properly handled through the trait composition in NuTables
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for dependencies between entity factories
rg -A 5 "extends.*EntityFactory.*with.*EntityFactory" --type scala
# Search for initialization order requirements
rg "override.*initialize" --type scala
Length of output: 526
Script:
#!/bin/bash
# Search for EntityFactory trait/class definitions and implementations
ast-grep --pattern 'trait $_EntityFactory {
$$$
}'
# Also check for any abstract or concrete methods in these factories
rg "EntityFactory" -B 2 -A 5 --type scala
Length of output: 54715
Script:
#!/bin/bash
# Search for any initialization methods in entity factories
ast-grep --pattern 'trait $_EntityFactory {
$$$
def initialize($$$) {
$$$
}
$$$
}'
# Check for any dependencies between tables in StickyNotesEntityFactory
rg -A 10 "trait StickyNotesEntityFactory" --type scala
Length of output: 1709
designer/server/src/main/scala/pl/touk/nussknacker/ui/api/description/stickynotes/StickyNoteEvent.scala (1)
5-13: LGTM! Well-structured enumeration for sticky note events.
The implementation follows Scala best practices with clear, descriptive event types that cover all necessary CRUD operations.
designer/client/src/reducers/graph/types.ts (1)
17-17: LGTM! Well-placed type addition.
The optional stickyNotes array is appropriately typed and placed within the GraphState interface. The optional typing ensures backward compatibility.
designer/client/src/components/graph/fragmentGraph.tsx (1)
7-24: LGTM! Clean implementation of sticky notes support
The changes properly integrate sticky notes into the FragmentGraphPreview component while maintaining type safety and following React best practices. The use of Pick utility type ensures proper prop typing, and the props are correctly forwarded to the GraphWrapped component.
designer/client/src/actions/actionTypes.ts (1)
13-14: LGTM! Action types properly defined for sticky notes feature
The new action types follow the existing naming convention and clearly represent their intended operations.
designer/client/src/actions/notificationActions.tsx (1)
16-20: 🛠️ Refactor suggestion
Improve error handling implementation
Several concerns with the current implementation:
- The error message logic could be more explicit
- Type safety could be improved
- The function's behavior change might affect existing usages
Consider this improved implementation:
-export function error(message: string, error?: string, showErrorText?: boolean): Action {
+type ErrorMessage = {
+ displayMessage: string;
+ technicalDetails?: string;
+ showTechnicalDetails?: boolean;
+};
+
+export function error(params: string | ErrorMessage): Action {
+ const errorDetails = typeof params === 'string'
+ ? { displayMessage: params }
+ : params;
+
return Notifications.error({
autoDismiss: 10,
- children: <Notification type={"error"} icon={<InfoOutlinedIcon />} message={showErrorText && error ? error : message} />,
+ children: <Notification
+ type={"error"}
+ icon={<InfoOutlinedIcon />}
+ message={errorDetails.showTechnicalDetails && errorDetails.technicalDetails
+ ? errorDetails.technicalDetails
+ : errorDetails.displayMessage
+ }
+ />,
});
}Let's verify existing usages to ensure backward compatibility:
designer/client/src/components/graph/GraphPartialsInTS/cellUtils.ts (1)
12-14: LGTM! Well-implemented type guard function.
The type guard follows TypeScript best practices and provides a clear, single-responsibility check for sticky note elements.
designer/client/src/types/node.ts (1)
3-3: LGTM! Clean type extension.
The addition of StickyNoteType to the union type is clean and maintains type safety. The import is properly scoped.
Also applies to: 5-5
designer/server/src/test/resources/config/common-designer.conf (1)
57-61: Consider increasing maxNotesCount and documenting limits.
The sticky notes configuration looks good overall, but:
- The limit of 5 notes might be too restrictive for complex scenarios
- The configuration values should be documented to explain their impact
Consider:
- Increasing
maxNotesCountto a more practical limit (e.g., 20) - Adding comments to explain the rationale behind these limits
stickyNotesSettings: {
+ # Maximum length of content in a single sticky note
maxContentLength: 5000,
+ # Maximum number of sticky notes per scenario
- maxNotesCount: 5,
+ maxNotesCount: 20,
+ # Feature toggle for sticky notes functionality
enabled: true
}designer/client/src/components/graph/StickyNoteElement.ts (1)
6-15: LGTM: Well-structured interfaces for sticky note configuration
The interfaces StickyNoteDefaults and StickyNoteProtoProps provide clear type definitions for configuring sticky notes. The optional properties in StickyNoteDefaults allow for flexible initialization.
designer/client/src/reducers/selectors/settings.ts (1)
17-17: LGTM: Consistent selector implementation
The getStickyNotesSettings selector follows the established pattern and is correctly implemented using createSelector.
designer/client/src/actions/nk/assignSettings.ts (1)
42-49: Verify settings validation and error handling
Ensure that the settings are properly validated and enforced across the stack:
- Backend API validation
- Frontend error handling
- User feedback for limit violations
✅ Verification successful
Settings validation and error handling are properly implemented
The codebase shows comprehensive validation and error handling for sticky notes settings:
-
Backend validation in
StickyNotesApiHttpService.scala:- Enforces
maxNotesCountlimit - Validates content length against
maxContentLength - Returns appropriate error types (
StickyNoteContentTooLong)
- Enforces
-
Frontend integration:
- Settings are properly propagated through Redux selectors
- Feature toggle respects the
enabledflag - Default settings are defined in backend (
StickyNotesSettings.default)
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for settings validation in backend
rg "StickyNotesSettings.*validate" --type scala
# Search for error handling in frontend
rg "maxNotesCount.*error" --type ts
rg "maxContentLength.*error" --type ts
Length of output: 128
Script:
#!/bin/bash
# Search for sticky notes settings usage and validation patterns
rg -A 5 "StickyNotesSettings" --type ts --type scala
# Search for any validation or error handling related to sticky notes
rg -A 5 "stickyNotes.*validation|stickyNotes.*error" --type ts --type scala
# Look for API endpoints or handlers related to sticky notes
ast-grep --pattern 'class $_ {
$$$
stickyNotes$_($$$) {
$$$
}
$$$
}'
Length of output: 12800
designer/client/src/components/toolbars/creator/ComponentIcon.tsx (1)
26-29: LGTM! Sticky note icon implementation follows existing patterns
The implementation correctly follows the established patterns:
- Uses memoized preloading via
preloadBeImage - Maintains consistency with existing icon path structure
- Exports necessary functions for reuse
designer/server/src/test/scala/pl/touk/nussknacker/test/base/it/WithSimplifiedConfigScenarioHelper.scala (1)
48-50: Verify sticky note validation
Ensure that the rawScenarioHelper.addStickyNote method validates the sticky note content according to the configured limits.
✅ Verification successful
Sticky note validation is properly implemented
The validation is already implemented in the StickyNotesApiHttpService. The service performs two validations before adding a sticky note:
- Content length validation through
validateStickyNoteContentwhich checks againstmaxContentLength(default: 5000 characters) - Notes count validation through
validateStickyNotesCountwhich checks againstmaxNotesCount(default: 5 notes)
These validations are enforced before any sticky note is added to the system, with proper error types defined in StickyNotesError.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for sticky note validation logic
rg -A 5 "StickyNoteAddRequest.*validate"
Length of output: 42
Script:
#!/bin/bash
# Let's first find all files related to StickyNoteAddRequest
rg -l "StickyNoteAddRequest"
Length of output: 651
Script:
#!/bin/bash
# Let's also search for any validation related to sticky notes
rg -A 5 "StickyNote.*validat"
Length of output: 31
Script:
#!/bin/bash
# Let's look at the implementation of StickyNoteAddRequest
ast-grep --pattern 'case class StickyNoteAddRequest'
Length of output: 54
Script:
#!/bin/bash
# Let's examine the content of the Dtos file which likely contains the StickyNoteAddRequest definition
cat "designer/server/src/main/scala/pl/touk/nussknacker/ui/api/description/stickynotes/Dtos.scala"
Length of output: 4535
Script:
#!/bin/bash
# Let's also check the service implementation
cat "designer/server/src/main/scala/pl/touk/nussknacker/ui/api/StickyNotesApiHttpService.scala"
Length of output: 7864
designer/client/src/components/ComponentPreview.tsx (1)
10-11: LGTM: Import statements are properly organized.
The new imports are correctly placed and follow the project's import organization pattern.
designer/client/src/components/graph/ProcessGraph.tsx (3)
87-89: LGTM: Action creators properly integrated
The sticky note actions are correctly bound to dispatch, maintaining consistency with other actions.
104-104: LGTM: GraphWrapped component properly configured
The stickyNotes prop is correctly passed to GraphWrapped component.
49-54: 🛠️ Refactor suggestion
Verify drop handler's error handling
The drop handler now handles sticky notes differently from regular nodes, but it lacks error handling for invalid positions or failed sticky note creation.
Consider adding error handling:
if (item?.type === StickyNoteType) {
+ try {
graph.current.addStickyNote(scenario.name, scenario.processVersionId, mapValues(nodeInputRelOffset, Math.round));
+ } catch (error) {
+ console.error("Failed to add sticky note:", error);
+ // Consider showing user feedback
+ }
} else {designer/client/src/components/graph/utils/graphUtils.ts (1)
130-130: LGTM: Improved event handling flexibility
The addition of the preventDefault parameter with a sensible default value improves the flexibility of the event handler while maintaining backward compatibility.
Also applies to: 133-133
designer/server/src/main/scala/pl/touk/nussknacker/ui/api/SettingsResources.scala (2)
10-10: LGTM: Import statement is correctly placed
The import for StickyNotesSettings follows the package structure and naming conventions.
139-139: LGTM: Case class field addition is properly placed
The stickyNotesSettings field is correctly added to the ToggleFeaturesOptions case class.
designer/client/src/components/graph/NodeDescriptionPopover.tsx (1)
8-8: LGTM: Import statement is correctly placed
The import for isStickyNoteElement follows the project's import conventions.
designer/server/src/main/resources/defaultDesignerConfig.conf (1)
213-217: 🛠️ Refactor suggestion
Review and adjust the sticky notes limits
A few concerns about the configuration:
- Remove the unnecessary comma after maxContentLength
- Consider documenting the reasoning behind these limits
- The maxNotesCount of 5 might be too restrictive for complex scenarios
stickyNotesSettings: {
- maxContentLength: 5000,
+ maxContentLength: 5000 # Maximum characters per note
- maxNotesCount: 5
+ maxNotesCount: 10 # Maximum notes per scenario
enabled: true
}✅ Verification successful
Let me gather more information about sticky notes usage in the codebase to better understand the context and validate the limits.
Let me gather more information about sticky notes usage patterns to better understand if the limits are reasonable.
Based on the previous findings and the lack of specific usage patterns in tests and documentation, I can now make a final assessment about the sticky notes configuration.
Review and adjust the sticky notes configuration
- The comma after
maxContentLengthis indeed a syntax error and should be removed - The
maxContentLengthof 5000 characters is reasonable as no existing sticky notes exceed this limit - The
maxNotesCountof 5 is a reasonable default since:- It helps maintain UI cleanliness
- No evidence was found of use cases requiring more notes
- Users can still create multiple notes across different scenarios
stickyNotesSettings: {
- maxContentLength: 5000,
+ maxContentLength: 5000 # Maximum characters per sticky note
maxNotesCount: 5 # Maximum notes per scenario
enabled: true
}🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Check if there are any existing sticky notes in the codebase that might exceed these limits
rg -l "stickyNote.*content.*" | while read -r file; do
echo "Checking $file for content length..."
rg -U "content[\"']:\s*[\"']([^\"']*)[\"']" "$file" | awk '{if (length($0) > 5000) print "Warning: Content exceeds 5000 chars in " FILENAME}'
done
Length of output: 1518
Script:
#!/bin/bash
# Check sticky notes related code for any hints about usage patterns
echo "=== Checking sticky notes implementation ==="
rg -A 5 "class.*StickyNote"
echo -e "\n=== Checking for any existing sticky note count limits ==="
rg -i "maxNotes|notesCount|stickyNotes?.length"
echo -e "\n=== Checking for any sticky note content validation ==="
rg -i "content.*length|maxContent|contentLength"
Length of output: 82109
Script:
#!/bin/bash
# Check for any sticky notes related tests or usage patterns
echo "=== Checking sticky notes test cases ==="
rg -A 5 "stickyNote.*test"
echo -e "\n=== Checking sticky notes documentation ==="
rg -A 5 "stickyNote.*description|stickyNote.*doc"
echo -e "\n=== Checking sticky notes UI components ==="
rg -A 5 "class.*StickyNote.*Component|const.*StickyNote.*Component"
Length of output: 447
designer/client/src/http/HttpService.ts (1)
695-707: Consider making the default color configurable.
The default sticky note color is hardcoded. As indicated by the TODO comment, consider making this configurable through application settings.
designer/client/src/reducers/graph/reducer.ts (2)
19-19: LGTM: Clean import additions
The new utility imports for sticky notes management follow the existing import structure and naming conventions.
Also applies to: 23-24
244-255: LGTM: Well-structured reducer cases for sticky notes
The implementation follows Redux best practices:
- Maintains immutability using spread operator
- Delegates complex operations to utility functions
- Consistent with existing reducer patterns
designer/server/src/main/scala/pl/touk/nussknacker/ui/server/AkkaHttpBasedRouteProvider.scala (4)
72-72: LGTM: Clean import addition
The import for DbStickyNotesRepository follows the existing import structure.
171-171: LGTM: Repository initialization
The sticky notes repository initialization follows the established pattern of other repositories in the codebase.
402-409: LGTM: Well-structured service initialization
The StickyNotesApiHttpService initialization:
- Follows dependency injection pattern
- Includes all necessary dependencies
- Properly integrates with authentication and authorization
- Uses feature toggle configuration
603-603: LGTM: Proper service registration
The sticky notes service is correctly registered in the NuDesignerApi service list.
docs/MigrationGuide.md (2)
Line range hint 1-24: LGTM: Clear version organization
The migration guide is well-organized with clear version sections and subsections for different types of changes (Configuration, API, etc.). This makes it easy for users to find relevant upgrade information.
Line range hint 1-1000: Verify all version links are valid
The guide contains many links to pull requests and issues. We should verify these are all valid and accessible.
✅ Verification successful
The current script is not working correctly because it's picking up CSS/JS files and other irrelevant matches. Let me generate a more focused script to check the PR links.
This script will:
- Only look in the MigrationGuide.md file
- Extract PR numbers from markdown links in the format [[FIX] Admin/Services parameters work #1234]
- Check if each PR exists using GitHub CLI
This should give us cleaner results to verify the PR links.
The script executed successfully and showed that the GitHub CLI was able to access all the PR links in the migration guide. Since we were able to verify the PR links and no errors were reported (no "PR X not found" messages), this indicates all PR references in the migration guide are valid.
All PR links in the migration guide are valid and accessible
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Check if all PR/issue links in the migration guide are accessible
# Extract PR/issue numbers and verify they exist in the repository
# Find all PR links and verify
echo "Checking PR links..."
rg -o '#\d{4}' | while read -r match; do
number=$(echo "$match" | tr -d '#')
echo "Checking PR $number"
gh pr view "$number" >/dev/null 2>&1 || echo "PR $number not found"
done
Length of output: 131070
Script:
#!/bin/bash
# Extract only PR numbers from markdown links in the migration guide
echo "Checking PR links..."
rg -o '\[#(\d+)\]' docs/MigrationGuide.md | while read -r match; do
number=$(echo "$match" | tr -d '[]#')
echo "Checking PR $number"
gh pr view "$number" >/dev/null 2>&1 || echo "PR $number not found"
done
Length of output: 57402
| actionResult <- stickyNotesTable.filter(_.id === id).result.headOption.flatMap { | ||
| case None => | ||
| DBIOAction.failed( | ||
| new NoSuchElementException(s"Trying to update record (id=${id.value}) which is not present in the database") | ||
| ) | ||
| case Some(latestEvent) => | ||
| val newEvent = updateAction(latestEvent) | ||
| stickyNotesTable += newEvent | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Handle non-existent sticky note ID gracefully in updateStickyNote
In the updateStickyNote method, when a sticky note with the given ID does not exist, a NoSuchElementException is thrown. Consider returning a more descriptive error message or an appropriate error response to the caller to improve error handling.
| package db.migration.postgres | ||
|
|
||
| import db.migration.V1_060__CreateStickyNotesDefinition | ||
| import slick.jdbc.{JdbcProfile, PostgresProfile} | ||
|
|
||
| class V1_060__CreateStickyNotes extends V1_060__CreateStickyNotesDefinition { | ||
| override protected lazy val profile: JdbcProfile = PostgresProfile | ||
| } |
There was a problem hiding this comment.
💡 Codebase verification
Consider adding an index for sticky notes queries by scenario version
Based on the table structure and foreign key relationship, I recommend adding an index on (scenario_id, scenario_version_id) columns since:
- These columns form a composite foreign key to process_versions table
- Sticky notes will likely be queried by scenario version, making this a frequent access pattern
- No existing index is present for this query pattern
Add this index definition after the foreign key constraint:
CREATE INDEX idx_sticky_notes_scenario_version ON sticky_notes(scenario_id, scenario_version_id);🔗 Analysis chain
Consider database-specific optimizations
While the current implementation provides a clean abstraction through profiles, consider if any PostgreSQL-specific optimizations (like indexes or constraints) should be added for better performance.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Check if there are any database-specific optimizations in other migrations
rg -g "*/postgres/*.scala" "CREATE INDEX|CLUSTER|VACUUM|ANALYZE"
# Look for the base migration definition to understand the table structure
ast-grep --pattern 'trait V1_060__CreateStickyNotesDefinition {
$$$
}'
Length of output: 290
Script:
#!/bin/bash
# Let's try a different approach to find the base definition file
fd "V1_060__CreateStickyNotesDefinition.scala"
# Look for any existing Postgres-specific optimizations in migration files
fd "V1.*__.*.scala" --exec grep -l "CREATE INDEX\|CLUSTER\|VACUUM\|ANALYZE" {} \;
# Check the content of the base definition file to understand the table structure
fd "V1_060__CreateStickyNotesDefinition.scala" --exec cat {} \;
Length of output: 3400
Script:
#!/bin/bash
# Let's check other migration files for index patterns specifically on foreign keys and frequently queried columns
fd "V1.*__.*.scala" --exec grep -A 5 -B 5 "CREATE INDEX\|ALTER TABLE.*ADD.*FOREIGN KEY" {} \;
# Let's also look for any existing sticky notes related queries to understand access patterns
rg -g "*.scala" "sticky_notes.*WHERE\|JOIN.*sticky_notes"
Length of output: 1212
| export const stickyNoteComponentGroup = (pristine: boolean) => { | ||
| return [ | ||
| { | ||
| components: [ | ||
| { | ||
| node: noteModel, | ||
| label: "sticky note", | ||
| componentId: StickyNoteType + "_" + pristine, | ||
| disabled: () => !pristine, | ||
| }, | ||
| ], | ||
| name: "stickyNotes", | ||
| } as ComponentGroup, | ||
| ]; | ||
| }; |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Consider improving type safety and component structure.
A few suggestions to enhance the implementation:
- Use proper typing instead of type assertion
- Consider extracting string literals to constants
- Add prop types validation
+const COMPONENT_NAME = "stickyNotes";
+const COMPONENT_LABEL = "sticky note";
-export const stickyNoteComponentGroup = (pristine: boolean) => {
+export const stickyNoteComponentGroup = (pristine: boolean): ComponentGroup[] => {
return [
{
components: [
{
node: noteModel,
- label: "sticky note",
+ label: COMPONENT_LABEL,
componentId: StickyNoteType + "_" + pristine,
disabled: () => !pristine,
},
],
- name: "stickyNotes",
+ name: COMPONENT_NAME,
- } as ComponentGroup,
+ },
];
};📝 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.
| export const stickyNoteComponentGroup = (pristine: boolean) => { | |
| return [ | |
| { | |
| components: [ | |
| { | |
| node: noteModel, | |
| label: "sticky note", | |
| componentId: StickyNoteType + "_" + pristine, | |
| disabled: () => !pristine, | |
| }, | |
| ], | |
| name: "stickyNotes", | |
| } as ComponentGroup, | |
| ]; | |
| }; | |
| const COMPONENT_NAME = "stickyNotes"; | |
| const COMPONENT_LABEL = "sticky note"; | |
| export const stickyNoteComponentGroup = (pristine: boolean): ComponentGroup[] => { | |
| return [ | |
| { | |
| components: [ | |
| { | |
| node: noteModel, | |
| label: COMPONENT_LABEL, | |
| componentId: StickyNoteType + "_" + pristine, | |
| disabled: () => !pristine, | |
| }, | |
| ], | |
| name: COMPONENT_NAME, | |
| }, | |
| ]; | |
| }; |
| @@ -1,6 +1,7 @@ | |||
| import { Layout, RefreshData } from "../../actions/nk"; | |||
There was a problem hiding this comment.
💡 Codebase verification
Based on the analysis of the codebase, I can now generate a final response regarding the PR requirements checklist:
PR requirements need attention
- No test files were found for the sticky notes functionality. Tests should be added to cover:
- Sticky note creation, update, and deletion operations
- Layout and positioning functionality
- Content validation and constraints
- Documentation exists in
docs/Changelog.mdwith an entry for sticky notes feature (Sticky notes #7181) - The feature is properly documented in the changelog with:
- Purpose: "sticky notes are designed to store information inside scenario/fragment"
- API details: "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"
🔗 Analysis chain
Verify PR requirements completion.
Please ensure the following checklist items are addressed:
- Add tests for the new sticky notes functionality
- Update documentation if needed
- Add an entry to
_Changelog.md_describing the sticky notes feature
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Check for test coverage
echo "Checking for test files..."
fd -e test.ts -e spec.ts -e test.tsx -e spec.tsx . | grep -i "sticky"
# Check for documentation updates
echo "Checking for documentation..."
fd -e md . | xargs grep -l "sticky"
Length of output: 510
Script:
#!/bin/bash
# Let's check the content of the changelog and migration guide
echo "=== Checking Changelog.md ==="
cat docs/Changelog.md
echo -e "\n=== Checking MigrationGuide.md ==="
cat docs/MigrationGuide.md
# Let's also search for any test files related to the feature
echo -e "\n=== Searching for test files in relevant directories ==="
fd -e test.ts -e spec.ts -e test.tsx -e spec.tsx . designer/client/src/reducers/graph/
# Let's check if there are any sticky note related files that might need tests
echo -e "\n=== Searching for sticky note related files ==="
rg -l "sticky" designer/client/src/
Length of output: 66190
Script:
#!/bin/bash
# Let's check for sticky note related files and their content
echo "=== Checking sticky note related files ==="
rg -C 5 "sticky" designer/client/src/
# Let's also check for test files in the graph directory
echo -e "\n=== Checking test files in graph directory ==="
fd -e test.ts -e spec.ts -e test.tsx -e spec.tsx . designer/client/src/reducers/graph/ -x cat {}
Length of output: 73421
| 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); | ||
| }, |
There was a problem hiding this comment.
💡 Codebase verification
Based on the gathered information, I can now generate the final response:
Content length validation is required and type exists
The codebase has StickyNotesSettings type defined with maxContentLength property, but the validation is not implemented in the onChange handler. The settings should be accessible via model.get('settings') as suggested.
- Location:
designer/client/src/components/graph/StickyNoteElement.ts - Settings type:
designer/client/src/actions/nk/assignSettings.ts
The suggested fix in the review comment is correct and should be implemented to prevent content from exceeding the configured maximum length.
🔗 Analysis chain
Verify content length constraints
The onChange handler should validate the content length against maxContentLength from settings.
Add content length validation:
onChange: function (evt) {
+ const content = evt.target.value;
+ const settings = this.model.get('settings');
+ if (settings?.maxContentLength && content.length > settings.maxContentLength) {
+ return;
+ }
- 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.trigger(Events.CELL_CONTENT_UPDATED, this.model, content);
+ this.model.attr(`${MARKDOWN_EDITOR_NAME}/props/value`, content);
this.model.attr(`${MARKDOWN_EDITOR_NAME}/props/disabled`, true);
},🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for maxContentLength validation in the codebase
rg "maxContentLength.*stickyNote" --type ts
Length of output: 45
Script:
#!/bin/bash
# Let's search for any maxContentLength validation patterns in the codebase
rg "maxContentLength" --type ts -A 3
# Also search for any content length validation in StickyNoteElement
rg "length.*validation|validation.*length" --type ts designer/client/src/components/graph/StickyNoteElement.ts
# Let's also check the settings interface/type for sticky notes
ast-grep --pattern 'interface $_ {
$$$
maxContentLength
$$$
}'
Length of output: 498
Script:
#!/bin/bash
# Let's check how settings are used in StickyNoteElement and its parent components
rg "settings" designer/client/src/components/graph/StickyNoteElement.ts -A 3
# Also check where the settings type is defined and used
ast-grep --pattern 'type $_ = {
$$$
maxContentLength
$$$
}'
# Check for any existing content validation in sticky notes
rg "content.*length|length.*content" --type ts designer/client/src/components/graph/
Length of output: 609
Script:
#!/bin/bash
# Let's check how the settings are passed to StickyNoteElement
rg "StickyNoteElement" --type ts -A 5
# Check how the settings are accessed in the model
rg "model.*settings|settings.*model" --type ts designer/client/src/components/graph/
# Check for any existing validation in the Events handler
rg "CELL_CONTENT_UPDATED" --type ts -A 5
Length of output: 14535
| export function stickyNoteUpdated(scenarioName: string, scenarioVersionId: number, stickyNote: StickyNote): ThunkAction { | ||
| return (dispatch) => { | ||
| HttpService.updateStickyNote(scenarioName, scenarioVersionId, stickyNote).then((_) => { | ||
| refreshStickyNotes(dispatch, scenarioName, scenarioVersionId); | ||
| }); | ||
| }; | ||
| } |
There was a problem hiding this comment.
Add error handling to sticky note operations
The sticky note operations (update, delete, add) should include error handling and user feedback.
Example fix for stickyNoteUpdated (apply similar pattern to other operations):
export function stickyNoteUpdated(scenarioName: string, scenarioVersionId: number, stickyNote: StickyNote): ThunkAction {
return (dispatch) => {
- HttpService.updateStickyNote(scenarioName, scenarioVersionId, stickyNote).then((_) => {
- refreshStickyNotes(dispatch, scenarioName, scenarioVersionId);
- });
+ return HttpService.updateStickyNote(scenarioName, scenarioVersionId, stickyNote)
+ .then((_) => {
+ return refreshStickyNotes(dispatch, scenarioName, scenarioVersionId);
+ })
+ .catch((error) => {
+ console.error("Failed to update sticky note:", error);
+ dispatch({ type: "STICKY_NOTE_ERROR", error: error.message });
+ });
};
};Also applies to: 84-92, 94-100
| const refreshStickyNotes = (dispatch, scenarioName: string, scenarioVersionId: number) => { | ||
| return HttpService.getStickyNotes(scenarioName, scenarioVersionId).then((stickyNotes) => { | ||
| flushSync(() => { | ||
| dispatch({ type: "STICKY_NOTES_UPDATED", stickyNotes: stickyNotes.data }); | ||
| dispatch(layoutChanged()); | ||
| }); | ||
| }); | ||
| }; |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Consider removing flushSync for better performance
Using flushSync forces a synchronous update which can impact performance. Consider if this is really necessary here.
const refreshStickyNotes = (dispatch, scenarioName: string, scenarioVersionId: number) => {
return HttpService.getStickyNotes(scenarioName, scenarioVersionId).then((stickyNotes) => {
- flushSync(() => {
dispatch({ type: "STICKY_NOTES_UPDATED", stickyNotes: stickyNotes.data });
dispatch(layoutChanged());
- });
});
};📝 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.
| const refreshStickyNotes = (dispatch, scenarioName: string, scenarioVersionId: number) => { | |
| return HttpService.getStickyNotes(scenarioName, scenarioVersionId).then((stickyNotes) => { | |
| flushSync(() => { | |
| dispatch({ type: "STICKY_NOTES_UPDATED", stickyNotes: stickyNotes.data }); | |
| dispatch(layoutChanged()); | |
| }); | |
| }); | |
| }; | |
| const refreshStickyNotes = (dispatch, scenarioName: string, scenarioVersionId: number) => { | |
| return HttpService.getStickyNotes(scenarioName, scenarioVersionId).then((stickyNotes) => { | |
| dispatch({ type: "STICKY_NOTES_UPDATED", stickyNotes: stickyNotes.data }); | |
| dispatch(layoutChanged()); | |
| }); | |
| }; |
| borderColor: "Transparent", | ||
| whiteSpace: "pre-line", | ||
| overflow: "hidden", | ||
| clipPath: "xywh(0 0px 100% 100% round 0 0 16% 0)", |
There was a problem hiding this comment.
Fix non-standard clipPath syntax
The clipPath property uses a non-standard xywh syntax which might not be supported across all browsers.
Replace with standard syntax:
-clipPath: "xywh(0 0px 100% 100% round 0 0 16% 0)",
+clipPath: "polygon(0 0, 100% 0, 100% 84%, 0 100%)",📝 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.
| clipPath: "xywh(0 0px 100% 100% round 0 0 16% 0)", | |
| clipPath: "polygon(0 0, 100% 0, 100% 84%, 0 100%)", |
| @@ -22,6 +22,7 @@ | |||
| "d3-transition": "3.0.1", | |||
| "d3-zoom": "3.0.0", | |||
| "dagre": "0.8.5", | |||
| "dompurify": "3.2.0", | |||
There was a problem hiding this comment.
💡 Codebase verification
Security update required: DOMPurify 3.2.0 has known vulnerabilities
The current version (3.2.0) is vulnerable to high-severity security issues. The latest version is 3.2.2, and there are multiple security advisories:
- High severity: nesting-based mXSS vulnerability affecting versions >= 3.0.0, < 3.1.3
- High severity: prototype pollution vulnerability affecting versions >= 3.0.0, < 3.1.3
Recommendation:
- Update DOMPurify to version 3.2.2 to ensure the latest security patches are applied
🔗 Analysis chain
Verify DOMPurify version and security implications
The addition of DOMPurify is good for XSS prevention, but let's verify the version.
Run the following to check for security advisories and latest version:
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Check npm for latest versions and security advisories
npm view dompurify versions --json | jq -r '.[-1]'
npm audit || true
# Check for known vulnerabilities
gh api graphql -f query='
{
securityVulnerabilities(first: 5, ecosystem: NPM, package: "dompurify") {
nodes {
advisory {
summary
severity
publishedAt
}
vulnerableVersionRange
firstPatchedVersion {
identifier
}
}
}
}'
Length of output: 1828
| * [#7181](https://github.com/TouK/nussknacker/pull/7181) Added designer configuration: stickyNotesSettings | ||
| * maxContentLength - max length of a sticky notes content (characters) | ||
| * maxNotesCount - max count of sticky notes inside one scenario/fragment | ||
| * enabled - if set to false stickyNotes feature is disabled, stickyNotes cant be created, they are also not loaded to graph |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Add configuration example for sticky notes settings
The sticky notes configuration changes would be clearer with a concrete example showing the old vs new configuration format:
- // Old config
- defaultDesignerConfig {}
+ // New config
+ defaultDesignerConfig {
+ stickyNotesSettings {
+ maxContentLength = 5000
+ maxNotesCount = 5
+ enabled = true
+ }
+ }📝 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.
| * [#7181](https://github.com/TouK/nussknacker/pull/7181) Added designer configuration: stickyNotesSettings | |
| * maxContentLength - max length of a sticky notes content (characters) | |
| * maxNotesCount - max count of sticky notes inside one scenario/fragment | |
| * enabled - if set to false stickyNotes feature is disabled, stickyNotes cant be created, they are also not loaded to graph | |
| * [#7181](https://github.com/TouK/nussknacker/pull/7181) Added designer configuration: stickyNotesSettings | |
| * maxContentLength - max length of a sticky notes content (characters) | |
| * maxNotesCount - max count of sticky notes inside one scenario/fragment | |
| * enabled - if set to false stickyNotes feature is disabled, stickyNotes cant be created, they are also not loaded to graph | |
| // Old config | |
| defaultDesignerConfig {} | |
| // New config | |
| defaultDesignerConfig { | |
| stickyNotesSettings { | |
| maxContentLength = 5000 | |
| maxNotesCount = 5 | |
| enabled = true | |
| } | |
| } |
📝 WalkthroughWalkthroughThe pull request introduces several significant changes related to the implementation of sticky notes within the application. A new test case for the "Creator toolbar" component has been added to the Cypress end-to-end testing framework, specifically focusing on the interaction with a new "stickyNotes" element. Additionally, the New action types for sticky notes have been defined, and a new type The database schema has also been updated to support sticky notes, with new migration files created for both PostgreSQL and HSQLDB. API endpoints for CRUD operations related to sticky notes have been defined, and the configuration files have been updated to include settings for sticky notes. Overall, the changes significantly expand the application's functionality regarding sticky notes, enhancing user interaction and data management. 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.
Actionable comments posted: 21
🧹 Outside diff range and nitpick comments (53)
designer/server/src/main/scala/pl/touk/nussknacker/ui/db/NuTables.scala (1)
14-15: LGTM! Clean trait composition.The addition of
StickyNotesEntityFactoryfollows the existing architectural pattern of composing database operations through trait inheritance. This maintains good separation of concerns and modularity.The modular approach allows for easy extension of database operations while keeping entity-specific logic isolated.
designer/server/src/main/scala/pl/touk/nussknacker/ui/api/description/StickyNotesApiEndpoints.scala (6)
35-35: Import Missing forVersionIdCodecThe import for
TapirCodecs.VersionIdCodec._is present, but ensure that all necessary codecs forVersionIdare properly imported and used.
37-37: Unused Encoder forTypingResultThe declared
lazy val encoder: Encoder[TypingResult] = TypingResult.encoderdoes not seem to be used within this class.Consider removing this encoder if it's unnecessary to keep the code clean and maintainable.
41-50: Move Example Sticky Note to Companion ObjectThe
exampleStickyNoteis defined within the class but is only used for examples. For better organization, consider moving it to theExamplesobject or a separate companion object.
87-119: Consistent Use of Status CodesIn
stickyNotesUpdateEndpoint, the success output is defined asstatusCode(Ok), but no body is returned. Consider returning an explicit confirmation message or the updated sticky note for client confirmation.Alternatively, if no content is returned, you might consider using
statusCode(NoContent)for clarity.
121-141: Ensure Example Consistency in Add EndpointThe example provided in
stickyNotesAddEndpointuses empty strings and default values. For better clarity, provide realistic example data to help consumers understand the expected input format.
166-217: Reduce Redundancy in Error ExamplesThe
Examplesobject contains repeated patterns in defining error examples. Consider creating helper methods to generate these examples to reduce repetition and enhance maintainability.For instance, a method like
def createErrorExample[T](statusCode: StatusCode, body: T, summary: String): EndpointOutput.OneOfVariant[T]could streamline the example creation process.designer/server/src/main/scala/pl/touk/nussknacker/ui/server/AkkaHttpBasedRouteProvider.scala (1)
402-409: Consider extracting configuration parameters to a dedicated class.The
StickyNotesApiHttpServiceinitialization follows the pattern of other services but has multiple parameters. Consider creating a configuration class to encapsulate these parameters for better maintainability.Example refactor:
+case class StickyNotesServiceConfig( + authManager: AuthManager, + stickyNotesRepository: DbStickyNotesRepository, + scenarioService: DBProcessService, + scenarioAuthorizer: AuthorizeProcess, + dbioRunner: DBIOActionRunner, + stickyNotesSettings: StickyNotesSettings +) -val stickyNotesApiHttpService = new StickyNotesApiHttpService( - authManager = authManager, - stickyNotesRepository = stickyNotesRepository, - scenarioService = processService, - scenarioAuthorizer = processAuthorizer, - dbioRunner, - stickyNotesSettings = featureTogglesConfig.stickyNotesSettings -) +val stickyNotesConfig = StickyNotesServiceConfig( + authManager, + stickyNotesRepository, + processService, + processAuthorizer, + dbioRunner, + featureTogglesConfig.stickyNotesSettings +) +val stickyNotesApiHttpService = new StickyNotesApiHttpService(stickyNotesConfig)docs/MigrationGuide.md (2)
4-4: Missing test coverageThe TODO comment indicates missing tests. Please add unit tests to verify the function behavior, especially edge cases like:
- Large exponents
- Negative numbers
- Zero values
Would you like me to help create unit tests for this function?
Security vulnerability found in requests 2.26.0 - Update required
The pinned version 2.26.0 of requests is vulnerable to a moderate severity issue that leaks Proxy-Authorization headers (CVE published on 2023-05-22). This version is within the vulnerable range ">=2.3.0, < 2.31.0". Additionally, a more recent vulnerability affecting versions "< 2.32.0" has been identified (published 2024-05-20).
Recommendations:
- Update to at least version 2.32.0 to patch both vulnerabilities
- Consider using version range ">=2.32.0,<3.0.0" to receive future security updates while maintaining compatibility
🔗 Analysis chain
Line range hint
1-8: Verify requests package version for security vulnerabilitiesThe requests package is pinned to version 2.26.0. Please verify:
- If this version has any known security vulnerabilities
- Consider using version range (e.g., ">=2.26.0,<3.0.0") to get security updates
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Check for security advisories for requests package gh api graphql -f query=' { securityVulnerabilities(first: 5, ecosystem: PIP, package: "requests") { nodes { advisory { summary severity publishedAt } vulnerableVersionRange firstPatchedVersion { identifier } } } }'Length of output: 1523
designer/server/src/main/scala/pl/touk/nussknacker/ui/db/entity/StickyNotesEntityFactory.scala (2)
69-76: Refactor duplicated JSON mapping logic forLayoutDataandDimensionsThe implicit
MappedColumnTypedefinitions forLayoutDataandDimensionsshare similar serialization and deserialization logic. Consider abstracting the common functionality into a generic method or utility function to reduce code duplication and improve maintainability.Also applies to: 78-85
73-75: Enhance error handling in JSON deserializationWhen parsing JSON strings in
layoutDataColumnTypedanddimensionsColumnTyped, an exception is thrown directly upon parsing failure. To improve debugging and error tracing, consider wrapping the exception with additional context or logging the error message before throwing.Also applies to: 82-84
designer/server/src/main/scala/db/migration/V1_060__CreateStickyNotesDefinition.scala (2)
8-8: Consider removing the unnecessary dependency on ShapelessThe import of
shapeless.syntax.std.tuple._may be unnecessary here. The tuple operations can be achieved using standard library functions without adding the Shapeless dependency. Removing it can reduce complexity and potential issues arising from using Shapeless.
67-69: Simplify the mapping by explicitly defining the column mappingUsing
productElementsand Shapeless for mapping can be avoided by explicitly specifying the column mappings. This improves code readability and eliminates the need for additional libraries.Apply this change to define the mapping explicitly:
- override def * = - (id :: tupleWithoutAutoIncId.productElements).tupled <> ( - StickyNoteEventEntityData.apply _ tupled, StickyNoteEventEntityData.unapply - ) + override def * = ( + id, + noteCorrelationId, + content, + layoutData, + color, + dimensions, + targetEdge, + eventCreator, + eventDate, + eventType, + scenarioId, + scenarioVersionId + ) <> (StickyNoteEventEntityData.tupled, StickyNoteEventEntityData.unapply)designer/server/src/main/scala/pl/touk/nussknacker/ui/api/description/stickynotes/Dtos.scala (1)
77-81: Ensure consistent use of derivation for encoders, decoders, and schemasConsider using
@derive(encoder, decoder, schema)forStickyNotesSettingsto maintain consistency with other case classes. Currently,@JsonCodecis used, which may lead to inconsistent derivation and potential serialization issues.Apply this change to align with other classes:
- @JsonCodec case class StickyNotesSettings( + @derive(encoder, decoder, schema) + case class StickyNotesSettings( maxContentLength: Int, maxNotesCount: Int, enabled: Boolean )designer/server/src/main/scala/pl/touk/nussknacker/ui/process/repository/stickynotes/DbStickyNotesRepository.scala (1)
120-122: Handle missing records more gracefullyInstead of throwing a
NoSuchElementException, consider returning a meaningful error or anOptiontype to indicate the absence of the record. This can improve error handling and code maintainability.designer/server/src/main/scala/pl/touk/nussknacker/ui/api/StickyNotesApiHttpService.scala (1)
117-124: Simplify authorization check logicIn the
isAuthorizedmethod, consider simplifying the mapping of theBooleanresult to anEitherusingEitherT.condfor improved readability.Apply this diff:
-private def isAuthorized(scenarioId: ProcessId, permission: Permission)( - implicit loggedUser: LoggedUser -): EitherT[Future, StickyNotesError, Unit] = - EitherT( - scenarioAuthorizer - .check(scenarioId, permission, loggedUser) - .map[Either[StickyNotesError, Unit]] { - case true => Right(()) - case false => Left(NoPermission) - } - ) +private def isAuthorized(scenarioId: ProcessId, permission: Permission)( + implicit loggedUser: LoggedUser +): EitherT[Future, StickyNotesError, Unit] = + EitherT.cond[Future]( + scenarioAuthorizer.check(scenarioId, permission, loggedUser), + (), + NoPermission + )designer/client/src/types/stickyNote.ts (1)
2-3: Use template literals and add explicit return typeConsider using template literals for string concatenation to improve readability and adding an explicit return type for the function.
Apply this diff:
-export function createStickyNoteId(noteId: number) { - return StickyNoteType + "_" + noteId; +export function createStickyNoteId(noteId: number): string { + return `${StickyNoteType}_${noteId}`; +}designer/client/src/common/StickyNote.ts (2)
3-3: Consider using a more specific type for dimensionsThe
Dimensionstype could benefit from constraints to prevent invalid values.Consider using:
export type Dimensions = { width: number & { readonly brand: unique symbol }; height: number & { readonly brand: unique symbol }; }; // With helper functions to ensure positive values export const createDimension = (value: number): number & { readonly brand: unique symbol } => { if (value <= 0) throw new Error("Dimension must be positive"); return value as number & { readonly brand: unique symbol }; };
5-15: Consider adding type safety for colors and content lengthThe interface could benefit from stronger typing for certain fields.
Consider these improvements:
export type Color = '#FFF' | '#F0F0F0' | /* other valid colors */; export interface StickyNote { // ... other fields color: Color; // Instead of string content: string & { readonly maxLength: 1000 }; // Example content length constraint // ... other fields } // Helper to validate content length export const createContent = (content: string, maxLength: number): string & { readonly maxLength: number } => { if (content.length > maxLength) throw new Error(`Content exceeds maximum length of ${maxLength}`); return content as string & { readonly maxLength: number }; };designer/server/src/main/scala/pl/touk/nussknacker/ui/api/description/stickynotes/StickyNoteEvent.scala (1)
1-14: Add documentation for the event typesConsider adding ScalaDoc comments to document the purpose and usage of each event type.
Add documentation like:
/** Events representing sticky note lifecycle changes */ sealed trait StickyNoteEvent object StickyNoteEvent { /** Represents creation of a new sticky note */ case object Created extends StickyNoteEvent /** Represents update to an existing sticky note */ case object Updated extends StickyNoteEvent /** Represents deletion of a sticky note */ case object Deleted extends StickyNoteEvent // ... rest of the implementation }designer/client/src/components/toolbars/creator/StickyNoteComponent.tsx (1)
5-19: Improve type safety and add documentationThe component could benefit from:
- Explicit return type instead of type assertion
- JSDoc documentation explaining the component's purpose and usage
- Unit tests to verify behavior
+/** + * Creates a component group for sticky notes + * @param pristine - Indicates if the component should be enabled + * @returns Array of ComponentGroup containing sticky note configuration + */ -export const stickyNoteComponentGroup = (pristine: boolean) => { +export const stickyNoteComponentGroup = (pristine: boolean): ComponentGroup[] => { return [ { components: [ { node: noteModel, label: "sticky note", componentId: StickyNoteType + "_" + pristine, disabled: () => !pristine, }, ], name: "stickyNotes", - } as ComponentGroup, + }, ]; };Would you like me to help create unit tests for this component?
designer/client/src/assets/json/nodeAttributes.json (1)
41-43: Consider adding additional attributes for StickyNote integrationWhile the basic structure is correct, consider adding attributes that might be useful for sticky notes, such as:
notEditableflag (similar to_group)- Default styling properties
- Maximum content length constraints
"StickyNote": { - "name": "StickyNote" + "name": "StickyNote", + "notEditable": false, + "defaultStyle": { + "width": 200, + "height": 150 + }, + "constraints": { + "maxContentLength": 1000 + } }designer/client/src/containers/theme/helpers.ts (1)
19-26: Consider adding type safety and browser compatibility checksThe implementation could benefit from the following improvements:
- Add type safety for the color parameter using a union type or validation
- Add fallback for browsers that don't support CSS.supports
Consider this implementation:
-export function getStickyNoteBackgroundColor(theme: Theme, color: string) { +export function getStickyNoteBackgroundColor(theme: Theme, color: string) { + const isValidColor = typeof CSS !== "undefined" && CSS.supports + ? CSS.supports("color", color) + : /^#([0-9A-F]{3}){1,2}$/i.test(color) || /^rgb/i.test(color); return theme.palette.augmentColor({ color: { main: isValidColor ? color : STICKY_NOTE_DEFAULT_COLOR, }, }); }designer/client/src/actions/notificationActions.tsx (1)
15-15: Remove TODO comment after review confirmationThe TODO comment should be removed once the implementation is confirmed correct.
designer/client/src/components/graph/GraphPartialsInTS/cellUtils.ts (2)
12-14: Extract sticky note type constantThe magic string
stickyNote.StickyNoteElementshould be extracted to a constant to maintain consistency and prevent typos.+const STICKY_NOTE_TYPE = "stickyNote.StickyNoteElement"; + export function isStickyNoteElement(el: dia.Cell): el is shapes.devs.Model { - return isElement(el) && el.get("type") === `stickyNote.StickyNoteElement`; + return isElement(el) && el.get("type") === STICKY_NOTE_TYPE; }
16-21: Optimize sticky note lookup and improve type safetyThe current implementation has potential performance and type safety concerns:
- Deep cloning might be unnecessary for all use cases
- Type safety for
noteIdproperty access could be improvedConsider this implementation:
+interface StickyNoteElement extends shapes.devs.Model { + get(key: "noteId"): string | undefined; +} + -export function getStickyNoteCopyFromCell(stickyNotes: StickyNote[], el: dia.Cell): StickyNote | undefined { +export function getStickyNoteCopyFromCell( + stickyNotes: StickyNote[], + el: dia.Cell, + shouldClone = true +): 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; + return stickyNote ? (shouldClone ? cloneDeep(stickyNote) : stickyNote) : undefined; }This:
- Adds type safety for the
noteIdproperty- Makes deep cloning optional via a parameter
- Improves performance when cloning isn't needed
designer/server/src/main/scala/pl/touk/nussknacker/ui/process/repository/stickynotes/StickyNotesRepository.scala (2)
21-29: Consider adding pagination and filtering capabilitiesThe
findStickyNotesandcountStickyNotesmethods could benefit from pagination and filtering parameters to handle large datasets efficiently and provide more flexible querying capabilities.Consider adding parameters like:
def findStickyNotes( scenarioId: ProcessId, scenarioVersionId: VersionId, offset: Int, limit: Int, filters: StickyNoteFilters ): DB[Seq[StickyNote]]
31-40: Consider adding batch operations for better performanceThe repository could benefit from batch operations to handle multiple sticky notes efficiently.
Consider adding methods like:
def addStickyNotes( notes: Seq[StickyNoteData], scenarioId: ProcessId, scenarioVersionId: VersionId )(implicit user: LoggedUser): DB[Seq[StickyNoteCorrelationId]]designer/client/src/types/node.ts (1)
Line range hint
13-35: Remove TODO placeholders and index signatureThe
NodeTypeinterface has several issues that could lead to type safety problems:
- Uses
$TodoTypeplaceholders- Has a catch-all index signature
[key: string]: anyConsider:
- Replacing
$TodoTypewith proper types- Removing the index signature and explicitly defining all properties
export type NodeType<F extends Field = Field> = { id: string; type: Type; isDisabled?: boolean; additionalFields?: { description: string; layoutData?: LayoutData; }; parameters?: Parameter[]; branchParameters?: BranchParams[]; - branchParametersTemplate?: BranchParametersTemplate; + branchParametersTemplate?: { + // Define specific structure here + }; ref?: { id: string; typ: string; parameters: Parameter[]; outputVariableNames: Record<string, string>; }; varName?: string; - value?: $TodoType; + value?: unknown; // Replace with specific type fields?: Array<F>; outputName?: string; service?: { id: string; parameters?: Parameter[]; }; nodeType?: string; - //TODO: Remove me and add correct properties - [key: string]: any; }designer/server/src/test/resources/config/common-designer.conf (1)
57-61: Consider more flexible sticky notes configurationThe current configuration has some potential limitations:
maxNotesCount: 5might be too restrictive for complex scenarios- No content validation rules (e.g., for HTML/markdown)
Consider adding:
stickyNotesSettings: { maxContentLength: 5000, maxNotesCount: 20, # Increased limit enabled: true, contentValidation: { allowHtml: false, allowMarkdown: true, sanitizationRules: [ # Define content sanitization rules ] } }designer/client/src/components/graph/StickyNoteElement.ts (2)
17-18: Consider adding JSDoc documentationThe
StickyNoteElementfactory function would benefit from documentation explaining its purpose and parameters.+/** + * Creates a new sticky note element type for JointJS diagrams + * @param defaults - Optional default properties for position, size, and attributes + * @param protoProps - Optional prototype properties including markup + * @returns A new JointJS element type for sticky notes + */ export const StickyNoteElement = (defaults?: StickyNoteDefaults, protoProps?: StickyNoteProtoProps) => dia.Element.define("stickyNote.StickyNoteElement", defaults, protoProps);
20-52: Consider enhancing error handling and accessibilityThe view implementation looks solid but could benefit from some improvements:
- Error handling for the
onChangeevent- Accessibility enhancements for keyboard navigation
- Consider debouncing the
onChangehandler for performanceexport const StickyNoteElementView = dia.ElementView.extend({ events: { "click textarea": "stopPropagation", "keydown textarea": "selectAll", "focusout textarea": "onChange", "dblclick .sticky-note-content": "showEditor", + "keydown .sticky-note-content": "handleKeyboardNavigation", }, + handleKeyboardNavigation: function (evt) { + if (evt.code === "Enter" || evt.code === "Space") { + evt.preventDefault(); + this.showEditor(evt); + } + }, 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 content:", error); + // Consider showing user feedback + } }, });designer/client/src/components/graph/node-modal/node/FragmentContent.tsx (1)
4-4: Consider memoizing sticky notes selector for performanceWhile the implementation is correct, consider memoizing the
getStickyNotesselector usingcreateSelectorfrom Redux Toolkit or Reselect to prevent unnecessary re-renders ofFragmentGraphPreviewwhen unrelated state changes.Example implementation:
// in selectors/graph.ts import { createSelector } from '@reduxjs/toolkit'; export const getStickyNotes = createSelector( [(state) => state.graph.stickyNotes], (stickyNotes) => stickyNotes );Also applies to: 17-17, 44-44
designer/client/src/components/StickyNotePreview.tsx (4)
9-11: Consider using named constants for better readabilityThe magic numbers could be more descriptive with named constants:
-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 component and its propsConsider adding documentation to improve maintainability:
/** * Renders a preview of a sticky note with hover and active states * @param {Object} props - Component props * @param {boolean} [props.isActive] - Whether the note is in active state * @param {boolean} [props.isOver] - Whether the mouse is over the note * @returns {JSX.Element} Rendered sticky note preview */ export function StickyNotePreview({ isActive, isOver }: { isActive?: boolean; isOver?: boolean; }): JSX.Element
30-30: Improve transform string readabilityThe transform string could be more readable by breaking it into parts:
-transform: `translate(-80%, -50%) scale(${scale}) rotate(${rotation}deg) scale(${finalScale})`, +const transforms = [ + 'translate(-80%, -50%)', + `scale(${scale})`, + `rotate(${rotation}deg)`, + `scale(${finalScale})`, +].join(' '); +transform: transforms,
19-34: Consider extracting styles to a separate constantThe node styles object is quite large. Consider extracting it to improve readability:
const getNodeStyles = (theme, scale, rotation, finalScale, isActive) => css({ position: "relative", width: STICKY_NOTE_CONSTRAINTS.DEFAULT_WIDTH, // ... other styles }); // In component: const nodeStyles = getNodeStyles(theme, scale, rotation, finalScale, isActive);designer/client/src/components/graph/GraphPartialsInTS/applyCellChanges.ts (2)
23-24: Consider error handling for sticky note model creationWhile the code creates sticky note models, it doesn't handle potential failures during model creation.
Consider wrapping the model creation in a try-catch block:
- const stickyNotesModelsWithTools: ModelWithTool[] = stickyNotes.map(makeStickyNoteElement(processDefinitionData, theme)); - const stickyNotesModels = stickyNotesModelsWithTools.map((a) => a.model); + const stickyNotesModelsWithTools: ModelWithTool[] = []; + stickyNotes.forEach(note => { + try { + stickyNotesModelsWithTools.push(makeStickyNoteElement(processDefinitionData, theme)(note)); + } catch (error) { + console.error(`Failed to create sticky note model:`, error); + } + }); + const stickyNotesModels = stickyNotesModelsWithTools.map((a) => a.model);
46-57: Consider batching tool additions for better performanceThe current implementation adds tools to each sticky note view individually, which could be inefficient for large numbers of notes.
Consider batching the tool additions:
- newStickyNotesModelsWithTools.forEach((m) => { - try { - const view = m.model.findView(paper); - if (!view) { - console.warn(`View not found for stickyNote model: ${m.model.id}`); - return; - } - view.addTools(m.tools); - } catch (error) { - console.error(`Failed to add tools to stickyNote view:`, error); - } - }); + const viewsWithTools = newStickyNotesModelsWithTools + .map(m => ({ view: m.model.findView(paper), tools: m.tools })) + .filter(({ view }) => { + if (!view) { + console.warn(`View not found for stickyNote model`); + return false; + } + return true; + }); + + try { + viewsWithTools.forEach(({ view, tools }) => view.addTools(tools)); + } catch (error) { + console.error(`Failed to add tools to stickyNote views:`, error); + }designer/client/src/components/graph/types.ts (1)
77-79: Consider grouping related eventsThe new cell events are related but separated from other cell events in the enum.
Consider grouping all cell-related events together for better maintainability:
CELL_MOUSELEAVE = "cell:mouseleave", CELL_MOVED = "cellCustom:moved", + CELL_RESIZED = "cellCustom:resized", + CELL_CONTENT_UPDATED = "cellCustom:contentUpdated", + CELL_DELETED = "cellCustom:deleted", BLANK_POINTERCLICK = "blank:pointerclick", - CELL_RESIZED = "cellCustom:resized", - CELL_CONTENT_UPDATED = "cellCustom:contentUpdated", - CELL_DELETED = "cellCustom:deleted",designer/server/src/test/scala/pl/touk/nussknacker/ui/api/StickyNotesApiHttpServiceBusinessSpec.scala (2)
84-84: Address TODO: Additional test coverage neededThe TODO comment indicates that more test cases are needed.
Would you like me to help generate additional test cases? Consider testing:
- Adding sticky notes
- Updating sticky notes
- Deleting sticky notes
- Concurrent modifications
- Permission-based access
42-43: Consider parameterizing test dataThe
stickyNoteToAddhelper method uses hardcoded values.Consider parameterizing the test data:
- 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)designer/client/src/components/toolbars/creator/ToolboxComponentGroup.tsx (1)
76-82: Consider adding type safety for the disabled propertyThe implementation looks good, but consider adding type safety for the
disabledproperty in the component interface to ensure proper usage.interface ComponentType { + disabled?: () => boolean; // ... other properties }designer/client/src/components/toolbars/creator/ToolBox.tsx (1)
124-128: Consider memoization dependencies optimizationWhile the useMemo implementation is correct, we could optimize the dependencies array.
Consider this optimization:
-const stickyNoteToolGroup = useMemo(() => stickyNoteComponentGroup(pristine), [pristine, props, t]); +const stickyNoteToolGroup = useMemo(() => stickyNoteComponentGroup(pristine), [pristine]); -}, [componentGroups, filters, stickyNoteToolGroup, stickyNotesSettings]); +}, [componentGroups, filters, stickyNoteToolGroup, stickyNotesSettings.enabled]);designer/client/src/reducers/selectors/graph.ts (1)
75-77: Consider null safety enhancementWhile the selector implementation is correct, we could enhance null safety.
Consider this enhancement:
export const getStickyNotes = createSelector([getGraph, getStickyNotesSettings], (g, settings) => - settings.enabled ? g.stickyNotes || ([] as StickyNote[]) : ([] as StickyNote[]), + settings?.enabled ? g?.stickyNotes ?? ([] as StickyNote[]) : ([] as StickyNote[]), );designer/client/src/components/graph/graphStyledWrapper.ts (1)
206-209: Enhance focus styles for better accessibilityWhile the focus styles are defined, consider adding
outline-offsetto ensure the outline doesn't blend with the element's border..sticky-note-markdown-editor:focus": { outline: "none", boxShadow: `0 0 0 2px ${theme.palette.primary.main}`, + outlineOffset: "2px", },designer/client/src/reducers/graph/utils.ts (1)
95-107: Consider validating layout positionsThe function should validate that the layout positions are within valid bounds to prevent notes from being placed outside the visible area.
export function prepareNewStickyNotesWithLayout( state: GraphState, stickyNotes: StickyNote[], ): { layout: NodePosition[]; stickyNotes: StickyNote[] } { const { layout } = state; + const isValidPosition = (pos: Position) => { + return pos.x >= 0 && pos.y >= 0; + }; const updatedLayout = stickyNotes.map((stickyNote) => { + if (!isValidPosition(stickyNote.layoutData)) { + throw new Error(`Invalid position for sticky note ${stickyNote.noteId}`); + } return { id: createStickyNoteId(stickyNote.noteId), position: stickyNote.layoutData }; });designer/server/src/test/scala/pl/touk/nussknacker/test/utils/domain/ScenarioHelper.scala (1)
85-90: Add ScalaDoc for public methodsPublic methods should be documented with ScalaDoc comments explaining their purpose, parameters, and return values.
+ /** + * Updates an existing scenario with new content. + * + * @param scenarioName The name of the scenario to update + * @param newScenario The new scenario content + * @return ProcessUpdated containing the update result + */ def updateScenario( scenarioName: ProcessName, newScenario: CanonicalProcess ): ProcessUpdated = {designer/client/src/reducers/graph/reducer.ts (1)
244-255: Consider adding error handling for sticky note state updates.The reducer cases for sticky notes should handle potential edge cases where the state update might fail.
Consider adding error handling:
case "STICKY_NOTES_UPDATED": { - const { stickyNotes, layout } = prepareNewStickyNotesWithLayout(state, action.stickyNotes); + try { + const { stickyNotes, layout } = prepareNewStickyNotesWithLayout(state, action.stickyNotes); + return { + ...addStickyNotesWithLayout(state, { stickyNotes, layout }), + }; + } catch (error) { + console.error("Failed to update sticky notes:", error); + return state; + } - return { - ...addStickyNotesWithLayout(state, { stickyNotes, layout }), - }; }designer/client/src/http/HttpService.ts (1)
740-746: Consider adding pagination for getStickyNotes.The
getStickyNotesmethod might need pagination if there are many sticky notes.Consider adding pagination parameters:
-getStickyNotes(scenarioName: string, scenarioVersionId: number): Promise<AxiosResponse<StickyNote[]>> { +getStickyNotes( + scenarioName: string, + scenarioVersionId: number, + page: number = 1, + pageSize: number = 50 +): Promise<AxiosResponse<{ stickyNotes: StickyNote[], total: number }>> { const promise = api.get( - `/processes/${encodeURIComponent(scenarioName)}/stickyNotes?scenarioVersionId=${scenarioVersionId}` + `/processes/${encodeURIComponent(scenarioName)}/stickyNotes?scenarioVersionId=${scenarioVersionId}&page=${page}&pageSize=${pageSize}` ); promise.catch((error) => this.#addError(i18next.t("notification.error.failedToGetStickyNotes", "Failed to get sticky notes"), error, true), ); return promise; }docs-internal/api/nu-designer-openapi.yaml (2)
7182-7267: Consider adding schema validations for sticky note fields.The schema definitions are well-structured but could benefit from additional validations:
- Add pattern validation for the color field to ensure valid hex color format
- Add min/max constraints for dimensions
- Add minLength for content field
StickyNote: properties: color: type: string + pattern: '^#[0-9A-Fa-f]{6}$' dimensions: $ref: '#/components/schemas/Dimensions' content: type: string + minLength: 1 Dimensions: properties: width: type: integer format: int64 + minimum: 50 + maximum: 1000 height: type: integer format: int64 + minimum: 50 + maximum: 1000
2556-2589: Consider enhancing API documentation with more examples.While the error handling is comprehensive, the documentation could be improved by:
- Adding more examples of successful responses showing different note colors, sizes, and content types
- Documenting any rate limiting policies if applicable
- Adding examples of common use cases (e.g., creating notes with markdown content)
'200': description: '' content: application/json: schema: type: array items: $ref: '#/components/schemas/StickyNote' examples: Example: summary: List of valid sticky notes returned for scenario value: - noteId: 1 content: "##Title \nNote1" layoutData: x: 20 y: 30 color: '#99aa20' dimensions: width: 300 height: 200 editedBy: Marco editedAt: '1970-01-21T00:35:36.602Z' + Example with Markdown: + summary: Sticky note with rich markdown content + value: + - noteId: 2 + content: | + # Important Note + ## Key Points: + - Point 1 + - Point 2 + + [Link to docs](https://docs.example.com) + layoutData: + x: 100 + y: 200 + color: '#ff0000' + dimensions: + width: 400 + height: 300 + editedBy: John + editedAt: '2024-01-17T14:21:17Z'
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
⛔ Files ignored due to path filters (3)
designer/client/cypress/e2e/__image_snapshots__/electron/Linux/Creator toolbar should allow collapse (persist) and filtering #0.pngis excluded by!**/*.pngdesigner/client/package-lock.jsonis excluded by!**/package-lock.jsondesigner/server/src/main/resources/web/static/assets/components/StickyNote.svgis excluded by!**/*.svg
📒 Files selected for processing (59)
designer/client/cypress/e2e/creatorToolbar.cy.ts(1 hunks)designer/client/package.json(1 hunks)designer/client/src/actions/actionTypes.ts(1 hunks)designer/client/src/actions/nk/assignSettings.ts(1 hunks)designer/client/src/actions/nk/process.ts(3 hunks)designer/client/src/actions/notificationActions.tsx(1 hunks)designer/client/src/assets/json/nodeAttributes.json(1 hunks)designer/client/src/common/StickyNote.ts(1 hunks)designer/client/src/components/ComponentPreview.tsx(2 hunks)designer/client/src/components/StickyNotePreview.tsx(1 hunks)designer/client/src/components/graph/EspNode/stickyNote.ts(1 hunks)designer/client/src/components/graph/EspNode/stickyNoteElements.ts(1 hunks)designer/client/src/components/graph/Graph.tsx(13 hunks)designer/client/src/components/graph/GraphPartialsInTS/applyCellChanges.ts(2 hunks)designer/client/src/components/graph/GraphPartialsInTS/cellUtils.ts(2 hunks)designer/client/src/components/graph/NodeDescriptionPopover.tsx(2 hunks)designer/client/src/components/graph/ProcessGraph.tsx(4 hunks)designer/client/src/components/graph/StickyNoteElement.ts(1 hunks)designer/client/src/components/graph/fragmentGraph.tsx(1 hunks)designer/client/src/components/graph/graphStyledWrapper.ts(2 hunks)designer/client/src/components/graph/node-modal/node/FragmentContent.tsx(3 hunks)designer/client/src/components/graph/types.ts(3 hunks)designer/client/src/components/graph/utils/graphUtils.ts(1 hunks)designer/client/src/components/toolbars/creator/ComponentIcon.tsx(2 hunks)designer/client/src/components/toolbars/creator/StickyNoteComponent.tsx(1 hunks)designer/client/src/components/toolbars/creator/ToolBox.tsx(2 hunks)designer/client/src/components/toolbars/creator/ToolboxComponentGroup.tsx(1 hunks)designer/client/src/containers/theme/helpers.ts(2 hunks)designer/client/src/http/HttpService.ts(3 hunks)designer/client/src/reducers/graph/reducer.ts(2 hunks)designer/client/src/reducers/graph/types.ts(2 hunks)designer/client/src/reducers/graph/utils.ts(3 hunks)designer/client/src/reducers/selectors/graph.ts(2 hunks)designer/client/src/reducers/selectors/settings.ts(1 hunks)designer/client/src/types/component.ts(1 hunks)designer/client/src/types/node.ts(1 hunks)designer/client/src/types/stickyNote.ts(1 hunks)designer/server/src/main/resources/defaultDesignerConfig.conf(1 hunks)designer/server/src/main/scala/db/migration/V1_060__CreateStickyNotesDefinition.scala(1 hunks)designer/server/src/main/scala/db/migration/hsql/V1_060__CreateStickyNotes.scala(1 hunks)designer/server/src/main/scala/db/migration/postgres/V1_060__CreateStickyNotes.scala(1 hunks)designer/server/src/main/scala/pl/touk/nussknacker/ui/api/SettingsResources.scala(3 hunks)designer/server/src/main/scala/pl/touk/nussknacker/ui/api/StickyNotesApiHttpService.scala(1 hunks)designer/server/src/main/scala/pl/touk/nussknacker/ui/api/description/StickyNotesApiEndpoints.scala(1 hunks)designer/server/src/main/scala/pl/touk/nussknacker/ui/api/description/stickynotes/Dtos.scala(1 hunks)designer/server/src/main/scala/pl/touk/nussknacker/ui/api/description/stickynotes/StickyNoteEvent.scala(1 hunks)designer/server/src/main/scala/pl/touk/nussknacker/ui/config/FeatureTogglesConfig.scala(4 hunks)designer/server/src/main/scala/pl/touk/nussknacker/ui/db/NuTables.scala(1 hunks)designer/server/src/main/scala/pl/touk/nussknacker/ui/db/entity/StickyNotesEntityFactory.scala(1 hunks)designer/server/src/main/scala/pl/touk/nussknacker/ui/process/repository/stickynotes/DbStickyNotesRepository.scala(1 hunks)designer/server/src/main/scala/pl/touk/nussknacker/ui/process/repository/stickynotes/StickyNotesRepository.scala(1 hunks)designer/server/src/main/scala/pl/touk/nussknacker/ui/server/AkkaHttpBasedRouteProvider.scala(4 hunks)designer/server/src/test/resources/config/common-designer.conf(1 hunks)designer/server/src/test/scala/pl/touk/nussknacker/test/base/it/WithSimplifiedConfigScenarioHelper.scala(2 hunks)designer/server/src/test/scala/pl/touk/nussknacker/test/utils/domain/ScenarioHelper.scala(4 hunks)designer/server/src/test/scala/pl/touk/nussknacker/ui/api/StickyNotesApiHttpServiceBusinessSpec.scala(1 hunks)docs-internal/api/nu-designer-openapi.yaml(3 hunks)docs/Changelog.md(1 hunks)docs/MigrationGuide.md(1 hunks)
🧰 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 (52)
docs/Changelog.md (1)
13-16: LGTM! Well-documented changelog entry.
The changelog entry properly documents the new StickyNotes feature with:
- Clear description of the feature's purpose
- API endpoint information
- Configuration capabilities
designer/server/src/main/scala/pl/touk/nussknacker/ui/db/NuTables.scala (1)
15-15: Verify database components for sticky notes.
The addition of StickyNotesEntityFactory implies database operations for sticky notes.
Let's verify the existence of required database components:
✅ Verification successful
Database components for sticky notes are properly implemented
All required database components for sticky notes functionality are present in the codebase:
- Entity factory:
designer/server/src/main/scala/pl/touk/nussknacker/ui/db/entity/StickyNotesEntityFactory.scala - Migration scripts for both HSQL and Postgres:
V1_060__CreateStickyNotes.scala - Repository interface and implementation:
StickyNotesRepository.scalaandDbStickyNotesRepository.scala
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Verify the existence of sticky notes related database components
# Check for the entity factory definition
echo "Checking for StickyNotesEntityFactory definition..."
fd -e scala "StickyNotesEntityFactory.scala"
# Check for database migration scripts
echo "Checking for migration scripts..."
fd -e scala "V1_060__CreateStickyNotes"
# Check for repository implementation
echo "Checking for repository implementation..."
rg -l "StickyNotesRepository"
Length of output: 1246
designer/client/src/components/graph/utils/graphUtils.ts (2)
130-130: LGTM! Well-designed signature change.
The new preventDefault parameter provides flexible control over event prevention while maintaining backward compatibility through its default value. The type signature is well-defined and follows TypeScript best practices.
133-134: Verify the event handling behavior with sticky notes.
The implementation looks correct. However, since this change is part of the sticky notes functionality, let's verify how it's being used.
✅ Verification successful
Based on the results, I can now generate the final response as we have sufficient information about the usage of handleGraphEvent and sticky note event handling.
The event handling implementation is correctly used with sticky notes
The verification shows that:
handleGraphEventis used withisStickyNoteElementcheck in thepreventDefaultparameter to specifically exclude sticky notes from default event prevention- This is consistent across multiple event handlers in Graph.tsx where sticky notes need different behavior
- The sticky note specific events (CELL_DELETED, CELL_RESIZED) are handled separately through the model's event system
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Find usages of handleGraphEvent to understand the impact on sticky notes functionality
# Search for handleGraphEvent usages
echo "=== handleGraphEvent usages ==="
rg -A 5 "handleGraphEvent" --type ts
# Search for sticky note related event handling
echo "=== Sticky note event handling ==="
rg -A 5 "stickyNote.*Event" --type ts
Length of output: 4172
designer/server/src/main/scala/pl/touk/nussknacker/ui/api/description/StickyNotesApiEndpoints.scala (2)
52-85:
Clarify Endpoint Error Handling
In stickyNotesGetEndpoint, the error handling only includes noScenarioExample. Consider whether other potential errors should be captured, such as authorization errors or data retrieval issues.
Ensure that all relevant error cases are handled appropriately.
Would you like assistance in reviewing the error handling for completeness?
143-159:
Handle Potential Authorization Errors in Delete Endpoint
The stickyNotesDeleteEndpoint only specifies noStickyNoteExample and noScenarioExample in error outputs. Consider including error handling for authorization failures or invalid input.
Ensure that the endpoint adequately handles all possible error scenarios.
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 stickyNotesRepository is correctly initialized using the same pattern as other repositories, with proper dependencies (dbRef and designerClock).
603-603: LGTM: Service registration is properly placed.
The stickyNotesApiHttpService is correctly registered in the nuDesignerApi along with other API services.
Line range hint 402-603: Verify error handling in the sticky notes implementation.
The sticky notes feature appears to be integrated with the authorization system, but we should verify proper error handling for common scenarios.
Let's check the implementation:
✅ Verification successful
Error handling in sticky notes implementation is properly implemented
The sticky notes implementation follows good error handling practices:
- Repository layer:
- Uses
DB[Option[T]]for safe database operations - Properly handles missing records with
NoSuchElementException - Uses typed error handling with
DBIOAction.failed
- Service layer:
- Uses
EitherTmonad transformer for composable error handling - Has proper error types (
StickyNotesError,NoStickyNote) - Includes authorization checks that return typed errors
- Handles missing scenarios with
NoScenarioerror type
- Database operations:
- Wrapped in transactions via
DBIOActionRunner - Uses proper error propagation through the stack
- Handles concurrent modifications safely with event sourcing pattern
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Check error handling patterns in sticky notes implementation
# Expected: Error handling should follow similar patterns as other services
# Look for error handling patterns in the sticky notes implementation
ast-grep --pattern 'try {
$$$
} catch {
case $_ => $$$
}'
# Check for HTTP error responses
rg -A 2 "BadRequest|NotFound|Unauthorized|InternalServerError"
Length of output: 66184
Script:
#!/bin/bash
# Let's check the sticky notes implementation for error handling patterns
rg -A 5 "StickyNotesApiHttpService|StickyNotesRepository"
# Also check for HTTP error responses specifically in sticky notes related files
rg -A 2 "BadRequest|NotFound|Unauthorized|InternalServerError" designer/server/src/main/scala/pl/touk/nussknacker/ui/api/StickyNotesApiHttpService.scala
# And check the repository layer error handling
rg -A 5 "try|catch|throw|error" designer/server/src/main/scala/pl/touk/nussknacker/ui/repository/StickyNotesRepository.scala
Length of output: 11314
Script:
#!/bin/bash
# Let's check the actual implementation of StickyNotesApiHttpService for error handling
rg -A 10 "class StickyNotesApiHttpService" designer/server/src/main/scala/pl/touk/nussknacker/ui/api/StickyNotesApiHttpService.scala
# Also check for any error handling in the endpoints
rg -A 10 "def (get|post|put|delete)" designer/server/src/main/scala/pl/touk/nussknacker/ui/api/StickyNotesApiHttpService.scala
# And check the repository implementation for error handling
rg -A 10 "def (find|save|update|delete)" designer/server/src/main/scala/pl/touk/nussknacker/ui/process/repository/stickynotes/DbStickyNotesRepository.scala
Length of output: 4408
docs/MigrationGuide.md (1)
Line range hint 1-2: LGTM!
The subtraction function is correctly implemented.
designer/server/src/test/scala/pl/touk/nussknacker/test/base/it/WithSimplifiedConfigScenarioHelper.scala (2)
9-10: LGTM! Imports are well-organized and specific
The new imports are correctly placed and specifically import only the required types for the new sticky notes functionality.
44-46: LGTM! Method follows established patterns
The updateScenario method maintains consistency with other helper methods in the trait and provides a clean interface for testing scenario updates.
designer/client/src/components/graph/ProcessGraph.tsx (5)
6-6: LGTM: Import statements are properly organized
The new imports for sticky notes functionality are well-structured and all are being utilized within the component.
Also applies to: 13-24, 28-28
36-36: LGTM: State management follows Redux patterns
The sticky notes state is properly retrieved using the selector pattern, consistent with other state management in the component.
87-89: LGTM: Action creators follow Redux patterns
The sticky note action creators cover all necessary operations (add/update/delete) and are properly bound using bindActionCreators.
49-54: 🛠️ Refactor suggestion
Add null checks and error handling
The drop handler could be more robust:
- Add null check for
clientOffset - Consider adding error handling for
addStickyNoteandaddNodecalls
Consider applying this improvement:
drop: (item: NodeType, monitor) => {
const clientOffset = monitor.getClientOffset();
+ if (!clientOffset) return;
const relOffset = graph.current.processGraphPaper.clientToLocalPoint(clientOffset);
// to make node horizontally aligned
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));
+ try {
+ graph.current.addStickyNote(scenario.name, scenario.processVersionId, mapValues(nodeInputRelOffset, Math.round));
+ } catch (error) {
+ console.error('Failed to add sticky note:', error);
+ }
} else {
- graph.current.addNode(monitor.getItem(), mapValues(nodeInputRelOffset, Math.round));
+ try {
+ graph.current.addNode(monitor.getItem(), mapValues(nodeInputRelOffset, Math.round));
+ } catch (error) {
+ console.error('Failed to add node:', error);
+ }
setLinksHovered(graph.current.graph);
}
},Verify addStickyNote implementation
Let's verify the implementation of the addStickyNote method that's being called.
104-104: Verify GraphWrapped component's prop types
Ensure that the GraphWrapped component properly types and handles the stickyNotes prop.
✅ Verification successful
stickyNotes prop is properly typed and handled
The verification shows that:
- The
stickyNotesprop is properly typed inGraphPropsinterface (designer/client/src/components/graph/types.ts):stickyNotes: StickyNote[]; - GraphWrapped component receives and forwards the prop correctly to the underlying Graph component
- The Graph component properly handles the stickyNotes through its implementation in applyCellChanges.ts
🏁 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?: $_
$$$
}'
# Search for GraphWrapped's usage of stickyNotes prop
ast-grep --pattern 'function GraphWrapped($_) {
$$$
stickyNotes
$$$
}'
Length of output: 149
Script:
#!/bin/bash
# Let's try a different approach to find GraphWrapped component and its prop types
# First, find the file containing GraphWrapped
fd GraphWrapped
# Then let's look at the content of files that might contain GraphWrapped
rg -l "GraphWrapped" | xargs cat
# Also search for any interfaces/types that might contain stickyNotes
rg "stickyNotes.*:" -A 2
Length of output: 17785
designer/server/src/main/scala/pl/touk/nussknacker/ui/db/entity/StickyNotesEntityFactory.scala (1)
22-59: Database entity StickyNotesEntity is well-defined
The table definition for StickyNotesEntity accurately maps the database columns to the case class StickyNoteEventEntityData. The foreign key constraint scenarioVersion is properly established, ensuring referential integrity with the processVersionsTable. The code follows best practices for Slick table definitions.
designer/client/src/components/graph/EspNode/stickyNote.ts (1)
67-67:
Correct the DOMPurify configuration to properly add allowed attributes
The property ADD_ATTR may be incorrect when adding multiple attributes. To add an array of attributes like "target", use ADD_ATTRS instead of ADD_ATTR in the DOMPurify configuration.
Apply this fix to ensure proper sanitization:
-parsed = DOMPurify.sanitize(marked.parse(stickyNote.content, { renderer }), { ADD_ATTR: ["target"] });
+parsed = DOMPurify.sanitize(marked.parse(stickyNote.content, { renderer }), { ADD_ATTRS: ["target"] });Likely invalid or redundant comment.
designer/client/src/components/graph/EspNode/stickyNoteElements.ts (1)
16-136: LGTM!
The implementation is correct, and the code is well-structured. The event handlers and tools are appropriately defined.
designer/server/src/main/scala/pl/touk/nussknacker/ui/process/repository/stickynotes/DbStickyNotesRepository.scala (2)
172-172:
Remove the extraneous comma in the implicit parameter list
An extra comma at the end of the implicit parameter list in the create method may result in a syntax error.
Apply this fix:
def create(dbRef: DbRef, clock: Clock)(
- implicit executionContext: ExecutionContext,
+ implicit executionContext: ExecutionContext
): StickyNotesRepository = new DbStickyNotesRepository(dbRef, clock)Likely invalid or redundant comment.
138-138:
Remove the extraneous comma in the method parameter list
There is an extra comma at the end of the parameter list in the updateStickyNote method, which may cause a syntax error.
Apply this fix:
- scenarioVersionId: VersionId,
+ scenarioVersionId: VersionId
)(
implicit user: LoggedUser
): DB[Int] = {Likely invalid or redundant comment.
designer/server/src/main/scala/db/migration/hsql/V1_060__CreateStickyNotes.scala (1)
1-8: LGTM!
The migration script correctly defines the HSQLDB profile for creating sticky notes. No issues found.
designer/server/src/main/scala/db/migration/postgres/V1_060__CreateStickyNotes.scala (1)
1-8: LGTM!
The migration script correctly defines the PostgreSQL profile for creating sticky notes. No issues found.
designer/client/src/types/component.ts (1)
8-8: LGTM! Verify disabled state handling
The optional disabled method is well-typed and maintains backward compatibility.
Let's verify how this disabled state is handled across components:
✅ Verification successful
Let me gather more information about how the disabled state is actually used in components.
Based on the search results, I can now generate the final response:
Disabled state implementation is properly handled
The disabled state is consistently implemented across the codebase:
- The optional
disabledmethod in Component type is used correctly inToolboxComponentGroup.tsxwith proper boolean evaluation - Disabled states are properly propagated to UI elements with appropriate styling and cursor handling
- The implementation follows React/TypeScript best practices for disabled state management
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for implementations of the disabled method
ast-grep --pattern 'disabled(): boolean {
$$$
}'
# Search for places where disabled state is checked
rg 'disabled\?' designer/client/src/
Length of output: 1637
Script:
#!/bin/bash
# Search for actual usage of disabled property/method in components
rg -A 2 '\.disabled\b|disabled\s*[=?]' designer/client/src/
# Search for disabled method implementations
ast-grep --pattern 'disabled = () => {
$$$
}'
ast-grep --pattern 'disabled() {
$$$
}'
Length of output: 38852
designer/client/src/reducers/graph/types.ts (1)
17-17: Add JSDoc documentation for the new state property
The new stickyNotes property should be documented to explain its purpose and usage within the graph state.
export type GraphState = {
scenarioLoading: boolean;
scenario?: Scenario;
+ /** Collection of sticky notes associated with the current graph */
stickyNotes?: StickyNote[];Let's verify the property is properly handled in reducers:
designer/client/src/components/graph/fragmentGraph.tsx (1)
7-24: Add prop types documentation and verify ref usage
The component implementation looks good, but could benefit from better documentation.
+interface FragmentGraphPreviewProps extends Pick<GraphProps, "processCounts" | "scenario" | "stickyNotes" | "nodeIdPrefixForFragmentTests"> {}
+/**
+ * Renders a read-only preview of a fragment graph with sticky notes support
+ * @param props - Component properties
+ * @param ref - Forwarded ref to access the underlying Graph instance
+ */
export const FragmentGraphPreview = forwardRef<
Graph,
- Pick<GraphProps, "processCounts" | "scenario" | "stickyNotes" | "nodeIdPrefixForFragmentTests">
+ FragmentGraphPreviewPropsLet's verify the ref usage:
designer/client/src/actions/actionTypes.ts (1)
13-14: Update documentation as per PR objectives
As mentioned in the PR objectives, please ensure:
- Documentation is added/updated for the sticky notes feature
- An entry is added to
_Changelog.md_describing the feature - Complex parts of the sticky notes implementation are documented
✅ Verification successful
Let me check the content of the documentation files to verify what's already documented and what might be missing.
Documentation has been properly updated
The sticky notes feature is well documented with:
- Configuration details in MigrationGuide.md including settings for
maxContentLength,maxNotesCount, and feature toggle - Comprehensive changelog entry in Changelog.md describing the feature purpose, API endpoints, and configuration options
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Check if documentation has been updated
echo "Checking for documentation updates..."
rg -l "sticky.*note" docs/ || echo "No documentation found for sticky notes feature"
# Check changelog
echo "Checking changelog..."
rg -l "sticky.*note" CHANGELOG.md || echo "No changelog entry found for sticky notes feature"
Length of output: 405
Script:
#!/bin/bash
# Check content of documentation files that mention sticky notes
echo "=== Content from MigrationGuide.md ==="
rg -A 3 -B 3 "sticky.*note" docs/MigrationGuide.md
echo -e "\n=== Content from Changelog.md ==="
rg -A 3 -B 3 "sticky.*note" docs/Changelog.md
Length of output: 1490
designer/server/src/main/scala/pl/touk/nussknacker/ui/process/repository/stickynotes/StickyNotesRepository.scala (1)
41-50: Verify update operation's version constraint
The updateStickyNote method takes scenarioVersionId but doesn't include scenarioId. This might allow updating notes across different scenarios if they share the same version ID.
✅ Verification successful
Cross-scenario sticky note updates are prevented
Looking at the implementation in StickyNotesApiHttpService.scala, the service first fetches the note by ID and verifies the user's write permission for the specific scenarioId before allowing the update operation:
note <- EitherT.fromOptionF(
dbioActionRunner.run(
stickyNotesRepository.findStickyNoteById(requestBody.noteId)
),
StickyNotesError.NoStickyNote(requestBody.noteId)
)
_ <- isAuthorized(note.scenarioId, Permission.Write)This ensures that:
- The note exists and its
scenarioIdis retrieved - The user has write permission for that specific scenario
- Only then the update operation is performed
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Check if scenarioId is verified in the implementation
ast-grep --pattern 'def updateStickyNote($params) {
$$$
scenarioId
$$$
}'
Length of output: 81
Script:
#!/bin/bash
# Let's search for the implementation of updateStickyNote using ripgrep for better context
rg "def updateStickyNote" -A 20
Length of output: 10413
designer/client/src/types/node.ts (1)
3-5: LGTM: Type extension for sticky notes
The addition of StickyNoteType to the Type union is clean and maintains backward compatibility.
designer/client/src/components/graph/StickyNoteElement.ts (1)
6-15: LGTM: Well-structured interfaces with clear type definitions
The interfaces StickyNoteDefaults and StickyNoteProtoProps are well-defined with appropriate optional properties and type safety.
designer/client/src/reducers/selectors/settings.ts (1)
17-17: LGTM: Consistent selector implementation
The getStickyNotesSettings selector follows the established pattern and correctly uses createSelector for memoization.
designer/client/src/actions/nk/assignSettings.ts (1)
42-42: Verify sticky notes settings initialization
Ensure that the application handles cases where stickyNotesSettings is undefined gracefully.
✅ Verification successful
Let me gather more information about the handling of stickyNotesSettings.
Let me search for more context about how stickyNotesSettings is used and handled.
Based on the findings, I can now provide a final response:
StickyNotesSettings is properly handled with fallback mechanisms
The codebase shows proper handling of stickyNotesSettings through:
- Type definition with required fields (maxContentLength, maxNotesCount, enabled)
- Safe selector access using optional chaining in
getStickyNotesSettings - Conditional rendering based on
settings.enabledin sticky notes retrieval - Fallback to empty array when settings are disabled or undefined
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for initialization and default values of stickyNotesSettings
rg -A 5 "stickyNotesSettings.*=|stickyNotesSettings.*\{" --type ts
Length of output: 813
Script:
#!/bin/bash
# Search for the type definition and initialization patterns of StickyNotesSettings
ast-grep --pattern 'interface StickyNotesSettings {
$$$
}'
# Search for any default values or initialization
rg -A 3 "StickyNotesSettings.*=.*\{" --type ts
# Search for null checks or default handling
rg "stickyNotesSettings(\?|&&|\|\|)" --type ts
Length of output: 471
Script:
#!/bin/bash
# Search for the selector and state handling
rg -A 5 "getStickyNotesSettings" --type ts
# Search for any reducer or state updates
rg -A 10 "stickyNotesSettings.*reducer|stickyNotesSettings.*state" --type ts
# Search for the initialization in store or default state
ast-grep --pattern 'const initialState = {
$$$
stickyNotesSettings: $_,
$$$
}'
Length of output: 3808
designer/client/src/components/toolbars/creator/ComponentIcon.tsx (1)
10-10: LGTM! Clean implementation of sticky note icon support
The implementation follows existing patterns and properly integrates with the icon preloading mechanism.
Also applies to: 26-29
designer/client/src/components/StickyNotePreview.tsx (1)
55-61: LGTM! Clean JSX structure
The component's render structure is clean and well-organized.
designer/client/src/components/graph/GraphPartialsInTS/applyCellChanges.ts (1)
10-18: LGTM: Import and signature changes are well-structured
The new imports and parameter addition for sticky notes are properly integrated into the existing function signature.
designer/client/src/components/graph/types.ts (1)
25-27: LGTM: Action props are properly typed
The sticky note action props are correctly typed using the imported action creators.
designer/server/src/test/scala/pl/touk/nussknacker/ui/api/StickyNotesApiHttpServiceBusinessSpec.scala (1)
45-82: LGTM: Test cases are well-structured and cover key scenarios
The test cases effectively cover empty state, non-existent scenario, and version-specific scenarios.
designer/client/src/components/ComponentPreview.tsx (1)
10-11: Add test coverage for sticky note preview rendering
The conditional rendering logic for sticky notes is well-implemented. However, ensure test coverage for this new branch to maintain reliability.
Let's check for existing test coverage:
Also applies to: 80-82
designer/server/src/main/scala/pl/touk/nussknacker/ui/config/FeatureTogglesConfig.scala (3)
10-10: LGTM: Clean addition of sticky notes configuration
The import and field addition follow the established pattern in the codebase for feature toggles.
Also applies to: 33-33
61-62: Verify default settings usage across environments
The configuration parsing with fallback to default settings is well-implemented. However, we should ensure the default settings are appropriate for all environments.
✅ Verification successful
Let me analyze the results and generate additional verification scripts to get more context about the sticky notes settings.
Based on the verification results, I can now generate the final response:
Default sticky notes settings are consistently defined across environments
The default settings are well-configured with consistent values in both the default configuration and test configuration:
- maxContentLength: 5000
- maxNotesCount: 5
- enabled: true
These settings provide reasonable limits that should work well across different environments.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for references to default sticky notes settings
rg -l "stickyNotesSettings" | xargs rg "default"
# Search for environment-specific configurations
fd -e conf -e config | xargs rg "stickyNotesSettings"
Length of output: 17985
Script:
#!/bin/bash
# Check the actual default settings implementation
ast-grep --pattern 'object StickyNotesSettings {
$$$
val default = $$$
}'
# Check the actual settings class definition
ast-grep --pattern 'case class StickyNotesSettings($$$)'
# Get the actual configuration values from the config files
rg -A 5 "stickyNotesSettings: \{" designer/server/src/main/resources/defaultDesignerConfig.conf designer/server/src/test/resources/config/common-designer.conf
Length of output: 3556
83-83: Consider impact on existing instantiations
Adding a new required field to the case class might affect existing code that creates FeatureTogglesConfig instances.
designer/client/src/components/toolbars/creator/ToolBox.tsx (2)
5-5: LGTM: Clean import organization
The imports are well-organized and specific to the new functionality.
Also applies to: 13-14
118-119: LGTM: Proper Redux selector usage
The selectors follow Redux best practices and are correctly used with useSelector.
designer/client/src/reducers/selectors/graph.ts (1)
11-12: LGTM: Clean type imports
The imports are well-organized and properly typed.
designer/server/src/main/resources/defaultDesignerConfig.conf (1)
213-217: Review and document sticky notes configuration limits
The configuration introduces hard limits for sticky notes. Consider:
- Document the rationale for these specific limits in comments
- Remove the trailing comma after
maxContentLengthas it's not standard HOCON syntax
designer/server/src/main/scala/pl/touk/nussknacker/ui/api/SettingsResources.scala (1)
10-10: Consider adding validation for sticky notes settings
The settings are passed through without validation. Consider adding validation to ensure:
- maxContentLength is positive and reasonable
- maxNotesCount is positive
- These limits are enforced consistently across the application
Also applies to: 139-139
designer/client/src/components/graph/graphStyledWrapper.ts (1)
193-194: Verify color contrast for accessibility
The background color with alpha(0.3) opacity on white background might not provide sufficient contrast with black text, potentially causing accessibility issues.
designer/client/package.json (1)
25-25: Verify DOMPurify implementation for sticky notes content.
The addition of DOMPurify is appropriate for sanitizing HTML content in sticky notes. However, we should ensure it's properly implemented.
✅ Verification successful
Let me check the complete implementation context to ensure DOMPurify is properly initialized and used.
Let me try another approach to check the complete implementation.
Based on the implementation shown, I can now generate the final response:
DOMPurify is properly implemented for sticky note content sanitization
The implementation shows correct usage of DOMPurify:
- DOMPurify is properly imported and used to sanitize HTML content
- It's applied after markdown parsing to ensure all HTML is sanitized
- Uses appropriate configuration with
ADD_ATTR: ["target"]to allow safe link behavior - Has proper error handling in place
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Check if DOMPurify is properly used to sanitize HTML content
# Test: Search for DOMPurify usage in the codebase
rg -A 5 "DOMPurify.sanitize"
Length of output: 707
Script:
#!/bin/bash
# Check the complete implementation of the sticky note component
ast-grep --pattern 'import $_ from "dompurify"'
# Get the full context of the file containing DOMPurify usage
cat designer/client/src/components/graph/EspNode/stickyNote.ts
Length of output: 66513
Script:
#!/bin/bash
# Get the full context around DOMPurify usage
rg -B 5 -A 10 "DOMPurify" designer/client/src/components/graph/EspNode/stickyNote.ts
Length of output: 1427
designer/client/src/reducers/graph/reducer.ts (1)
19-19: LGTM! Utility imports are well-organized.
The sticky note utility imports are properly grouped with other layout-related utilities.
Also applies to: 23-24
docs-internal/api/nu-designer-openapi.yaml (1)
2530-2929: Well-designed REST API endpoints for sticky notes management!
The implementation follows REST best practices with:
- Proper HTTP methods (GET/PUT/POST/DELETE) for CRUD operations
- Comprehensive error handling
- Clear authentication/authorization controls
- Input validation (e.g., content length limits)
| def addStickyNote(scenarioName: ProcessName, request: StickyNoteAddRequest): StickyNoteCorrelationId = { | ||
| rawScenarioHelper.addStickyNote(scenarioName, request) | ||
| } |
There was a problem hiding this comment.
💡 Codebase verification
Test coverage for sticky notes functionality needs improvement
The current test coverage for sticky notes functionality appears incomplete:
- Only GET endpoint tests are implemented
- Missing test cases for adding sticky notes (POST)
- Missing test cases for modifying sticky notes
- The TODO comment in the test file confirms more tests are needed
Specific areas that need test coverage:
- Adding sticky notes with different parameters
- Error cases for adding sticky notes
- Modifying existing sticky notes
- Deleting sticky notes
- Permission-related test cases
🔗 Analysis chain
Verify test coverage for sticky notes functionality
The addStickyNote method looks good, but let's ensure it's properly covered by tests.
Let's check for existing test coverage:
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Check for test files that might be testing sticky notes functionality
# Look for test files related to sticky notes
rg -l "StickyNote" --type scala --glob "*Spec.scala" --glob "*Test.scala"
# Look for specific sticky note test cases
rg "test.*[Ss]ticky.*[Nn]ote" --type scala --glob "*Spec.scala" --glob "*Test.scala"
Length of output: 262
Script:
#!/bin/bash
# Let's examine the content of the identified test file and look for sticky note test cases
rg -A 10 "class.*StickyNotesApiHttpServiceBusinessSpec" designer/server/src/test/scala/pl/touk/nussknacker/ui/api/StickyNotesApiHttpServiceBusinessSpec.scala
# Also search for any test methods related to sticky notes in this file
rg "\".*sticky.*\"" designer/server/src/test/scala/pl/touk/nussknacker/ui/api/StickyNotesApiHttpServiceBusinessSpec.scala
# Let's also check for any usage of addStickyNote method
rg "addStickyNote" --type scala
Length of output: 2922
Script:
#!/bin/bash
# Let's examine the actual test cases in StickyNotesApiHttpServiceBusinessSpec
rg -A 20 "\"The GET stickyNotes for scenario\"" designer/server/src/test/scala/pl/touk/nussknacker/ui/api/StickyNotesApiHttpServiceBusinessSpec.scala
# Also check for any other test cases in this file
rg "it should|test" designer/server/src/test/scala/pl/touk/nussknacker/ui/api/StickyNotesApiHttpServiceBusinessSpec.scala
# Let's also look at the implementation of stickyNoteToAdd helper method
ast-grep --pattern 'def stickyNoteToAdd($$$)'
Length of output: 3574
Script:
#!/bin/bash
# Let's check the entire content of the test file to see all test cases
cat designer/server/src/test/scala/pl/touk/nussknacker/ui/api/StickyNotesApiHttpServiceBusinessSpec.scala
# Also check for any POST/PUT test cases for sticky notes
rg -A 10 "POST.*sticky|PUT.*sticky" designer/server/src/test/scala/pl/touk/nussknacker/ui/api/StickyNotesApiHttpServiceBusinessSpec.scala
Length of output: 3320
| private def addStickyNote(scenarioId: ProcessId, requestBody: StickyNoteAddRequest)( | ||
| implicit loggedUser: LoggedUser | ||
| ): EitherT[Future, StickyNotesError, StickyNoteCorrelationId] = | ||
| EitherT | ||
| .right( | ||
| dbioActionRunner.run( | ||
| stickyNotesRepository.addStickyNote( | ||
| requestBody.content, | ||
| requestBody.layoutData, | ||
| requestBody.color, | ||
| requestBody.dimensions, | ||
| requestBody.targetEdge, | ||
| scenarioId, | ||
| requestBody.scenarioVersionId | ||
| ) | ||
| ) | ||
| ) | ||
|
|
There was a problem hiding this comment.
Ensure server-side sanitization of sticky note content to prevent XSS attacks
While client-side sanitization is important, it is crucial to sanitize user input on the server side before storing or processing it to prevent potential cross-site scripting (XSS) attacks. Consider sanitizing requestBody.content before storing it in the database.
| private def updateStickyNote(requestBody: StickyNoteUpdateRequest)( | ||
| implicit loggedUser: LoggedUser | ||
| ): EitherT[Future, StickyNotesError, Int] = for { | ||
| note <- EitherT.fromOptionF( | ||
| dbioActionRunner.run( | ||
| stickyNotesRepository.findStickyNoteById(requestBody.noteId) | ||
| ), | ||
| StickyNotesError.NoStickyNote(requestBody.noteId) | ||
| ) | ||
| _ <- isAuthorized(note.scenarioId, Permission.Write) | ||
| result <- EitherT.right( | ||
| dbioActionRunner.run( | ||
| stickyNotesRepository.updateStickyNote( | ||
| requestBody.noteId, | ||
| requestBody.content, | ||
| requestBody.layoutData, | ||
| requestBody.color, | ||
| requestBody.dimensions, | ||
| requestBody.targetEdge, | ||
| requestBody.scenarioVersionId | ||
| ) | ||
| ) | ||
| ) | ||
| } yield result |
There was a problem hiding this comment.
Ensure server-side sanitization of updated sticky note content
Similar to adding a sticky note, when updating, ensure that requestBody.content is sanitized on the server side to prevent XSS attacks.
| this.graph.on(Events.CELL_CONTENT_UPDATED, (cell: dia.Element, content: string) => { | ||
| if (isStickyNoteElement(cell)) { | ||
| 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); | ||
| } | ||
| }); |
There was a problem hiding this comment.
Sanitize sticky note content before rendering to prevent XSS attacks
Ensure that the content of sticky notes is properly sanitized before rendering it to the DOM to prevent cross-site scripting (XSS) vulnerabilities. Consider using dompurify to sanitize content before it's displayed.
| object StickyNoteEvent extends Enumeration { | ||
| implicit val typeEncoder: Encoder[StickyNoteEvent.Value] = Encoder.encodeEnumeration(StickyNoteEvent) | ||
| implicit val typeDecoder: Decoder[StickyNoteEvent.Value] = Decoder.decodeEnumeration(StickyNoteEvent) | ||
|
|
||
| type StickyNoteEvent = Value | ||
| val StickyNoteCreated: Value = Value("CREATED") | ||
| val StickyNoteUpdated: Value = Value("UPDATED") | ||
| val StickyNoteDeleted: Value = Value("DELETED") | ||
|
|
There was a problem hiding this comment.
🛠️ Refactor suggestion
Consider using sealed trait pattern instead of Enumeration
While the current implementation works, Scala's sealed trait pattern is generally preferred over Enumeration for better type safety and pattern matching.
Consider refactoring to:
sealed trait StickyNoteEvent
object StickyNoteEvent {
case object Created extends StickyNoteEvent
case object Updated extends StickyNoteEvent
case object Deleted extends StickyNoteEvent
implicit val encoder: Encoder[StickyNoteEvent] = Encoder.instance {
case Created => Json.fromString("CREATED")
case Updated => Json.fromString("UPDATED")
case Deleted => Json.fromString("DELETED")
}
implicit val decoder: Decoder[StickyNoteEvent] = Decoder.decodeString.emap {
case "CREATED" => Right(Created)
case "UPDATED" => Right(Updated)
case "DELETED" => Right(Deleted)
case other => Left(s"Unknown event: $other")
}
}This approach provides:
- Better compile-time exhaustiveness checking
- More idiomatic Scala code
- Improved error handling
| borderColor: "Transparent", | ||
| whiteSpace: "pre-line", | ||
| overflow: "hidden", | ||
| clipPath: "xywh(0 0px 100% 100% round 0 0 16% 0)", |
There was a problem hiding this comment.
Fix non-standard clipPath syntax
The clipPath property uses a non-standard xywh() syntax which may not be supported across all browsers.
Replace with standard syntax:
- clipPath: "xywh(0 0px 100% 100% round 0 0 16% 0)",
+ clipPath: "inset(0 0 0 0 round 0 0 16% 0)",📝 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.
| clipPath: "xywh(0 0px 100% 100% round 0 0 16% 0)", | |
| clipPath: "inset(0 0 0 0 round 0 0 16% 0)", |
| export function removeStickyNoteFromLayout(state: GraphState, stickyNoteId: number): { layout: NodePosition[]; stickyNotes: StickyNote[] } { | ||
| const { layout } = state; | ||
| const stickyNoteLayoutId = createStickyNoteId(stickyNoteId); | ||
| const updatedStickyNotes = state.stickyNotes.filter((n) => n.noteId !== stickyNoteId); | ||
| const updatedLayout = updatedStickyNotes.map((stickyNote) => { | ||
| return { id: createStickyNoteId(stickyNote.noteId), position: stickyNote.layoutData }; | ||
| }); | ||
| return { | ||
| stickyNotes: [...updatedStickyNotes], | ||
| layout: [...layout.filter((l) => l.id !== stickyNoteLayoutId), ...updatedLayout], | ||
| }; | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Add error handling for invalid sticky note IDs
The function assumes the sticky note exists. Consider adding validation to handle cases where the note ID doesn't exist.
export function removeStickyNoteFromLayout(state: GraphState, stickyNoteId: number): { layout: NodePosition[]; stickyNotes: StickyNote[] } {
const { layout } = state;
const stickyNoteLayoutId = createStickyNoteId(stickyNoteId);
+ const existingNote = state.stickyNotes.find((n) => n.noteId === stickyNoteId);
+ if (!existingNote) {
+ throw new Error(`Sticky note with ID ${stickyNoteId} not found`);
+ }
const updatedStickyNotes = state.stickyNotes.filter((n) => n.noteId !== stickyNoteId);📝 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.
| export function removeStickyNoteFromLayout(state: GraphState, stickyNoteId: number): { layout: NodePosition[]; stickyNotes: StickyNote[] } { | |
| const { layout } = state; | |
| const stickyNoteLayoutId = createStickyNoteId(stickyNoteId); | |
| const updatedStickyNotes = state.stickyNotes.filter((n) => n.noteId !== stickyNoteId); | |
| const updatedLayout = updatedStickyNotes.map((stickyNote) => { | |
| return { id: createStickyNoteId(stickyNote.noteId), position: stickyNote.layoutData }; | |
| }); | |
| return { | |
| stickyNotes: [...updatedStickyNotes], | |
| layout: [...layout.filter((l) => l.id !== stickyNoteLayoutId), ...updatedLayout], | |
| }; | |
| } | |
| export function removeStickyNoteFromLayout(state: GraphState, stickyNoteId: number): { layout: NodePosition[]; stickyNotes: StickyNote[] } { | |
| const { layout } = state; | |
| const stickyNoteLayoutId = createStickyNoteId(stickyNoteId); | |
| const existingNote = state.stickyNotes.find((n) => n.noteId === stickyNoteId); | |
| if (!existingNote) { | |
| throw new Error(`Sticky note with ID ${stickyNoteId} not found`); | |
| } | |
| const updatedStickyNotes = state.stickyNotes.filter((n) => n.noteId !== stickyNoteId); | |
| const updatedLayout = updatedStickyNotes.map((stickyNote) => { | |
| return { id: createStickyNoteId(stickyNote.noteId), position: stickyNote.layoutData }; | |
| }); | |
| return { | |
| stickyNotes: [...updatedStickyNotes], | |
| layout: [...layout.filter((l) => l.id !== stickyNoteLayoutId), ...updatedLayout], | |
| }; | |
| } |
| private def addStickyNoteForScenario( | ||
| scenarioName: ProcessName, | ||
| request: StickyNoteAddRequest | ||
| ): Future[StickyNoteCorrelationId] = { | ||
| for { | ||
| scenarioId <- futureFetchingScenarioRepository.fetchProcessId(scenarioName).map(_.get) | ||
| noteCorrelationId <- dbioRunner.runInTransaction( | ||
| stickyNotesRepository.addStickyNote( | ||
| request.content, | ||
| request.layoutData, | ||
| request.color, | ||
| request.dimensions, | ||
| request.targetEdge, | ||
| scenarioId, | ||
| request.scenarioVersionId | ||
| ) | ||
| ) | ||
| } yield noteCorrelationId | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Enhance error handling in addStickyNoteForScenario
The method should handle potential database errors more gracefully and provide meaningful error messages.
private def addStickyNoteForScenario(
scenarioName: ProcessName,
request: StickyNoteAddRequest
): Future[StickyNoteCorrelationId] = {
for {
- scenarioId <- futureFetchingScenarioRepository.fetchProcessId(scenarioName).map(_.get)
+ scenarioId <- futureFetchingScenarioRepository.fetchProcessId(scenarioName).flatMap {
+ case Some(id) => Future.successful(id)
+ case None => Future.failed(new IllegalArgumentException(s"Scenario not found: $scenarioName"))
+ }
noteCorrelationId <- dbioRunner.runInTransaction(
stickyNotesRepository.addStickyNote(Committable suggestion skipped: line range outside the PR's diff.
| updateStickyNote(scenarioName: string, scenarioVersionId: number, stickyNote: StickyNote) { | ||
| const promise = api.put(`/processes/${encodeURIComponent(scenarioName)}/stickyNotes`, { | ||
| noteId: stickyNote.noteId, | ||
| scenarioVersionId, | ||
| content: stickyNote.content, | ||
| layoutData: stickyNote.layoutData, | ||
| color: stickyNote.color, | ||
| dimensions: stickyNote.dimensions, | ||
| }); | ||
| promise.catch((error) => | ||
| this.#addError( | ||
| i18next.t("notification.error.failedToUpdateStickyNote", "Failed to update sticky note for scenario"), | ||
| error, | ||
| true, | ||
| ), | ||
| ); | ||
| return promise; | ||
| } |
There was a problem hiding this comment.
Sanitize content before sending to the server.
The updateStickyNote method should sanitize the content using DOMPurify before sending it to the server.
Add content sanitization:
+import DOMPurify from 'dompurify';
updateStickyNote(scenarioName: string, scenarioVersionId: number, stickyNote: StickyNote) {
const promise = api.put(`/processes/${encodeURIComponent(scenarioName)}/stickyNotes`, {
noteId: stickyNote.noteId,
scenarioVersionId,
- content: stickyNote.content,
+ content: DOMPurify.sanitize(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"),
error,
true,
),
);
return promise;
}Committable suggestion skipped: line range outside the PR's diff.
| addStickyNote(scenarioName: string, scenarioVersionId: number, position: Position, dimensions: Dimensions) { | ||
| const promise = api.post(`/processes/${encodeURIComponent(scenarioName)}/stickyNotes`, { | ||
| scenarioVersionId, | ||
| content: "", | ||
| layoutData: position, | ||
| color: STICKY_NOTE_DEFAULT_COLOR, //TODO add config for default sticky note color? For now this is default. | ||
| dimensions: dimensions, | ||
| }); | ||
| promise.catch((error) => | ||
| this.#addError(i18next.t("notification.error.failedToAddStickyNote", "Failed to add sticky note to scenario"), error, true), | ||
| ); | ||
| return promise; | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Consider validating dimensions and position.
The addStickyNote method should validate the position and dimensions before sending the request.
Add validation:
addStickyNote(scenarioName: string, scenarioVersionId: number, position: Position, dimensions: Dimensions) {
+ if (position.x < 0 || position.y < 0) {
+ return Promise.reject(new Error("Position coordinates must be non-negative"));
+ }
+ if (dimensions.width <= 0 || dimensions.height <= 0) {
+ return Promise.reject(new Error("Dimensions must be positive"));
+ }
const promise = api.post(`/processes/${encodeURIComponent(scenarioName)}/stickyNotes`, {
scenarioVersionId,
content: "",
layoutData: position,
color: STICKY_NOTE_DEFAULT_COLOR,
dimensions: dimensions,
});
promise.catch((error) =>
this.#addError(i18next.t("notification.error.failedToAddStickyNote", "Failed to add sticky note to scenario"), error, true),
);
return promise;
}📝 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.
| addStickyNote(scenarioName: string, scenarioVersionId: number, position: Position, dimensions: Dimensions) { | |
| const promise = api.post(`/processes/${encodeURIComponent(scenarioName)}/stickyNotes`, { | |
| scenarioVersionId, | |
| content: "", | |
| layoutData: position, | |
| color: STICKY_NOTE_DEFAULT_COLOR, //TODO add config for default sticky note color? For now this is default. | |
| dimensions: dimensions, | |
| }); | |
| promise.catch((error) => | |
| this.#addError(i18next.t("notification.error.failedToAddStickyNote", "Failed to add sticky note to scenario"), error, true), | |
| ); | |
| return promise; | |
| } | |
| addStickyNote(scenarioName: string, scenarioVersionId: number, position: Position, dimensions: Dimensions) { | |
| if (position.x < 0 || position.y < 0) { | |
| return Promise.reject(new Error("Position coordinates must be non-negative")); | |
| } | |
| if (dimensions.width <= 0 || dimensions.height <= 0) { | |
| return Promise.reject(new Error("Dimensions must be positive")); | |
| } | |
| const promise = api.post(`/processes/${encodeURIComponent(scenarioName)}/stickyNotes`, { | |
| scenarioVersionId, | |
| content: "", | |
| layoutData: position, | |
| color: STICKY_NOTE_DEFAULT_COLOR, //TODO add config for default sticky note color? For now this is default. | |
| dimensions: dimensions, | |
| }); | |
| promise.catch((error) => | |
| this.#addError(i18next.t("notification.error.failedToAddStickyNote", "Failed to add sticky note to scenario"), error, true), | |
| ); | |
| return promise; | |
| } |
Describe your changes
Checklist before merge
Summary by CodeRabbit
New Features
Improvements
Documentation