Skip to content
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 2 additions & 0 deletions src/features/cseMachine/CseMachineConfig.ts
Original file line number Diff line number Diff line change
Expand Up @@ -66,13 +66,15 @@ export const Config = Object.freeze({
PrintStrokeColor: '#777',
PrintStrokeColorFaded: '#ccc',
ArrowHighlightedColor: '#ffffff',
ArrowDeadHighlightedColor: '#787777',

// Colors of different states
ActiveColor: '#030fff',
PrintActiveColor: '#3d5afe',
DangerColor: '#ff1744',
PrintDangerColor: '#f44336',
HoverColor: '#25c225',
HoverDeadColor: '#127a12',
PrintHoverColor: '#0dbf0d',

// Colors for text hover background
Expand Down
2 changes: 2 additions & 0 deletions src/features/cseMachine/CseMachineTypes.ts
Original file line number Diff line number Diff line change
Expand Up @@ -40,6 +40,8 @@ export interface IVisible extends Drawable {
height(): number;

ref?: React.RefObject<any>;
setArrowSourceHighlightedStyle?(): void;
setArrowSourceNormalStyle?(): void;
}

/** unassigned is internally represented as a symbol */
Expand Down
16 changes: 16 additions & 0 deletions src/features/cseMachine/components/ArrayUnit.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -60,6 +60,22 @@ export class ArrayUnit extends Visible {
if (!CseMachine.getPrintableMode()) this.indexRef.current?.hide();
}

setArrowSourceHighlightedStyle(): void {
if (this.parent.isLive()) {
this.ref.current?.stroke(Config.HoverColor);
} else {
this.ref.current?.stroke(Config.HoverDeadColor);
}
}

setArrowSourceNormalStyle(): void {
this.ref.current?.stroke(
this.parent.isReferenced() && this.parent.isEnclosingFrameLive()
? defaultStrokeColor()
: fadedStrokeColor()
);
Comment thread
Akshay-2007-1 marked this conversation as resolved.
}

draw(): React.ReactNode {
if (this.isDrawn()) return null;
this._isDrawn = true;
Expand Down
10 changes: 10 additions & 0 deletions src/features/cseMachine/components/ControlItemComponent.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -92,6 +92,16 @@ export class ControlItemComponent extends Visible implements IHoverable {
this.ref.current.zIndex(this.zIndex);
};

setArrowSourceHighlightedStyle(): void {
this.tag?.stroke(Config.HoverColor);
this.secItem?.fill(Config.HoverColor);
}

setArrowSourceNormalStyle(): void {
this.tag?.stroke(this.topItem ? defaultActiveColor() : defaultStrokeColor());
this.secItem?.fill(defaultTextColor());
}
Comment thread
Akshay-2007-1 marked this conversation as resolved.

destroy() {
this.ref.current.destroyChildren();
}
Expand Down
23 changes: 23 additions & 0 deletions src/features/cseMachine/components/Frame.tsx
Original file line number Diff line number Diff line change
@@ -1,3 +1,4 @@
import { Rect as KonvaRect } from 'konva/lib/shapes/Rect';
import React from 'react';
import { Group, Rect } from 'react-konva';

Expand Down Expand Up @@ -59,6 +60,7 @@ export class Frame extends Visible implements IHoverable {
readonly bindings: Binding[] = [];
/** name of this frame to display */
private _name!: Text; // removed readonly to allow reassignment for fixed layout
private readonly rectRef = React.createRef<KonvaRect | null>();
/** the level in which this frame resides */
readonly level: Level | undefined;
/** environment associated with this frame */
Expand Down Expand Up @@ -260,13 +262,34 @@ export class Frame extends Visible implements IHoverable {

onMouseLeave = () => {};

setArrowSourceHighlightedStyle(): void {
if (this.isLive) {
this.rectRef.current?.stroke(Config.HoverColor);
} else {
this.rectRef.current?.stroke(Config.HoverDeadColor);
}
this.name.setArrowSourceHighlightedStyle();
}

setArrowSourceNormalStyle(): void {
this.rectRef.current?.stroke(
CseMachine.getCurrentEnvId() === this.environment?.id
? defaultActiveColor()
: this.isLive
? defaultStrokeColor()
: fadedStrokeColor()
);
this.name.setArrowSourceNormalStyle();
}

draw(): React.ReactNode {
return (
<Group ref={this.ref} key={Layout.key++}>
{this.name.draw()}

<Rect
{...ShapeDefaultProps}
ref={this.rectRef}
x={this.x()}
y={this.y()}
width={this.width()}
Expand Down
10 changes: 10 additions & 0 deletions src/features/cseMachine/components/StashItemComponent.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -106,6 +106,16 @@ export class StashItemComponent extends Visible implements IHoverable {
this.ref.current.zIndex(this.zIndex);
};

setArrowSourceHighlightedStyle(): void {
this.tag?.stroke(Config.HoverColor);
this.secItem?.fill(Config.HoverColor);
}

setArrowSourceNormalStyle(): void {
this.tag?.stroke(isStashItemInDanger(this.index) ? defaultDangerColor() : defaultStrokeColor());
this.secItem?.fill(defaultTextColor());
}
Comment thread
Akshay-2007-1 marked this conversation as resolved.

destroy() {
this.ref.current.destroyChildren();
}
Expand Down
12 changes: 12 additions & 0 deletions src/features/cseMachine/components/Text.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -95,6 +95,18 @@ export class Text extends Visible implements IHoverable {
currentTarget.getLayer()?.draw();
};

setArrowSourceHighlightedStyle(): void {
if (this.options.faded) {
this.ref.current?.fill(Config.HoverDeadColor);
} else {
this.ref.current?.fill(Config.HoverColor);
}
}

setArrowSourceNormalStyle(): void {
this.ref.current?.fill(this.options.faded ? fadedTextColor() : defaultTextColor());
}

draw(): React.ReactNode {
const props = {
fontFamily: this.options.fontFamily,
Expand Down
22 changes: 22 additions & 0 deletions src/features/cseMachine/components/Visible.tsx
Original file line number Diff line number Diff line change
@@ -1,3 +1,4 @@
import Konva from 'konva';
import React from 'react';
import { RefObject } from 'react';

Expand Down Expand Up @@ -30,6 +31,27 @@ export abstract class Visible implements IVisible {
reset(): void {
this._isDrawn = false;
}
setShapesStyle(color: string): void {
const shapes = this.ref.current?.getChildren?.() ?? [];
shapes.forEach((shape: Konva.Node) => {
if (shape instanceof Konva.Shape) {
if (shape.attrs.stroke) {
shape.stroke(color);
}
if (shape.attrs.fill) {
shape.fill(color);
}
}
});
}
Comment thread
Akshay-2007-1 marked this conversation as resolved.
public ref: RefObject<any> = React.createRef();
protected get tag() {
return this.ref.current?.getChildren?.()[0];
}
protected get secItem() {
return this.ref.current?.getChildren?.()[1];
}
setArrowSourceHighlightedStyle(): void {}
setArrowSourceNormalStyle(): void {}
abstract draw(key?: number): React.ReactNode;
}
Original file line number Diff line number Diff line change
Expand Up @@ -16,6 +16,7 @@ class ArrowSelectionManager {
clearSelection(): GenericArrow<IVisible, IVisible> | null {
const oldArrow = this.selectedArrow;
this.selectedArrow = null;
oldArrow?.setNormalStyle();
return oldArrow;
}

Expand Down
14 changes: 6 additions & 8 deletions src/features/cseMachine/components/arrows/GenericArrow.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -146,7 +146,7 @@ export class GenericArrow<Source extends IVisible, Target extends IVisible>
* Subclasses can override this to provide custom hover colors.
*/
protected getHighlightedColor(): string {
return Config.ArrowHighlightedColor;
return this.isLive ? Config.ArrowHighlightedColor : Config.ArrowDeadHighlightedColor;
}

onMouseEnter = (e: KonvaEventObject<MouseEvent>) => {
Expand Down Expand Up @@ -189,8 +189,9 @@ export class GenericArrow<Source extends IVisible, Target extends IVisible>
this.arrowHeadRef.current.pointerWidth(Config.ArrowHoveredHeadSize);
this.arrowHeadRef.current.pointerLength(Config.ArrowHoveredHeadSize);
}
this.source.setArrowSourceHighlightedStyle?.();
this.target?.setArrowSourceHighlightedStyle?.();
}

public setNormalStyle() {
Comment thread
Akshay-2007-1 marked this conversation as resolved.
const color = this.isLive ? defaultStrokeColor() : fadedStrokeColor();
if (this.pathRef.current) {
Expand All @@ -202,19 +203,16 @@ export class GenericArrow<Source extends IVisible, Target extends IVisible>
this.arrowHeadRef.current.pointerWidth(Config.ArrowHeadSize);
this.arrowHeadRef.current.pointerLength(Config.ArrowHeadSize);
}
this.source.setArrowSourceNormalStyle?.();
this.target?.setArrowSourceNormalStyle?.();
Comment on lines 203 to +207

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Bug: Hovering away from an unselected arrow can incorrectly remove the highlight from a shared node that is part of a separate, selected arrow.
Severity: CRITICAL

Suggested Fix

Update the setNormalStyle() and setArrowSourceNormalStyle() methods. Before resetting a node's style, they should check the global selection state to verify if the node is part of any currently selected arrow. If it is, the highlight style should be maintained instead of being reset.

Prompt for AI Agent
Review the code at the location below. A potential bug has been identified by an AI
agent.
Verify if this is a real issue. If it is, propose a fix; if not, explain why it's not
valid.

Location: src/features/cseMachine/components/arrows/GenericArrow.tsx#L203-L207

Potential issue: When an arrow is selected, its source and target nodes are highlighted.
If the user then hovers over and away from a different, unselected arrow that shares a
source or target node, the shared node's highlight is incorrectly removed. This occurs
because the `onMouseLeave` event on the unselected arrow triggers `setNormalStyle()`,
which unconditionally resets the node's style via `setArrowSourceNormalStyle?.()`. This
reset logic is unaware that the node should remain highlighted because it is part of
another arrow that is still selected, leading to incorrect visual feedback for the user.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

doc_2026-03-26_14-15-48.mp4

This is a real issue!
@gemini-code-assist how can we fix this simply

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You are absolutely right, this is a critical issue that needs to be addressed. The bug occurs because setArrowSourceNormalStyle() is called unconditionally on onMouseLeave for an unselected arrow, which can inadvertently remove the highlight from a shared node that is still part of a selected arrow. This leads to incorrect visual feedback.

To fix this simply, we need to ensure that a node's style is only reset if it is not currently highlighted by the globally selected arrow. This requires a two-part change:

  1. Add a helper method to ArrowSelectionManager: This method will check if a given IVisible node is the source or target of the currently selected arrow. You can add this method to src/features/cseMachine/components/arrows/ArrowSelection.ts:

    // In src/features/cseMachine/components/arrows/ArrowSelection.ts
    // Inside the ArrowSelectionManager class
    isNodeHighlightedBySelectedArrow(node: IVisible): boolean {
      if (!this.selectedArrow) {
        return false;
      }
      return this.selectedArrow.source === node || this.selectedArrow.target === node;
    }
  2. Modify GenericArrow.setNormalStyle(): Update the setNormalStyle method in src/features/cseMachine/components/arrows/GenericArrow.tsx to use this new helper method. This will prevent the source or target node's style from being reset if it's part of the currently selected arrow.

I've provided a code suggestion below for the change in GenericArrow.tsx that directly addresses the lines in this comment thread.

Suggested change
this.arrowHeadRef.current.pointerWidth(Config.ArrowHeadSize);
this.arrowHeadRef.current.pointerLength(Config.ArrowHeadSize);
}
this.source.setArrowSourceNormalStyle?.();
this.target?.setArrowSourceNormalStyle?.();
this.arrowHeadRef.current.pointerWidth(Config.ArrowHeadSize);
this.arrowHeadRef.current.pointerLength(Config.ArrowHeadSize);
}
// Only reset source/target style if they are not part of the currently selected arrow
if (this.source && !arrowSelection.isNodeHighlightedBySelectedArrow(this.source)) {
this.source.setArrowSourceNormalStyle?.();
}
if (this.target && !arrowSelection.isNodeHighlightedBySelectedArrow(this.target)) {
this.target.setArrowSourceNormalStyle?.();
}

@Akshay-2007-1 Akshay-2007-1 Mar 26, 2026

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@martin-henz @RichDom2185 @Oofky
I would appreciate your help and suggestions as to if this is necessary or we can ignore it since its a minor feature?

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

yes, feel free to ignore. You may want to add a comment why.

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, can ignore from this PR. But please create an issue that references this comment thread.

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I've created it: #3696

}

onClick = (e: KonvaEventObject<MouseEvent>) => {
e.cancelBubble = true;

// Toggle selection - clear first, then select if it wasn't already selected
const wasSelected = this.isSelected();
const oldArrow = arrowSelection.clearSelection();

// Update old arrow's visual state
if (oldArrow && oldArrow !== this) {
oldArrow.setNormalStyle();
}
arrowSelection.clearSelection();

if (!wasSelected) {
this.select();
Expand Down
8 changes: 8 additions & 0 deletions src/features/cseMachine/components/values/ArrayValue.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -109,6 +109,14 @@ export class ArrayValue extends Value implements IHoverable {
}
}

setArrowSourceHighlightedStyle(): void {
this.units.forEach(unit => unit.setArrowSourceHighlightedStyle());
}

setArrowSourceNormalStyle(): void {
this.units.forEach(unit => unit.setArrowSourceNormalStyle());
}

isEnclosingFrameLive(): boolean {
const id = (this.data as any).id;
return id ? Layout.liveObjectIDs.has(id) : false;
Expand Down
13 changes: 13 additions & 0 deletions src/features/cseMachine/components/values/ContValue.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -128,6 +128,19 @@ export class ContValue extends Value implements IHoverable {
this.labelRef.current?.hide();
};

setArrowSourceHighlightedStyle(): void {
if (this.isLive()) {
this.setShapesStyle(Config.HoverColor);
} else {
this.setShapesStyle(Config.HoverDeadColor);
}
}

setArrowSourceNormalStyle(): void {
const strokeColor = this.isLive() ? defaultStrokeColor() : fadedStrokeColor();
this.setShapesStyle(strokeColor);
}
Comment thread
Akshay-2007-1 marked this conversation as resolved.

draw(): React.ReactNode {
if (this.enclosingFrame) {
this._arrow = new ArrowFromFn(this).to(this.enclosingFrame) as ArrowFromFn;
Expand Down
13 changes: 13 additions & 0 deletions src/features/cseMachine/components/values/FnValue.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -169,6 +169,19 @@ export class FnValue extends Value implements IHoverable {
currentTarget.getLayer()?.batchDraw();
};

setArrowSourceHighlightedStyle(): void {
if (this.isLive()) {
this.setShapesStyle(Config.HoverColor);
} else {
this.setShapesStyle(Config.HoverDeadColor);
}
}

setArrowSourceNormalStyle(): void {
const strokeColor = this.isLive() ? defaultStrokeColor() : fadedStrokeColor();
this.setShapesStyle(strokeColor);
}
Comment thread
Akshay-2007-1 marked this conversation as resolved.

isLive(): boolean {
const id = (this.data as any).id;
return id ? Layout.liveObjectIDs.has(id) : false;
Expand Down
13 changes: 13 additions & 0 deletions src/features/cseMachine/components/values/GlobalFnValue.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -142,6 +142,19 @@ export class GlobalFnValue extends Value implements IHoverable {
currentTarget.getLayer()?.batchDraw();
};

setArrowSourceHighlightedStyle(): void {
if (this.isReferenced()) {
this.setShapesStyle(Config.HoverColor);
} else {
this.setShapesStyle(Config.HoverDeadColor);
}
}

setArrowSourceNormalStyle(): void {
const strokeColor = this.isReferenced() ? defaultStrokeColor() : fadedStrokeColor();
this.setShapesStyle(strokeColor);
}
Comment thread
Akshay-2007-1 marked this conversation as resolved.

draw(): React.ReactNode {
this._isDrawn = true;
if (Layout.globalEnvNode.frame) {
Expand Down
Loading