Skip to content

fix(connectionHandler): apply perimeter style to new edges#1026

Open
LOUISNOYEZ wants to merge 2 commits intomaxGraph:mainfrom
LOUISNOYEZ:fix/connection_handler_new_edge_perimeter_prop_value
Open

fix(connectionHandler): apply perimeter style to new edges#1026
LOUISNOYEZ wants to merge 2 commits intomaxGraph:mainfrom
LOUISNOYEZ:fix/connection_handler_new_edge_perimeter_prop_value

Conversation

@LOUISNOYEZ
Copy link
Copy Markdown

@LOUISNOYEZ LOUISNOYEZ commented Mar 24, 2026

PR Checklist

  • Addresses an existing open issue: closes FixedPoints Story: the edge are connected on a rectangle perimeter instead on the connection constraints of the vertex #840.
  • You have discussed this issue with the maintainers of maxGraph, and you are assigned to the issue.
  • The scope of the PR is sufficiently narrow to be examined in a single session. A PR covering several issues must be split into separate PRs. Do not create a large PR, otherwise it cannot be reviewed and you will be asked to split it later or the PR will be closed.
  • I have added tests to prove my fix is effective or my feature works. This can be done in the form of automatic tests in packages/core/_tests_ or a new or altered Storybook story in packages/html/stories (an existing story may also demonstrate the change).
  • I have provided screenshot/videos to demonstrate the change. If no releavant, explain why.
  • I have added or edited necessary documentation, or no docs changes are needed.
  • The PR title follows the "Conventional Commits" guidelines.

Overview

Restore mxgraph new edges endpoints positionning when using the connection handler to connect vertex cell shapes with non rectangular perimeters.

Bug description

When using the connection handler to connect cell shapes which have a non rectangular perimeter, the new edge preview displays correctly, but when the new edge is created, its endpoints are connected to connection points on the rectangular bounding box of the cell shape instead of to the connection points projected on the perimeter of cell shapes.
This pull request is intended to fix issue #840 although this bug occurs on any graph which uses the connection handler and vertex with non rectangular perimeters.

Demonstration using the Fixed Points story

  • Current maxgraph behavior : The new edge connects to the bounding box of the cell shape instead of connecting to the actual perimeter of the cell shape.

    connection_perimeter_wrong.mp4
  • Intended mxgraph behavior : the new edge connects directly to the perimeter of the cell shape.

    connection_perimeter_right.mp4

Analysis

As per the migration guide from mxgraph to maxgraph, some styling properties that used to be set using strings "0" or "1"' have been replaced with boolean values.

Property type changed from `number` (0 or 1) to `boolean` (if not specified, from [email protected]):

This includes the properties entryPerimeter and exitPerimeter of edge's style, which dictate whether edges should take the perimeter into account when connecting.

- `entryPerimeter`
- `exitPerimeter`

This type change has been taken into account accross the codebase, but not everywhere. Specifically it has not been taken into account for the connection handler in its connection handler mixin.

When creating new edges via connections, this style property is still set using a string in the setConnectionConstraint method, and they are thus ignored :

if (!constraint.perimeter) {
this.setCellStyles(source ? 'exitPerimeter' : 'entryPerimeter', '0', [edge]);
} else {
this.setCellStyles(source ? 'exitPerimeter' : 'entryPerimeter', null, [edge]);
}

This issue is fixed by setting the appropriate values :

    if (!constraint.perimeter) {
      this.setCellStyles(source ? 'exitPerimeter' : 'entryPerimeter', false, [edge]);
    } else {
      this.setCellStyles(source ? 'exitPerimeter' : 'entryPerimeter', true, [edge]);
    }

Notes

  • I've added no tests under packages/core/__tests__. Correctness can be assed using any story involving the connection handler and cell shapes with non rectangle perimeters.
  • The setCellStyles method does not take advantage of the typescript type checker to catch these kinds of mistakes statically. Its signature is
    setCellStyles: (key: keyof CellStateStyle, value: CellStateStyle[keyof CellStateStyle], cells?: Cell[]) => void
    However there is no direct correlation between the first argument key and the type of the second argument value. Its type is a union of all types of CellStateStyle properties, which includes string, hence why typescript doesn't catch the error. I believe a possible solution might be to pass an object to setCellStyles to update styles instead of two separate arguments in order to catch errors, maybe
    setCellStyles: (style: Partial<CellStateStyle>, cells?: Cell[]) => void
    This change is probably far reaching and would probably involve a large refactor.
  • It's probably not a good idea for to use ternary operators in arguments to resolve keys at runtime, which might confuse the typescript type checker.

Keywords

closes #840

Summary by CodeRabbit

  • Bug Fixes
    • Improved handling of connection perimeter constraints so connection styling is applied consistently, leading to more predictable and reliable connection behavior.

@coderabbitai
Copy link
Copy Markdown

coderabbitai bot commented Mar 24, 2026

Walkthrough

Changed ConnectionsMixin.setConnectionConstraint to assign explicit boolean values (true/false) to exitPerimeter and entryPerimeter style properties instead of the previous mixed string/null values.

Changes

Cohort / File(s) Summary
Perimeter Constraint Handling
packages/core/src/view/mixin/ConnectionsMixin.ts
Replaced '0'/null style assignments for exitPerimeter and entryPerimeter with explicit booleans (false/true) to make perimeter style values type-consistent.

Estimated code review effort

🎯 2 (Simple) | ⏱️ ~10 minutes

Suggested labels

bug

🚥 Pre-merge checks | ✅ 5
✅ Passed checks (5 passed)
Check name Status Explanation
Title check ✅ Passed Title clearly summarizes the main change: fixing perimeter style application to new edges in the connection handler.
Linked Issues check ✅ Passed The code change directly addresses issue #840 by fixing entryPerimeter/exitPerimeter values from strings to booleans in setConnectionConstraint, restoring mxGraph perimeter-based connection behavior.
Out of Scope Changes check ✅ Passed All changes in ConnectionsMixin.ts are focused and directly related to fixing the perimeter constraint styling issue described in issue #840.
Docstring Coverage ✅ Passed No functions found in the changed files to evaluate docstring coverage. Skipping docstring coverage check.
Description check ✅ Passed PR description comprehensively covers all required template sections with clear issue reference, detailed analysis, demonstration videos, and explicit checkbox confirmations.

✏️ Tip: You can configure your own custom pre-merge checks in the settings.


Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out.

❤️ Share

Comment @coderabbitai help to get the list of available commands and usage tips.

@LOUISNOYEZ LOUISNOYEZ marked this pull request as ready for review March 24, 2026 13:04
Copy link
Copy Markdown

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

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

🧹 Nitpick comments (1)
packages/core/src/view/mixin/ConnectionsMixin.ts (1)

202-207: Outdated comment no longer reflects the code behavior.

The comment "Only writes 0 since 1 is default" is now misleading:

  1. The code writes false/true (booleans), not 0/1 (strings)
  2. The code now writes both false and true, not just the non-default value

Consider updating or removing the comment to reflect the current behavior.

📝 Suggested comment update or simplification

Option 1: Update the comment

-          // Only writes 0 since 1 is default
+          // Writes perimeter constraint as boolean style value
           if (!constraint.perimeter) {
             this.setCellStyles(source ? 'exitPerimeter' : 'entryPerimeter', false, [edge]);
           } else {
             this.setCellStyles(source ? 'exitPerimeter' : 'entryPerimeter', true, [edge]);
           }

Option 2: Simplify the logic (removes need for comment)

-          // Only writes 0 since 1 is default
-          if (!constraint.perimeter) {
-            this.setCellStyles(source ? 'exitPerimeter' : 'entryPerimeter', false, [edge]);
-          } else {
-            this.setCellStyles(source ? 'exitPerimeter' : 'entryPerimeter', true, [edge]);
-          }
+          this.setCellStyles(
+            source ? 'exitPerimeter' : 'entryPerimeter',
+            !!constraint.perimeter,
+            [edge]
+          );

ℹ️ Review info
⚙️ Run configuration

Configuration used: Organization UI

Review profile: CHILL

Plan: Pro

Run ID: 67284926-e1d8-4764-8741-f19bc7d0b515

📥 Commits

Reviewing files that changed from the base of the PR and between 24357a7 and 7541353.

📒 Files selected for processing (1)
  • packages/core/src/view/mixin/ConnectionsMixin.ts

Copy link
Copy Markdown
Member

@tbouffard tbouffard left a comment

Choose a reason for hiding this comment

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

Thanks for your contribution.

There are some minor errors to fix.
I need to manually test storybook with this PR, but it looks fine.

Regarding the possible improvements in the signature of setCellStyles, I have proposed #1028 that is able to catch errors like the one fixed here

this.setCellStyles(source ? 'exitPerimeter' : 'entryPerimeter', false, [edge]);
} else {
this.setCellStyles(source ? 'exitPerimeter' : 'entryPerimeter', null, [edge]);
this.setCellStyles(source ? 'exitPerimeter' : 'entryPerimeter', true, [edge]);
Copy link
Copy Markdown
Member

@tbouffard tbouffard Mar 24, 2026

Choose a reason for hiding this comment

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

suggestion: here we can keep null, the default value will be then used (true). This is what the comment line 202 tries to explain (I guess).

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

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

From my tests in the Fixed Points story, the new edge fails to connect to the perimeter of the shape when the value is set to null. I've tried to search through the files but I couldn't find code indicating that the style value would default to true when the value null is used. The properties entryPerimeter and exitPerimeter do not appear to be set to a default value in the default stylesheet.
Incidentally, entryPerimeter/exitPerimeter is also set to null on line 195. I'm not sure if the intent is to set it to default, but I cannot find any default. I assume maxgraph treats it as "false" in this case.

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.

OK, fine, let's set it to true, this is also more explicit.

The default value for exitPerimeter is documented in

/**
* Defines if the perimeter should be used to find the exact entry point along the perimeter
* of the source.
* @default true
*/
exitPerimeter?: boolean;
and it is supposed to be true (the same applies to entryPerimeter).

In mxGraph, the default was actually true.

https://github.com/jgraph/mxgraph/blob/ff141aab158417bd866e2dfebd06c61d40773cd2/javascript/src/js/view/mxGraph.js#L6787-L6793

perimeter = mxUtils.getValue(edge.style, (source) ? mxConstants.STYLE_EXIT_PERIMETER :
			mxConstants.STYLE_ENTRY_PERIMETER, true);

But what I see in the maxGraph code is that the default is false 😭

let perimeter = false;
let dx = 0;
let dy = 0;
if (point) {
perimeter = edge.style[source ? 'exitPerimeter' : 'entryPerimeter'] || false;

The default was true in c89ce4c, removed in 648e324, then set to false in 413796a

Do you think we should fix this in this PR or do we create another PR? This may impact several features/stories, so maybe it is safer to treat this in a dedicated PR.

@sonarqubecloud
Copy link
Copy Markdown

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.

FixedPoints Story: the edge are connected on a rectangle perimeter instead on the connection constraints of the vertex

2 participants