Skip to content

Conversation

dyon21
Copy link
Collaborator

@dyon21 dyon21 commented Aug 2, 2025

No description provided.

@dyon21 dyon21 requested a review from MrCoder August 2, 2025 08:42
@MrCoder
Copy link
Collaborator

MrCoder commented Aug 17, 2025

@CodiumAI-Agent /improve

@CodiumAI-Agent
Copy link

PR Code Suggestions ✨

CategorySuggestion                                                                                                                                    Impact
Possible issue
Correct drag coordinate normalization

Fix the coordinate scaling order to avoid incorrect drag line positioning. Subtract
the diagram's origin first, then divide by scale, mirroring how DragLine computes
mouse coordinates.

src/components/DiagramFrame/SeqDiagram/LifeLineLayer/Participant.tsx [101-114]

 const handleDrag = () => {
   const {
     left = 0,
     top = 0,
     width = 0,
     height = 0,
   } = elRef.current?.getBoundingClientRect() || {};
   const diagramRect = diagramElement?.getBoundingClientRect();
+  const dx = left + width / 2 - (diagramRect?.left || 0);
+  const dy = top + height / 2 - (diagramRect?.top || 0);
   setDragParticipant({
     name: props.entity.name,
-    x: (left + width / 2) / scale - (diagramRect?.left || 0),
-    y: (top + height / 2) / scale - (diagramRect?.top || 0),
+    x: dx / scale,
+    y: dy / scale,
   });
 };
Suggestion importance[1-10]: 7

__

Why: The suggestion aligns drag coordinate normalization with the approach used in DragLine by subtracting diagram origin before dividing by scale, reducing positioning errors. It is contextually accurate and improves correctness, though not a critical bug fix.

Medium
Normalize anchor coords consistently

Align anchor coordinates with the same normalization as mouse/drag computations.
Compute delta to diagram origin in pixel space first, then divide by scale to
prevent scaling errors.

src/components/DiagramFrame/SeqDiagram/MessageLayer/Anchor.tsx [161-173]

+const dx = left - (diagramRect?.left || 0);
+const dy =
+  top + height / 2 - (diagramRect?.top || 0) + (props.offset ?? 25);
 setAnchor({
   id: id.current,
   context: props.context,
   index: props.index,
   participant: props.participant,
-  x: (left - (diagramRect?.left || 0)) / scale,
-  y:
-    (top +
-      height / 2 -
-      (diagramRect?.top || 0) +
-      (props.offset ?? 25)) /
-    scale,
+  x: dx / scale,
+  y: dy / scale,
 });
Suggestion importance[1-10]: 6

__

Why: This mirrors the same normalization logic (subtract then divide) for anchor positions, improving consistency and reducing scaling drift. It’s a reasonable minor fix; impact is moderate.

Low
General
Guard drop to valid target

Prevent unwanted drops when not dragging an anchor or when dragAnchor belongs to a
different participant. Guard onMouseUp to only trigger handleDrop for the matching
participant context.

src/components/DiagramFrame/SeqDiagram/LifeLineLayer/LifeLine.tsx [106-114]

 {props.renderLifeLine && (
   <>
     <div
       className={cn(
-        "absolute top-0 bottom-0 -left-2.5 -right-2.5 hover:bg-sky-200 cursor-copy",
+        "absolute top-0 bottom-0 -left-2.5 -right-2.5 hover:bg-sky-200",
+        dragAnchor ? "cursor-copy" : "cursor-default",
         dragAnchor ? "visible" : "invisible",
       )}
-      onMouseUp={handleDrop}
+      onMouseUp={() => {
+        if (dragAnchor && dragAnchor.participant === props.entity.name) {
+          handleDrop();
+        }
+      }}
     />
     <div className="relative line mx-auto flex-grow w-px bg-[linear-gradient(to_bottom,transparent_50%,var(--color-border-base)_50%)] bg-[length:1px_10px] pointer-events-none" />
   </>
 )}
Suggestion importance[1-10]: 5

__

Why: Adding a guard to only drop when the dragged anchor matches the participant can prevent unintended drops, improving robustness. However, useAnchorDrop already checks dragAnchor existence and context, so impact is limited.

Low

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants