refactor: ensure mixins do not create shared properties in AbstractGraph #879
Conversation
WalkthroughPublic API changes move folding options and alternateEdgeStyle onto AbstractGraph, converting some mixin-provided properties to class-level. Tests are updated to validate per-instance state and expanded serialization (including alternateEdgeStyle, multiplicities, and options). Documentation comments are revised; type-only imports introduced; no functional logic changes beyond property relocation. Changes
Sequence Diagram(s)Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Possibly related PRs
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✨ Finishing touches
🧪 Generate unit tests
Tip 👮 Agentic pre-merge checks are now available in preview!Pro plan users can now enable pre-merge checks in their settings to enforce checklists before merging PRs.
Please see the documentation for more information. Example: reviews:
pre_merge_checks:
custom_checks:
- name: "Undocumented Breaking Changes"
mode: "warning"
instructions: |
Pass/fail criteria: All breaking changes to public APIs, CLI flags, environment variables, configuration keys, database schemas, or HTTP/GraphQL endpoints must be documented in the "Breaking Change" section of the PR description and in CHANGELOG.md. Exclude purely internal or private changes (e.g., code not exported from package entry points or explicitly marked as internal).Please share your feedback with us on this Discord post. 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. Comment |
There was a problem hiding this comment.
Actionable comments posted: 1
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
packages/core/src/view/AbstractGraph.ts(0 hunks)packages/core/src/view/mixins/ValidationMixin.ts(1 hunks)packages/core/src/view/mixins/ValidationMixin.type.ts(1 hunks)
💤 Files with no reviewable changes (1)
- packages/core/src/view/AbstractGraph.ts
🧰 Additional context used
🧠 Learnings (2)
📓 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#740
File: packages/core/src/util/requestUtils.ts:79-106
Timestamp: 2025-03-31T08:32:17.055Z
Learning: PR #740 is focused on reorganizing code into namespaces (guiUtils and requestUtils) without changing any implementations. Implementation improvements should be suggested for future PRs.
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/mixins/ValidationMixin.type.ts (4)
Learnt from: tbouffard
PR: maxGraph/maxGraph#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: maxGraph/maxGraph#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: 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#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.
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (4)
- GitHub Check: build (macos-14)
- GitHub Check: build (ubuntu-24.04)
- GitHub Check: build (windows-2022)
- GitHub Check: build
🔇 Additional comments (1)
packages/core/src/view/mixins/ValidationMixin.type.ts (1)
23-26: Good addition of contextual JSDoc
Adds missing description and cross–links; nothing to change.
|
The current implementation of the function applying mixins creates shared state for "object" or "array" properties. For such properties, ensure they are all defined in AbstractGraph and add tests to validate that there is no global states. Here, this includes a fix for 'alternateEdgeStyle' which is no longer a global state. In some mixin type files, remove properties that are already defined in AbstractGraph.
66115fc to
37219de
Compare
There was a problem hiding this comment.
Actionable comments posted: 0
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
packages/core/src/view/AbstractGraph.ts (1)
98-125: Fix shared mutable default in PageBreaksMixin (initialize per-instance)
packages/core/src/view/mixins/PageBreaksMixin.ts:59 — exported PageBreaksMixin contains an array literal property (shared mutable state); change to a factory/getter or initialize the array inside each instance so it isn’t shared.
🧹 Nitpick comments (2)
packages/core/__tests__/view/BaseGraph.test.ts (1)
296-310: Avoid brittle assumptions about default listener countAsserting exact initial length (1) can break if defaults change. Derive from the initial value instead.
- const graph1 = new BaseGraph(); - expect(graph1.mouseListeners).toHaveLength(1); - const graph2 = new BaseGraph(); - expect(graph2.mouseListeners).toHaveLength(1); + const graph1 = new BaseGraph(); + const graph2 = new BaseGraph(); + const g1Initial = graph1.mouseListeners.length; + const g2Initial = graph2.mouseListeners.length; + expect(g1Initial).toBe(g2Initial); graph1.mouseListeners.push({ mouseDown: () => {}, mouseMove: () => {}, mouseUp: () => {}, }); - expect(graph2.mouseListeners).toHaveLength(1); - expect(graph1.mouseListeners).toHaveLength(2); + expect(graph2.mouseListeners).toHaveLength(g2Initial); + expect(graph1.mouseListeners).toHaveLength(g1Initial + 1);packages/core/src/view/AbstractGraph.ts (1)
92-95: Comment wording nitConsider clarifying that these variables used to live in mixins but must be instantiated per graph to avoid shared state.
-// Group: Variables that should be in the mixins -// The current implementation of the function applying mixins create a shared state for properties with "complex" type (object, array) -// whereas here, we need to be created a specific instance for each new class instance. +// Group: Variables previously in mixins but requiring per-instance initialization +// The mixin application currently causes shared state for complex types (object/array), +// so these are defined here to ensure a fresh instance per Graph.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (7)
packages/core/__tests__/serialization/codecs/all-graph-classes.test.ts(3 hunks)packages/core/__tests__/view/BaseGraph.test.ts(2 hunks)packages/core/src/internal/utils.ts(2 hunks)packages/core/src/view/AbstractGraph.ts(4 hunks)packages/core/src/view/mixins/EdgeMixin.ts(1 hunks)packages/core/src/view/mixins/EdgeMixin.type.ts(0 hunks)packages/core/src/view/mixins/ImageMixin.type.ts(0 hunks)
💤 Files with no reviewable changes (2)
- packages/core/src/view/mixins/EdgeMixin.type.ts
- packages/core/src/view/mixins/ImageMixin.type.ts
✅ Files skipped from review due to trivial changes (1)
- packages/core/src/internal/utils.ts
🧰 Additional context used
🧠 Learnings (3)
📓 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#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.
📚 Learning: 2025-04-24T12:34:56.726Z
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.
Applied to files:
packages/core/src/view/AbstractGraph.ts
📚 Learning: 2025-04-24T12:34:10.366Z
Learnt from: tbouffard
PR: maxGraph/maxGraph#776
File: packages/core/src/view/Graph.ts:85-92
Timestamp: 2025-04-24T12:34:10.366Z
Learning: The Graph class intentionally doesn't use collaborators (cellRenderer, selectionModel, view) from options in initializeCollaborators, as it's designed to replicate the previous implementation using only its factory methods for backward compatibility.
Applied to files:
packages/core/src/view/AbstractGraph.ts
🧬 Code graph analysis (1)
packages/core/src/view/AbstractGraph.ts (1)
packages/core/src/types.ts (2)
CellStyle(50-75)GraphFoldingOptions(1441-1462)
🔇 Additional comments (12)
packages/core/src/view/mixins/EdgeMixin.ts (2)
27-41: Graph-level alternateEdgeStyle wiring — LGTMPicking 'alternateEdgeStyle' from AbstractGraph in PartialGraph removes the shared-state hazard from the mixin. Matches the PR goal.
149-166: Confirm flipEdge toggle semantics didn't regressEdgeMixin.ts currently applies alternateEdgeStyle when edge.getStyle() is non-empty and sets {} when empty — this is inverted vs. the method doc which says it should toggle between null/empty and alternateEdgeStyle (packages/core/src/view/mixins/EdgeMixin.type.ts:115–117). If this is legacy/intentional, resolve; otherwise add a test and invert the condition so empty style → apply alternateEdgeStyle, non-empty → clear it.
packages/core/__tests__/view/BaseGraph.test.ts (2)
29-31: New public types imports — LGTMImporting ImageBundle and Multiplicity from the public surface aligns tests with the new API.
266-340: Per-instance isolation tests — LGTMGood coverage for alternateEdgeStyle, cells, imageBundles, mouseListeners, multiplicities, and options. Confirms the refactor eliminates shared state.
packages/core/__tests__/serialization/codecs/all-graph-classes.test.ts (3)
24-25: Multiplicity import on public API — LGTMConfirms Multiplicity is exposed for consumer use and testing.
44-62: Serialize new graph-level fields — LGTMTemplate includes alternateEdgeStyle, multiplicities, and options with images. Matches the refactor.
109-121: Populate multiplicities before export — LGTMEnsures codec covers the new field; keeps the test self-contained.
packages/core/src/view/AbstractGraph.ts (5)
49-59: Type-only import for CellStyle — LGTMKeeps runtime lean and clarifies intent.
60-60: ImageBundle as type-only — LGTMAvoids accidental side effects from runtime imports.
98-103: Per-instance alternateEdgeStyle — LGTMDefining CellStyle = {} on the class removes the shared object from the mixin and aligns with tests.
118-125: Per-instance folding options — LGTMFresh Image instances and object literal prevent cross-graph leakage. Matches serialization expectations.
116-117: Resolved — multiplicities are per-instance and validation stories require no follow-up.
Validation.stories.ts pushes to graph.multiplicities and ValidationMixin reads this.multiplicities; AbstractGraph initializes multiplicities as an instance array.
|
The current implementation of the function applying mixins creates shared state for "object" or "array" properties.
For such properties, ensure they are all defined in AbstractGraph and add tests to validate that there is no global
states. Here, this includes a fix for 'alternateEdgeStyle' which is no longer a global state.
In some mixin type files, remove properties that are already defined in AbstractGraph.
Tasks
Graph.optionstruly per-instance #751 that workaround similar problems in the pastSummary by CodeRabbit
New Features
Refactor
Documentation
Tests