Skip to content

fix: consistently compute the arcSize for edges and vertices #884

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom
Merged
tbouffard merged 1 commit into from
Jul 23, 2025
Merged

fix: consistently compute the arcSize for edges and vertices #884

tbouffard merged 1 commit into from
Jul 23, 2025

Conversation

@tbouffard
Copy link
Member

@tbouffard tbouffard commented Jul 23, 2025 •
edited by coderabbitai bot
Loading

The arcSize of edges missed a division by 2, which was inconsistent with the past mxGraph behavior and the computation done for vertices.
This had been introduced by mistake in 0453da2.
The result of getSwimlaneArcSize was only correct when fallback to the default value. Otherwise, it missed the division by 2 as well.

In addition, refactor:

  • remove duplication in various shapes (introduce Shape.getBaseArcSize)
  • simplify implementations
  • add tests
  • improve JSDOC for CellStyle.arcSize

BREAKING CHANGES: The arcSize of rounded shapes was not always correctly computed in the past.
The computation is now consistent everywhere in the code and matches the mxGraph behavior.
To have the same rendering as before for edges, you must multiply the arcSize by 2 in your styles.

Notes

Here are the comparison between the mxGraph code and the incorrectly migrated code in maxGraph.
It includes a diff showing a correct implementation. This PR implements it differently after refactoring and code simplification.

SwimlaneShape.getSwimlaneArcSize

Division by 2 not correctly positioned (missing  parenthesis):

- return Math.min(w / 2, Math.min(h / 2, this.style?.arcSize ?? LINE_ARCSIZE / 2));
+ return Math.min(w / 2, Math.min(h / 2, (this.style?.arcSize ?? LINE_ARCSIZE) / 2));

PolylineShape.paintLine

Missing division by 2:

- const arcSize = this.style?.arcSize ?? LINE_ARCSIZE; 
+ const arcSize = (this.style?.arcSize ?? LINE_ARCSIZE) / 2;

Summary by CodeRabbit

  • Documentation
    • Updated changelog and in-code documentation to clarify the behavior of the arcSize property for rounded shapes and its impact on rendering.
  • Refactor
    • Centralized and standardized arc size computation across all relevant shapes for consistent rendering.
  • Tests
    • Added comprehensive tests for arc size calculations in various shape classes to ensure correct behavior and edge case handling.

@tbouffard
fix: consistently compute the arcSize for edges and vertices
e42d903 
The arcSize of edges missed a division by 2, which was inconsistent with the past mxGraph behavior and the computation
done for vertices. This had been introduced by mistake in 0453da2.
The result of `getSwimlaneArcSize` was only correct when fallback to the default value. Otherwise, it missed the
division by 2 as well.

In addition, refactor:
  - remove duplication in various shapes (introduce Shape.getBaseArcSize)
  - simplify implementations
  - add tests
  - improve JSDOC for CellStyle.arcSize

BREAKING CHANGES: The `arcSize` of rounded shapes was not always correctly computed in the past.
The computation is now consistent everywhere in the code and matches the mxGraph behavior.
To have the same rendering as before for edges, you must multiply the `arcSize` by `2` in your styles.
@tbouffard tbouffard added the bug Something isn't working label Jul 23, 2025
@coderabbitai
Copy link

coderabbitai bot commented Jul 23, 2025 •
edited
Loading

Walkthrough

This change refactors how the arcSize property is computed for rounded shapes, ensuring consistent behavior across the codebase and aligning with the original mxGraph implementation. The update introduces a new helper method for arc size calculation, updates documentation, and adds comprehensive tests for the affected shape classes. Several shape classes and their rendering methods are updated to use the new calculation logic.

Changes

Files / Areas Change Summary
CHANGELOG.md Updated changelog to document breaking change in arcSize computation and clarify plugin method notes.
packages/core/__tests__/view/shape/Shape.test.ts
.../edge/PolyLineShape.test.ts
.../node/SwimlaneShape.test.ts		 Added comprehensive Jest tests for Shape.getArcSize, PolylineShape.paintLine, and SwimlaneShape.getSwimlaneArcSize.
packages/core/src/types.ts Clarified documentation for the arcSize property in the CellStateStyle type.
packages/core/src/util/Constants.ts Updated JSDoc comments for RECTANGLE_ROUNDING_FACTOR and LINE_ARCSIZE.
packages/core/src/view/shape/Shape.ts Refactored getArcSize logic, added getBaseArcSize helper, updated related documentation comments.
.../edge/PolylineShape.ts
.../node/HexagonShape.ts
.../node/RhombusShape.ts
.../node/TriangleShape.ts		 Refactored arc size calculation to use getBaseArcSize() instead of direct constant or style property access.
.../node/RectangleShape.ts Simplified import statements and refactored paintBackground to use getArcSize.
.../node/SwimlaneShape.ts Refactored getSwimlaneArcSize to use getBaseArcSize and updated documentation.

Sequence Diagram(s)

sequenceDiagram
    participant Client
    participant Shape
    participant Style

    Client->>Shape: getArcSize(w, h)
    alt absoluteArcSize true
        Shape->>Shape: getBaseArcSize()
        Shape-->>Client: min(baseArcSize, min(w, h) / 2)
    else absoluteArcSize false or unset
        Shape->>Shape: getBaseArcSize()
        Shape-->>Client: min(w, h) * baseArcSize / 100
    end
Loading

Estimated code review effort

3 (~45 minutes)

Warning

There were issues while running some tools. Please review the errors and either fix the tool's configuration or disable the tool if it's a critical failure.

🔧 ESLint

If the error stems from missing dependencies, add them to the package.json file. For unrecoverable errors (e.g., due to private dependencies), disable the tool in the CodeRabbit configuration.

npm error Exit handler never called!
npm error This is an error with npm itself. Please report this error at:
npm error https://github.com/npm/cli/issues
npm error A complete log of this run can be found in: /.npm/_logs/2025-07-23T14_25_25_643Z-debug-0.log

📜 Recent review details

Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between dd72497 and e42d903.

📒 Files selected for processing (13)
  • CHANGELOG.md (1 hunks)
  • packages/core/__tests__/view/shape/Shape.test.ts (1 hunks)
  • packages/core/__tests__/view/shape/edge/PolyLineShape.test.ts (1 hunks)
  • packages/core/__tests__/view/shape/node/SwimlaneShape.test.ts (1 hunks)
  • packages/core/src/types.ts (1 hunks)
  • packages/core/src/util/Constants.ts (1 hunks)
  • packages/core/src/view/shape/Shape.ts (4 hunks)
  • packages/core/src/view/shape/edge/PolylineShape.ts (1 hunks)
  • packages/core/src/view/shape/node/HexagonShape.ts (1 hunks)
  • packages/core/src/view/shape/node/RectangleShape.ts (2 hunks)
  • packages/core/src/view/shape/node/RhombusShape.ts (1 hunks)
  • packages/core/src/view/shape/node/SwimlaneShape.ts (2 hunks)
  • packages/core/src/view/shape/node/TriangleShape.ts (1 hunks)
🧠 Learnings (12)
📓 Common learnings 
Learnt from: tbouffard
PR: maxGraph/maxGraph#776
File: packages/core/src/view/AbstractGraph.ts:492-495
Timestamp: 2025-04-24T12:34:56.726Z
Learning: PR #776 focuses on refactoring to introduce AbstractGraph as a base class and moving code from Graph to AbstractGraph. Type improvements and other refinements should be addressed in future PRs to keep this one focused on the core architectural change.

Learnt from: tbouffard
PR: maxGraph/maxGraph#857
File: packages/core/src/view/plugins/SelectionHandler.ts:1081-1092
Timestamp: 2025-06-26T13:27:19.012Z
Learning: User tbouffard prefers to keep PRs focused on their main objective and defer implementation improvements or logic fixes to future dedicated PRs. For maxGraph, refactoring PRs like Dictionary→Map migration should not include unrelated logic improvements even when valid issues are identified in the migrated code.

Learnt from: tbouffard
PR: maxGraph/maxGraph#715
File: packages/html/stories/ZoomAndFit.stories.ts:0-0
Timestamp: 2025-03-14T06:01:22.326Z
Learning: For future improvements to the Graph component in maxGraph, tbouffard is interested in refactoring the `fit` method to use an object parameter pattern instead of multiple positional parameters, as the current approach makes the API harder to use when many parameters need to be specified with default values.

Learnt from: tbouffard
PR: maxGraph/maxGraph#593
File: .github/workflows/create-github-release.yml:42-42
Timestamp: 2025-01-28T16:22:25.804Z
Learning: In the GitHub release notes template for maxGraph, TODO comments are intentionally included to mark sections that must be reviewed by maintainers before publishing the release. These TODOs should not be removed or converted to validation steps as they are part of the release review process.

Learnt from: tbouffard
PR: maxGraph/maxGraph#849
File: packages/html/stories/DragSource.stories.js:98-101
Timestamp: 2025-06-13T07:48:10.300Z
Learning: User tbouffard prefers answers in English; avoid switching to other languages in future replies.
packages/core/src/view/shape/node/RhombusShape.ts (2)

Learnt from: tbouffard
PR: #785
File: packages/core/src/view/style/register.ts:19-19
Timestamp: 2025-04-28T08:24:39.831Z
Learning: In the maxGraph project, the Perimeter namespace is defined in the 'builtin-style-elements.ts' file using the statement 'export * as Perimeter from './perimeter';'. To access the Perimeter namespace in other files, they must import from './builtin-style-elements' instead of directly from './perimeter', as part of the tree-shaking optimization.

Learnt from: tbouffard
PR: #785
File: packages/core/src/view/style/register.ts:19-19
Timestamp: 2025-04-28T08:24:39.831Z
Learning: In the maxGraph project, the Perimeter namespace is defined in the './builtin-style-elements' file as a namespace export ('export * as Perimeter'), making this the correct import location for accessing the Perimeter namespace.

packages/core/src/view/shape/node/HexagonShape.ts (2)

Learnt from: tbouffard
PR: #785
File: packages/core/src/view/style/register.ts:19-19
Timestamp: 2025-04-28T08:24:39.831Z
Learning: In the maxGraph project, the Perimeter namespace is defined in the 'builtin-style-elements.ts' file using the statement 'export * as Perimeter from './perimeter';'. To access the Perimeter namespace in other files, they must import from './builtin-style-elements' instead of directly from './perimeter', as part of the tree-shaking optimization.

Learnt from: tbouffard
PR: #785
File: packages/core/src/view/style/register.ts:19-19
Timestamp: 2025-04-28T08:24:39.831Z
Learning: In the maxGraph project, the Perimeter namespace is defined in the './builtin-style-elements' file as a namespace export ('export * as Perimeter'), making this the correct import location for accessing the Perimeter namespace.

packages/core/src/view/shape/node/RectangleShape.ts (2)

Learnt from: tbouffard
PR: #785
File: packages/core/src/view/style/register.ts:19-19
Timestamp: 2025-04-28T08:24:39.831Z
Learning: In the maxGraph project, the Perimeter namespace is defined in the 'builtin-style-elements.ts' file using the statement 'export * as Perimeter from './perimeter';'. To access the Perimeter namespace in other files, they must import from './builtin-style-elements' instead of directly from './perimeter', as part of the tree-shaking optimization.

Learnt from: tbouffard
PR: #785
File: packages/core/src/view/style/register.ts:19-19
Timestamp: 2025-04-28T08:24:39.831Z
Learning: In the maxGraph project, the Perimeter namespace is defined in the './builtin-style-elements' file as a namespace export ('export * as Perimeter'), making this the correct import location for accessing the Perimeter namespace.

packages/core/src/view/shape/edge/PolylineShape.ts (3)

Learnt from: tbouffard
PR: #785
File: packages/core/src/view/style/register.ts:19-19
Timestamp: 2025-04-28T08:24:39.831Z
Learning: In the maxGraph project, the Perimeter namespace is defined in the 'builtin-style-elements.ts' file using the statement 'export * as Perimeter from './perimeter';'. To access the Perimeter namespace in other files, they must import from './builtin-style-elements' instead of directly from './perimeter', as part of the tree-shaking optimization.

Learnt from: tbouffard
PR: #791
File: packages/website/docs/usage/edge-styles.md:59-63
Timestamp: 2025-04-29T13:30:18.686Z
Learning: In the EdgeStyles documentation, tbouffard prefers the existing text "A perimeter is a function matching the EdgeStyleFunction type" over suggestions that might create redundancy, even if terminology might be technically misaligned.

Learnt from: tbouffard
PR: #785
File: packages/core/src/view/style/register.ts:19-19
Timestamp: 2025-04-28T08:24:39.831Z
Learning: In the maxGraph project, the Perimeter namespace is defined in the './builtin-style-elements' file as a namespace export ('export * as Perimeter'), making this the correct import location for accessing the Perimeter namespace.

packages/core/src/view/shape/node/SwimlaneShape.ts (2)

Learnt from: tbouffard
PR: #785
File: packages/core/src/view/style/register.ts:19-19
Timestamp: 2025-04-28T08:24:39.831Z
Learning: In the maxGraph project, the Perimeter namespace is defined in the 'builtin-style-elements.ts' file using the statement 'export * as Perimeter from './perimeter';'. To access the Perimeter namespace in other files, they must import from './builtin-style-elements' instead of directly from './perimeter', as part of the tree-shaking optimization.

Learnt from: tbouffard
PR: #785
File: packages/core/src/view/style/register.ts:19-19
Timestamp: 2025-04-28T08:24:39.831Z
Learning: In the maxGraph project, the Perimeter namespace is defined in the './builtin-style-elements' file as a namespace export ('export * as Perimeter'), making this the correct import location for accessing the Perimeter namespace.

packages/core/__tests__/view/shape/edge/PolyLineShape.test.ts (1)

Learnt from: tbouffard
PR: #582
File: packages/core/tests/view/handler/config.test.ts:0-0
Timestamp: 2024-11-24T17:28:08.023Z
Learning: In packages/core/__tests__/view/handler/config.test.ts, when writing Jest tests in TypeScript for configuration reset functions, small duplication is acceptable if it maintains test clarity and readability. Avoid refactoring tests in ways that could make them harder to understand, especially when future use cases might require specific checks.

CHANGELOG.md (6)

Learnt from: tbouffard
PR: #593
File: .github/workflows/create-github-release.yml:42-42
Timestamp: 2025-01-28T16:22:25.804Z
Learning: In the GitHub release notes template for maxGraph, TODO comments are intentionally included to mark sections that must be reviewed by maintainers before publishing the release. These TODOs should not be removed or converted to validation steps as they are part of the release review process.

Learnt from: tbouffard
PR: #776
File: packages/core/src/view/AbstractGraph.ts:492-495
Timestamp: 2025-04-24T12:34:56.726Z
Learning: PR #776 focuses on refactoring to introduce AbstractGraph as a base class and moving code from Graph to AbstractGraph. Type improvements and other refinements should be addressed in future PRs to keep this one focused on the core architectural change.

Learnt from: tbouffard
PR: #715
File: packages/html/stories/ZoomAndFit.stories.ts:0-0
Timestamp: 2025-03-14T06:01:22.326Z
Learning: For future improvements to the Graph component in maxGraph, tbouffard is interested in refactoring the fit method to use an object parameter pattern instead of multiple positional parameters, as the current approach makes the API harder to use when many parameters need to be specified with default values.

Learnt from: tbouffard
PR: #791
File: packages/core/src/view/style/edge/Segment.ts:26-35
Timestamp: 2025-04-29T13:29:45.122Z
Learning: In the maxGraph project, JSDoc {@link} references to symbols don't need to be imported in the current file to work correctly. TypeDoc can resolve symbols across the entire project namespace, not just within the current file, which is common in modern documentation systems and IDEs.

Learnt from: tbouffard
PR: #770
File: packages/core/src/view/cell/CellTracker.ts:99-101
Timestamp: 2025-04-18T07:02:31.931Z
Learning: In TypeScript, when implementing an interface, methods can have fewer parameters than declared in the interface due to TypeScript's structural typing system. Both omitting unused parameters entirely and prefixing them with underscore (e.g., _sender) are valid patterns in the maxGraph codebase.

Learnt from: tbouffard
PR: #679
File: packages/html/stories/Drop.stories.js:71-94
Timestamp: 2025-02-17T06:01:50.745Z
Learning: maxGraph does not support Internet Explorer, making IE-specific browser compatibility checks unnecessary.

packages/core/src/view/shape/node/TriangleShape.ts (1)

Learnt from: tbouffard
PR: #785
File: packages/core/src/view/style/register.ts:19-19
Timestamp: 2025-04-28T08:24:39.831Z
Learning: In the maxGraph project, the Perimeter namespace is defined in the 'builtin-style-elements.ts' file using the statement 'export * as Perimeter from './perimeter';'. To access the Perimeter namespace in other files, they must import from './builtin-style-elements' instead of directly from './perimeter', as part of the tree-shaking optimization.

packages/core/__tests__/view/shape/node/SwimlaneShape.test.ts (1)

Learnt from: tbouffard
PR: #582
File: packages/core/tests/view/handler/config.test.ts:0-0
Timestamp: 2024-11-24T17:28:08.023Z
Learning: In packages/core/__tests__/view/handler/config.test.ts, when writing Jest tests in TypeScript for configuration reset functions, small duplication is acceptable if it maintains test clarity and readability. Avoid refactoring tests in ways that could make them harder to understand, especially when future use cases might require specific checks.

packages/core/src/types.ts (2)

Learnt from: tbouffard
PR: #791
File: packages/website/docs/usage/edge-styles.md:59-63
Timestamp: 2025-04-29T13:32:14.572Z
Learning: In the EdgeStyles documentation, the sentence "A perimeter is a function matching the EdgeStyleFunction type" contains an incorrect term - "perimeter" should be replaced with "EdgeStyle" or "custom EdgeStyle" as the document is specifically about EdgeStyles.

Learnt from: tbouffard
PR: #791
File: packages/website/docs/usage/edge-styles.md:59-63
Timestamp: 2025-04-29T13:30:18.686Z
Learning: In the EdgeStyles documentation, tbouffard prefers the existing text "A perimeter is a function matching the EdgeStyleFunction type" over suggestions that might create redundancy, even if terminology might be technically misaligned.

packages/core/__tests__/view/shape/Shape.test.ts (1)

Learnt from: tbouffard
PR: #582
File: packages/core/tests/view/handler/config.test.ts:0-0
Timestamp: 2024-11-24T17:28:08.023Z
Learning: In packages/core/__tests__/view/handler/config.test.ts, when writing Jest tests in TypeScript for configuration reset functions, small duplication is acceptable if it maintains test clarity and readability. Avoid refactoring tests in ways that could make them harder to understand, especially when future use cases might require specific checks.

🧬 Code Graph Analysis (1)
packages/core/src/view/shape/Shape.ts (1)
packages/core/src/util/Constants.ts (2)
  • LINE_ARCSIZE (337-337)
  • RECTANGLE_ROUNDING_FACTOR (331-331)
🧰 Additional context used
🧠 Learnings (12)
📓 Common learnings 
Learnt from: tbouffard
PR: maxGraph/maxGraph#776
File: packages/core/src/view/AbstractGraph.ts:492-495
Timestamp: 2025-04-24T12:34:56.726Z
Learning: PR #776 focuses on refactoring to introduce AbstractGraph as a base class and moving code from Graph to AbstractGraph. Type improvements and other refinements should be addressed in future PRs to keep this one focused on the core architectural change.

Learnt from: tbouffard
PR: maxGraph/maxGraph#857
File: packages/core/src/view/plugins/SelectionHandler.ts:1081-1092
Timestamp: 2025-06-26T13:27:19.012Z
Learning: User tbouffard prefers to keep PRs focused on their main objective and defer implementation improvements or logic fixes to future dedicated PRs. For maxGraph, refactoring PRs like Dictionary→Map migration should not include unrelated logic improvements even when valid issues are identified in the migrated code.

Learnt from: tbouffard
PR: maxGraph/maxGraph#715
File: packages/html/stories/ZoomAndFit.stories.ts:0-0
Timestamp: 2025-03-14T06:01:22.326Z
Learning: For future improvements to the Graph component in maxGraph, tbouffard is interested in refactoring the `fit` method to use an object parameter pattern instead of multiple positional parameters, as the current approach makes the API harder to use when many parameters need to be specified with default values.

Learnt from: tbouffard
PR: maxGraph/maxGraph#593
File: .github/workflows/create-github-release.yml:42-42
Timestamp: 2025-01-28T16:22:25.804Z
Learning: In the GitHub release notes template for maxGraph, TODO comments are intentionally included to mark sections that must be reviewed by maintainers before publishing the release. These TODOs should not be removed or converted to validation steps as they are part of the release review process.

Learnt from: tbouffard
PR: maxGraph/maxGraph#849
File: packages/html/stories/DragSource.stories.js:98-101
Timestamp: 2025-06-13T07:48:10.300Z
Learning: User tbouffard prefers answers in English; avoid switching to other languages in future replies.
packages/core/src/view/shape/node/RhombusShape.ts (2)

Learnt from: tbouffard
PR: #785
File: packages/core/src/view/style/register.ts:19-19
Timestamp: 2025-04-28T08:24:39.831Z
Learning: In the maxGraph project, the Perimeter namespace is defined in the 'builtin-style-elements.ts' file using the statement 'export * as Perimeter from './perimeter';'. To access the Perimeter namespace in other files, they must import from './builtin-style-elements' instead of directly from './perimeter', as part of the tree-shaking optimization.

Learnt from: tbouffard
PR: #785
File: packages/core/src/view/style/register.ts:19-19
Timestamp: 2025-04-28T08:24:39.831Z
Learning: In the maxGraph project, the Perimeter namespace is defined in the './builtin-style-elements' file as a namespace export ('export * as Perimeter'), making this the correct import location for accessing the Perimeter namespace.

packages/core/src/view/shape/node/HexagonShape.ts (2)

Learnt from: tbouffard
PR: #785
File: packages/core/src/view/style/register.ts:19-19
Timestamp: 2025-04-28T08:24:39.831Z
Learning: In the maxGraph project, the Perimeter namespace is defined in the 'builtin-style-elements.ts' file using the statement 'export * as Perimeter from './perimeter';'. To access the Perimeter namespace in other files, they must import from './builtin-style-elements' instead of directly from './perimeter', as part of the tree-shaking optimization.

Learnt from: tbouffard
PR: #785
File: packages/core/src/view/style/register.ts:19-19
Timestamp: 2025-04-28T08:24:39.831Z
Learning: In the maxGraph project, the Perimeter namespace is defined in the './builtin-style-elements' file as a namespace export ('export * as Perimeter'), making this the correct import location for accessing the Perimeter namespace.

packages/core/src/view/shape/node/RectangleShape.ts (2)

Learnt from: tbouffard
PR: #785
File: packages/core/src/view/style/register.ts:19-19
Timestamp: 2025-04-28T08:24:39.831Z
Learning: In the maxGraph project, the Perimeter namespace is defined in the 'builtin-style-elements.ts' file using the statement 'export * as Perimeter from './perimeter';'. To access the Perimeter namespace in other files, they must import from './builtin-style-elements' instead of directly from './perimeter', as part of the tree-shaking optimization.

Learnt from: tbouffard
PR: #785
File: packages/core/src/view/style/register.ts:19-19
Timestamp: 2025-04-28T08:24:39.831Z
Learning: In the maxGraph project, the Perimeter namespace is defined in the './builtin-style-elements' file as a namespace export ('export * as Perimeter'), making this the correct import location for accessing the Perimeter namespace.

packages/core/src/view/shape/edge/PolylineShape.ts (3)

Learnt from: tbouffard
PR: #785
File: packages/core/src/view/style/register.ts:19-19
Timestamp: 2025-04-28T08:24:39.831Z
Learning: In the maxGraph project, the Perimeter namespace is defined in the 'builtin-style-elements.ts' file using the statement 'export * as Perimeter from './perimeter';'. To access the Perimeter namespace in other files, they must import from './builtin-style-elements' instead of directly from './perimeter', as part of the tree-shaking optimization.

Learnt from: tbouffard
PR: #791
File: packages/website/docs/usage/edge-styles.md:59-63
Timestamp: 2025-04-29T13:30:18.686Z
Learning: In the EdgeStyles documentation, tbouffard prefers the existing text "A perimeter is a function matching the EdgeStyleFunction type" over suggestions that might create redundancy, even if terminology might be technically misaligned.

Learnt from: tbouffard
PR: #785
File: packages/core/src/view/style/register.ts:19-19
Timestamp: 2025-04-28T08:24:39.831Z
Learning: In the maxGraph project, the Perimeter namespace is defined in the './builtin-style-elements' file as a namespace export ('export * as Perimeter'), making this the correct import location for accessing the Perimeter namespace.

packages/core/src/view/shape/node/SwimlaneShape.ts (2)

Learnt from: tbouffard
PR: #785
File: packages/core/src/view/style/register.ts:19-19
Timestamp: 2025-04-28T08:24:39.831Z
Learning: In the maxGraph project, the Perimeter namespace is defined in the 'builtin-style-elements.ts' file using the statement 'export * as Perimeter from './perimeter';'. To access the Perimeter namespace in other files, they must import from './builtin-style-elements' instead of directly from './perimeter', as part of the tree-shaking optimization.

Learnt from: tbouffard
PR: #785
File: packages/core/src/view/style/register.ts:19-19
Timestamp: 2025-04-28T08:24:39.831Z
Learning: In the maxGraph project, the Perimeter namespace is defined in the './builtin-style-elements' file as a namespace export ('export * as Perimeter'), making this the correct import location for accessing the Perimeter namespace.

packages/core/__tests__/view/shape/edge/PolyLineShape.test.ts (1)

Learnt from: tbouffard
PR: #582
File: packages/core/tests/view/handler/config.test.ts:0-0
Timestamp: 2024-11-24T17:28:08.023Z
Learning: In packages/core/__tests__/view/handler/config.test.ts, when writing Jest tests in TypeScript for configuration reset functions, small duplication is acceptable if it maintains test clarity and readability. Avoid refactoring tests in ways that could make them harder to understand, especially when future use cases might require specific checks.

CHANGELOG.md (6)

Learnt from: tbouffard
PR: #593
File: .github/workflows/create-github-release.yml:42-42
Timestamp: 2025-01-28T16:22:25.804Z
Learning: In the GitHub release notes template for maxGraph, TODO comments are intentionally included to mark sections that must be reviewed by maintainers before publishing the release. These TODOs should not be removed or converted to validation steps as they are part of the release review process.

Learnt from: tbouffard
PR: #776
File: packages/core/src/view/AbstractGraph.ts:492-495
Timestamp: 2025-04-24T12:34:56.726Z
Learning: PR #776 focuses on refactoring to introduce AbstractGraph as a base class and moving code from Graph to AbstractGraph. Type improvements and other refinements should be addressed in future PRs to keep this one focused on the core architectural change.

Learnt from: tbouffard
PR: #715
File: packages/html/stories/ZoomAndFit.stories.ts:0-0
Timestamp: 2025-03-14T06:01:22.326Z
Learning: For future improvements to the Graph component in maxGraph, tbouffard is interested in refactoring the fit method to use an object parameter pattern instead of multiple positional parameters, as the current approach makes the API harder to use when many parameters need to be specified with default values.

Learnt from: tbouffard
PR: #791
File: packages/core/src/view/style/edge/Segment.ts:26-35
Timestamp: 2025-04-29T13:29:45.122Z
Learning: In the maxGraph project, JSDoc {@link} references to symbols don't need to be imported in the current file to work correctly. TypeDoc can resolve symbols across the entire project namespace, not just within the current file, which is common in modern documentation systems and IDEs.

Learnt from: tbouffard
PR: #770
File: packages/core/src/view/cell/CellTracker.ts:99-101
Timestamp: 2025-04-18T07:02:31.931Z
Learning: In TypeScript, when implementing an interface, methods can have fewer parameters than declared in the interface due to TypeScript's structural typing system. Both omitting unused parameters entirely and prefixing them with underscore (e.g., _sender) are valid patterns in the maxGraph codebase.

Learnt from: tbouffard
PR: #679
File: packages/html/stories/Drop.stories.js:71-94
Timestamp: 2025-02-17T06:01:50.745Z
Learning: maxGraph does not support Internet Explorer, making IE-specific browser compatibility checks unnecessary.

packages/core/src/view/shape/node/TriangleShape.ts (1)

Learnt from: tbouffard
PR: #785
File: packages/core/src/view/style/register.ts:19-19
Timestamp: 2025-04-28T08:24:39.831Z
Learning: In the maxGraph project, the Perimeter namespace is defined in the 'builtin-style-elements.ts' file using the statement 'export * as Perimeter from './perimeter';'. To access the Perimeter namespace in other files, they must import from './builtin-style-elements' instead of directly from './perimeter', as part of the tree-shaking optimization.

packages/core/__tests__/view/shape/node/SwimlaneShape.test.ts (1)

Learnt from: tbouffard
PR: #582
File: packages/core/tests/view/handler/config.test.ts:0-0
Timestamp: 2024-11-24T17:28:08.023Z
Learning: In packages/core/__tests__/view/handler/config.test.ts, when writing Jest tests in TypeScript for configuration reset functions, small duplication is acceptable if it maintains test clarity and readability. Avoid refactoring tests in ways that could make them harder to understand, especially when future use cases might require specific checks.

packages/core/src/types.ts (2)

Learnt from: tbouffard
PR: #791
File: packages/website/docs/usage/edge-styles.md:59-63
Timestamp: 2025-04-29T13:32:14.572Z
Learning: In the EdgeStyles documentation, the sentence "A perimeter is a function matching the EdgeStyleFunction type" contains an incorrect term - "perimeter" should be replaced with "EdgeStyle" or "custom EdgeStyle" as the document is specifically about EdgeStyles.

Learnt from: tbouffard
PR: #791
File: packages/website/docs/usage/edge-styles.md:59-63
Timestamp: 2025-04-29T13:30:18.686Z
Learning: In the EdgeStyles documentation, tbouffard prefers the existing text "A perimeter is a function matching the EdgeStyleFunction type" over suggestions that might create redundancy, even if terminology might be technically misaligned.

packages/core/__tests__/view/shape/Shape.test.ts (1)

Learnt from: tbouffard
PR: #582
File: packages/core/tests/view/handler/config.test.ts:0-0
Timestamp: 2024-11-24T17:28:08.023Z
Learning: In packages/core/__tests__/view/handler/config.test.ts, when writing Jest tests in TypeScript for configuration reset functions, small duplication is acceptable if it maintains test clarity and readability. Avoid refactoring tests in ways that could make them harder to understand, especially when future use cases might require specific checks.

🧬 Code Graph Analysis (1)
packages/core/src/view/shape/Shape.ts (1)
packages/core/src/util/Constants.ts (2)
  • LINE_ARCSIZE (337-337)
  • RECTANGLE_ROUNDING_FACTOR (331-331)
🔇 Additional comments (24)
packages/core/src/util/Constants.ts (2)

327-331: Documentation improvement looks good.

The reformatted comment for RECTANGLE_ROUNDING_FACTOR is more concise while maintaining all the essential information about the valid range and typical usage.

334-337: Good clarification for broader usage.

Adding "vertices" to the LINE_ARCSIZE documentation correctly reflects that this constant is used for both rounded edges and vertices, which aligns with the centralized arcSize approach introduced in this PR.

packages/core/src/view/shape/node/RhombusShape.ts (1)

60-60: Good refactoring to centralized arc size calculation.

The change from manual calculation (this.style?.arcSize ?? LINE_ARCSIZE) / 2 to using this.getBaseArcSize() centralizes the arc size logic and removes direct dependency on the LINE_ARCSIZE constant. This aligns with the PR's goal of consistent arcSize computation across shapes.

packages/core/src/view/shape/node/RectangleShape.ts (2)

19-19: Good cleanup of unused imports.

Removing the unused LINE_ARCSIZE and RECTANGLE_ROUNDING_FACTOR imports aligns with the refactoring to centralized arc size calculation methods.

75-75: Excellent centralization of arc size logic.

Replacing the manual calculation with this.getArcSize(w, h) centralizes the arc size determination logic and properly handles both absolute and percentage-based arc sizes by passing the shape dimensions. This removes the inline conditional logic and ensures consistent behavior across the codebase.

packages/core/src/view/shape/node/HexagonShape.ts (1)

39-39: Consistent refactoring to centralized arc size calculation.

The change from (this.style?.arcSize ?? LINE_ARCSIZE) / 2 to this.getBaseArcSize() follows the same pattern applied across other shape classes, centralizing arc size logic and removing direct dependency on constants. This ensures consistent behavior and easier maintenance.

packages/core/src/view/shape/edge/PolylineShape.ts (1)

89-89: Excellent extension of centralized arc size logic to edges.

The change to directly use this.getBaseArcSize() instead of a local variable eliminates the direct dependency on LINE_ARCSIZE and applies the centralized arc size calculation to edge shapes. This is particularly important for this PR's goal of ensuring consistent arcSize computation between edges and vertices.

packages/core/src/view/shape/node/TriangleShape.ts (1)

47-47: LGTM! Arc size calculation properly standardized.

The change from manual calculation with division by 2 to using this.getBaseArcSize() correctly implements the consistent arc size computation mentioned in the PR objectives. This aligns with the broader refactoring to standardize arc size handling across all shape classes.

packages/core/src/view/shape/node/SwimlaneShape.ts (2)

53-53: Documentation improvement looks good.

The JSDoc comment clarification makes the property description clearer and more precise.

136-141: Arc size calculation properly standardized.

The refactoring of getSwimlaneArcSize correctly implements the consistent arc size computation:

  • Uses this.getBaseArcSize() for the absoluteArcSize case, maintaining consistency with other shape classes
  • Preserves the existing fallback calculation logic while improving code readability with better variable naming
  • Aligns with the PR's objective to standardize arc size computation across the codebase
packages/core/__tests__/view/shape/edge/PolyLineShape.test.ts (1)

25-38: Well-structured test validates arc size refactoring.

This focused test effectively verifies that paintLine uses the correct arc size through the getBaseArcSize() method. The test setup is appropriate with minimal mocking, and the expected value of 10 correctly represents the default arc size when no style is set. This provides good coverage for the arc size standardization changes in PolylineShape.

CHANGELOG.md (1)

17-20: Breaking change documentation is clear and helpful.

The changelog entry accurately documents the arc size computation standardization and provides clear guidance for users who need to maintain previous rendering behavior. The explanation that arcSize "was not always correctly computed in the past" and the specific instruction to "multiply the arcSize by 2" for edges gives users actionable steps to handle this breaking change.

packages/core/__tests__/view/shape/Shape.test.ts (1)

20-120: Comprehensive test suite provides excellent coverage.

This well-structured test suite thoroughly validates the getArcSize method behavior across all relevant scenarios:

  • Complete coverage: Tests all combinations of absoluteArcSize values (true, false, undefined, and null style)
  • Edge case handling: Includes zero dimensions, zero arcSize, and various width/height relationships
  • Good test organization: Uses describe blocks and parameterized tests appropriately
  • Correct expected values: The assertions align with the standardized arc size computation logic

The test suite provides strong validation for the arc size refactoring and ensures the new consistent behavior works correctly across different configurations.

packages/core/__tests__/view/shape/node/SwimlaneShape.test.ts (4)

17-26: LGTM! Well-structured test setup.

The test imports, describe block organization, and beforeEach setup follow Jest best practices. Using null! for unused constructor parameters in tests is appropriate.

28-65: Excellent test coverage for absoluteArcSize: true scenarios.

The tests comprehensively cover the arc size calculation when absoluteArcSize is true, including:

  • Different dimension ratios (height vs width)
  • Edge cases (zero arcSize, zero dimensions, unset arcSize)
  • Proper verification that the arc size is divided by 2 and clamped to the smaller dimension

This aligns perfectly with the PR objective of consistent arc size computation.

67-70: Good use of test.each for percentage-based calculations.

The test efficiently verifies that both false and undefined values for absoluteArcSize produce the same percentage-based arc size calculation. The parameterized approach reduces code duplication while ensuring comprehensive coverage.

72-92: Comprehensive null style fallback testing.

The tests verify that getSwimlaneArcSize handles null styles gracefully with appropriate fallback calculations. Testing various dimension combinations and start parameter values ensures robust behavior when style properties are unavailable.

packages/core/src/types.ts (1)

104-119: Excellent documentation improvements for arcSize property.

The updated documentation significantly improves clarity by:

  • Clearly distinguishing between vertex and edge behavior
  • Specifying which shapes use absolute vs percentage-based calculations
  • Including hexagon in the supported shapes list
  • Explaining the role of the absoluteArcSize flag
  • Providing clear fallback value information

This documentation aligns perfectly with the PR's goal of standardizing arc size computation and provides developers with clear guidance on how the property behaves across different contexts.

packages/core/src/view/shape/Shape.ts (6)

279-279: Good documentation clarification for init method.

The updated documentation clarifies that the shape is only added to the container if the node doesn't already have a parent, which helps developers understand the conditional behavior of this method.

303-303: LGTM! Documentation simplification.

The updated documentation is more concise while maintaining clarity about the method's behavior.

310-310: Good JSDoc linking improvement.

Using the JSDoc link style {@link strokeWidth} improves documentation consistency and provides better cross-referencing capabilities.

760-765: Excellent centralization of arc size logic - addresses core PR objective.

The new getBaseArcSize() method effectively centralizes arc size retrieval and ensures the critical division by 2 is consistently applied across all shapes. This directly addresses the main issue described in the PR where the division was missing in edge calculations.

Key benefits:

  • Centralizes arc size logic in the base Shape class
  • Ensures consistent division by 2 across all shape types
  • Provides fallback to LINE_ARCSIZE for backward compatibility
  • Protected visibility allows subclass access while maintaining encapsulation

767-776: Well-implemented refactoring of getArcSize method.

The refactored implementation provides excellent improvements:

  1. Centralized logic: Uses the new getBaseArcSize() helper for consistent behavior
  2. Proper flag handling: Correctly differentiates between absolute pixel sizes (absoluteArcSize: true) and percentage-based calculations
  3. Appropriate clamping: Ensures absolute arc sizes don't exceed half the smaller dimension
  4. Good fallbacks: Uses RECTANGLE_ROUNDING_FACTOR * 100 as default percentage when arcSize is unspecified

This implementation directly supports the PR's goal of consistent arc size computation across vertex and edge shapes.

1016-1016: LGTM! Documentation simplification.

The updated documentation is more concise while maintaining clarity about the method's default behavior.

✨ Finishing Touches
  • 📝 Generate Docstrings

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
🪧 Tips

Chat

There are 3 ways to chat with CodeRabbit:

  • Review comments: Directly reply to a review comment made by CodeRabbit. Example:
    • I pushed a fix in commit <commit_id>, please review it.
    • Explain this complex logic.
    • Open a follow-up GitHub issue for this discussion.
  • Files and specific lines of code (under the "Files changed" tab): Tag @coderabbitai in a new review comment at the desired location with your query. Examples:
    • @coderabbitai explain this code block.
    • @coderabbitai modularize this function.
  • PR comments: Tag @coderabbitai in a new PR comment to ask questions about the PR branch. For the best results, please provide a very specific query, as very limited context is provided in this mode. Examples:
    • @coderabbitai gather interesting stats about this repository and render them as a table. Additionally, render a pie chart showing the language distribution in the codebase.
    • @coderabbitai read src/utils.ts and explain its main purpose.
    • @coderabbitai read the files in the src/scheduler package and generate a class diagram using mermaid and a README in the markdown format.
    • @coderabbitai help me debug CodeRabbit configuration file.

Support

Need help? Create a ticket on our support page for assistance with any issues or questions.

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)

  • @coderabbitai pause to pause the reviews on a PR.
  • @coderabbitai resume to resume the paused reviews.
  • @coderabbitai review to trigger an incremental review. This is useful when automatic reviews are disabled for the repository.
  • @coderabbitai full review to do a full review from scratch and review all the files again.
  • @coderabbitai summary to regenerate the summary of the PR.
  • @coderabbitai generate docstrings to generate docstrings for this PR.
  • @coderabbitai generate sequence diagram to generate a sequence diagram of the changes in this PR.
  • @coderabbitai resolve resolve all the CodeRabbit review comments.
  • @coderabbitai configuration to show the current CodeRabbit configuration for the repository.
  • @coderabbitai help to get help.

Other keywords and placeholders

  • Add @coderabbitai ignore anywhere in the PR description to prevent this PR from being reviewed.
  • Add @coderabbitai summary to generate the high-level summary at a specific location in the PR description.
  • Add @coderabbitai anywhere in the PR title to generate the title automatically.

CodeRabbit Configuration File (.coderabbit.yaml)

  • You can programmatically configure CodeRabbit by adding a .coderabbit.yaml file to the root of your repository.
  • Please see the configuration documentation for more information.
  • If your editor has YAML language server enabled, you can add the path at the top of this file to enable auto-completion and validation: # yaml-language-server: $schema=https://coderabbit.ai/integrations/schema.v2.json

Documentation and Community

  • Visit our Documentation for detailed information on how to use CodeRabbit.
  • Join our Discord Community to get help, request features, and share feedback.
  • Follow us on X/Twitter for updates and announcements.

@sonarqubecloud
Copy link

sonarqubecloud bot commented Jul 23, 2025

Quality Gate Passed Quality Gate passed

Issues
0 New issues
0 Accepted issues

Measures
0 Security Hotspots
0.0% Coverage on New Code
0.0% Duplication on New Code

See analysis details on SonarQube Cloud

@tbouffard tbouffard marked this pull request as ready for review July 23, 2025 14:12
@tbouffard tbouffard merged commit e3d3dfc into main Jul 23, 2025
7 checks passed
@tbouffard tbouffard deleted the refactor/improve_mgt_CellStyle_arcSize branch July 23, 2025 16:00
@coderabbitai coderabbitai bot mentioned this pull request Nov 5, 2025
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

No reviews

Assignees

No one assigned

Labels

bug Something isn't working

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

1 participant

@tbouffard