fix: make `Graph.options` truly per-instance #751

tbouffard · 2025-04-05T08:26:55Z

Before, setting Graph.options.foldingEnabled = false on one Graph instance affected other instances too - because the options object was shared between them.
For example, switching to the Constituent story (which disables Graph.options.foldingEnabled) then back to another story relying on the default (true) would break folding: the folding icon disappeared and cells couldn't be collapsed anymore.
This could be seen with the Collapse and the Folding stories.

The bug exists at least as of version 0.14.0, but probably from the first version of maxGraph.

🔍 Root cause: options was defined via mixins as an object, but the internal mixInto function probably copies
object references, not values — effectively making Graph.options global.

✅ Fix: move the options property directly onto the Graph class.
It's the only object-typed prop in our mixins, and we're planning to turn FoldingMixin into a plugin anyway — so this fix is simple and future-proof.

ℹ️ No impact for users, just internal code structure changes.

In addition, migrate Constituent.stories.js to TypeScript to ease future maintenance.

Summary by CodeRabbit

New Features
- Introduced enhanced graph folding with improved collapse/expand visuals for a more intuitive diagram experience.
Refactor
- Streamlined the configuration of graph folding options for consistent behavior.
- Improved handling of graph interactions to ensure smoother selection and stability during user events.
Documentation
- Updated configuration guidelines for internationalization to provide clearer information on variable behavior when the feature is disabled.

Before, setting `Graph.options.foldingEnabled = false` on one `Graph` instance affected other instances too — because the `options` object was shared between them. For example, switching to the Constituent story (which disables `Graph.options.foldingEnabled`) then back to another story relying on the default (`true`) would break folding: the folding icon disappeared and cells couldn't be collapsed anymore. This could be seen with the `Collapse` and the `Folding` stories. The bug exists at least as of version 0.14.0, but probably from the first version of maxGraph. 🔍 Root cause: `options` was defined via mixins as an object, but the internal `applyMixin` function probably copies object references, not values — effectively making `Graph.options` global. ✅ Fix: move the `options` property directly onto the `Graph` class. It's the only object-typed prop in our mixins, and we're planning to turn `FoldingMixin` into a plugin anyway — so this fix is simple and future-proof. ℹ️ No impact for users, just internal code structure changes. In addition, migrate Constituent.stories.js to TypeScript to ease future maintenance.

coderabbitai · 2025-04-05T08:27:03Z

Walkthrough

The changes update documentation for i18n configuration, introduce and integrate a new GraphFoldingOptions type for configuring graph folding behavior, and refactor the folding mixin definitions by removing redundant options and type dependencies. In addition, the pull request enhances the type safety and event-handling methods in a constituent story. No alterations were made to exported or public entities aside from these modifications.

Changes

File(s)	Change Summary
`packages/core/src/i18n/config.ts`	Updated documentation comments for `TranslationsConfig`: clarified the list of variables cleared when internationalization is disabled by removing `{@link CellRenderer.collapseExpandResource}` and adding `{@link Graph.collapseExpandResource}`.
`packages/core/src/types.ts`, `packages/core/src/view/Graph.ts`, `packages/core/src/view/mixins/FoldingMixin.ts`, `packages/core/src/view/mixins/FoldingMixin.type.ts`	Introduced new type `GraphFoldingOptions` in `types.ts` with properties for folding behavior; added an `options` property to the `Graph` class with default folding settings; removed the `options` property from `FoldingMixin` and updated the corresponding type definitions by deleting `GraphFoldingOptions` from the mixin types and changing the return type of `getFoldingImage` from `Image
`packages/html/stories/Constituent.stories.ts`	Enhanced type safety and event handling: added type annotations to methods (`getInitialCellForEvent`, `isPart`, `selectCellForEvent`), introduced a new `CustomCellStateStyle` type and a utility function `isNullish`, and updated graph creation and cell insertion logic.

Sequence Diagram(s)

sequenceDiagram
    participant User
    participant Graph
    participant FoldingManager
    User->>Graph: Trigger fold/expand action
    Graph->>FoldingManager: Retrieve folding options
    FoldingManager-->>Graph: Return configured ImageBox (collapsed/expanded)
    Graph-->>User: Update visual with folding image

sequenceDiagram
    participant User
    participant MyCustomGraph
    participant Template
    User->>MyCustomGraph: Initiate mouse event
    MyCustomGraph->>MyCustomGraph: getInitialCellForEvent(event: InternalMouseEvent)
    MyCustomGraph->>MyCustomGraph: isPart(cell: Cell | null)
    MyCustomGraph->>MyCustomGraph: selectCellForEvent(cell: Cell, event: MouseEvent)
    MyCustomGraph-->>User: Update selection based on event

Suggested labels

refactor

📜 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 6a003ee and 2b7cdb3.

📒 Files selected for processing (6)

packages/core/src/i18n/config.ts (2 hunks)
packages/core/src/types.ts (1 hunks)
packages/core/src/view/Graph.ts (2 hunks)
packages/core/src/view/mixins/FoldingMixin.ts (1 hunks)
packages/core/src/view/mixins/FoldingMixin.type.ts (2 hunks)
packages/html/stories/Constituent.stories.ts (4 hunks)

🧰 Additional context used

🧬 Code Definitions (2)

packages/core/src/view/Graph.ts (1)

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

GraphFoldingOptions (1349-1370)

packages/html/stories/Constituent.stories.ts (1)

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

CellStateStyle (72-883)

⏰ Context from checks skipped due to timeout of 90000ms (1)

GitHub Check: build (windows-2022)

🔇 Additional comments (24)

packages/core/src/i18n/config.ts (2)

82-83: Documentation update provides better clarity.

The clarification that the list may not be exhaustive helps prevent assumptions about complete coverage of all variables affected by i18n disabling.

92-92: Updated resource reference aligns with architectural changes.

The change from CellRenderer.collapseExpandResource to Graph.collapseExpandResource correctly reflects the refactoring where folding options are now owned by the Graph class rather than mixins.

packages/core/src/view/Graph.ts (2)

50-50: Import now includes GraphFoldingOptions type.

This import supports the new per-instance options property that's being added to the Graph class.

399-405: Added per-instance folding options to Graph class.

This change directly addresses the bug in the PR objective by making Graph.options truly per-instance instead of shared via mixins. The initialization with default values aligns with the type definition in types.ts.

This is a clean solution that maintains the existing API while fixing the instance isolation issue. As mentioned in the PR objectives, this will also facilitate future plans to convert FoldingMixin into a plugin.

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

1349-1370: Added GraphFoldingOptions type with well-documented properties.

This new type definition provides a clean structure for the folding options with appropriate JSDoc comments and default values. Moving this type to the central types.ts file helps with organization and makes it available throughout the codebase.

packages/core/src/view/mixins/FoldingMixin.type.ts (2)

19-19: Updated import from Image to ImageBox.

This change ensures type consistency with the updated return type of getFoldingImage method.

56-56: Updated getFoldingImage return type from Image to ImageBox.

This type change aligns with the GraphFoldingOptions type definition that uses ImageBox for the folding images. This ensures type consistency throughout the codebase.

packages/core/src/view/mixins/FoldingMixin.ts (5)

26-39: The options property is now correctly included in the PartialGraph type.

This change aligns with the PR objective to fix the issue where Graph.options affected all instances. By including options in the PartialGraph type, the mixin can properly access per-instance options from the parent Graph class instead of using a shared reference.

40-52: Good removal of the options property from PartialFolding type.

The PR correctly removes the options property from the PartialFolding type, as options are now managed through the Graph class directly. This helps ensure that options are truly per-instance and not shared across Graph instances.

63-65: Accessing options through the parent Graph instance.

The code now accesses this.options.foldingEnabled directly through the parent Graph instance, rather than from a property defined in the mixin itself. This correctly implements the per-instance approach.

77-86: Properly using the instance-specific options.

The getFoldingImage method now correctly uses this.options.collapsedImage and this.options.expandedImage, which are properties accessed from the Graph class. This ensures that each graph instance has its own independent configuration.

168-179: Correctly using the per-instance collapseToPreferredSize option.

The code now properly accesses this.options.collapseToPreferredSize from the parent Graph instance, ensuring that this setting is specific to each graph instance rather than being shared across all instances.

packages/html/stories/Constituent.stories.ts (12)

18-27: Good improvement of type imports and better type safety.

The changes properly import type annotations for Cell and CellStateStyle, improving type safety. The explicit imports of InternalMouseEvent and SelectionHandler also enhance the code's type checking capabilities.

41-42: Good addition of a type-safe null check utility.

The isNullish helper function is a good addition that improves type safety when checking for null or undefined values. This utility function helps TypeScript understand the type narrowing that occurs during null checks.

43-50: Improved structure and type safety in the Template function.

The Template function now has a proper type annotation (Record<string, string>), and properly creates and returns a main div. This structural change improves the consistency of the story design.

58-64: Enhanced type safety in getInitialCellForEvent method.

The override properly specifies the InternalMouseEvent type for the event parameter and improves null handling when getting the parent cell, using the null coalescing operator (??) for safer access.

67-69: Good addition of a custom type for CellStateStyle.

Creating a dedicated CustomCellStateStyle type that extends CellStateStyle with the constituent boolean property improves type safety and makes the code more self-documenting.

72-76: Better typed constructor and plugin initialization.

The constructor now properly specifies the HTMLElement type for the container parameter and initializes with default plugins in a cleaner way.

74-74: Correctly using the per-instance options.

This line sets this.options.foldingEnabled = false on the specific graph instance, demonstrating the fix implemented by this PR, where options are now truly per-instance rather than shared across all Graph instances.

79-84: Improved type safety in the isPart method.

The isPart method now properly handles null checks using the isNullish utility function and includes correct type casting to CustomCellStateStyle. This makes the code more robust and type-safe.

87-93: Enhanced selectCellForEvent method with better null handling.

The method now uses arrow function syntax for consistent this binding and properly handles potential null values when redirecting selection to the parent cell.

106-107: Simplified parent acquisition.

This change simplifies getting the default parent by directly calling graph.getDefaultParent() instead of a potentially more complex approach, making the code cleaner and more maintainable.

116-117: Better type safety with boolean constituent property.

Using a boolean value for the constituent property instead of a numeric value makes the code more type-safe and readable. The explicit type cast to CustomCellStateStyle ensures type checking works correctly.

121-121: More appropriate return value.

Returning the div instead of the container ensures that the full UI component, including both the description and the graph container, is returned from the Template function.

✨ 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.
- Generate unit testing code for this file.
- 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 generate unit testing code for this file.
- @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 generate unit testing code.
- @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.

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 resolve resolve all the CodeRabbit review comments.
@coderabbitai plan to trigger planning for file edits and PR creation.
@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 · 2025-04-06T05:52:40Z

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 added the bug Something isn't working label Apr 5, 2025

tbouffard added 2 commits April 5, 2025 10:30

refactor: improve type

ef549a0

refactor: remove unused imports

2b7cdb3

tbouffard marked this pull request as ready for review April 5, 2025 08:32

tbouffard changed the title ~~fix(graph): make Graph.options truly per-instance~~ fix: make Graph.options truly per-instance Apr 5, 2025

Merge branch 'main' into fix/side_effect_when_changing_Graph_options

91bea00

tbouffard merged commit 8a01d75 into main Apr 6, 2025
7 checks passed

tbouffard deleted the fix/side_effect_when_changing_Graph_options branch April 6, 2025 06:14

tbouffard mentioned this pull request Jul 23, 2025

refactor: ensure mixins do not create shared properties in AbstractGraph #879

Merged

7 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

fix: make `Graph.options` truly per-instance #751

fix: make `Graph.options` truly per-instance #751

Uh oh!

tbouffard commented Apr 5, 2025 •

edited

Loading

Uh oh!

coderabbitai bot commented Apr 5, 2025 •

edited

Loading

Chat

CodeRabbit Commands (Invoked using PR comments)

Other keywords and placeholders

CodeRabbit Configuration File (`.coderabbit.yaml`)

Documentation and Community

Uh oh!

sonarqubecloud bot commented Apr 6, 2025

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

1 participant

fix: make Graph.options truly per-instance #751

fix: make Graph.options truly per-instance #751

Uh oh!

Conversation

tbouffard commented Apr 5, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Summary by CodeRabbit

Uh oh!

coderabbitai bot commented Apr 5, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Walkthrough

Changes

Sequence Diagram(s)

Suggested labels

Chat

CodeRabbit Commands (Invoked using PR comments)

Other keywords and placeholders

CodeRabbit Configuration File (.coderabbit.yaml)

Documentation and Community

Uh oh!

sonarqubecloud bot commented Apr 6, 2025

Quality Gate passed

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

1 participant

fix: make `Graph.options` truly per-instance #751

fix: make `Graph.options` truly per-instance #751

tbouffard commented Apr 5, 2025 •

edited

Loading

coderabbitai bot commented Apr 5, 2025 •

edited

Loading

CodeRabbit Configuration File (`.coderabbit.yaml`)