fix!: improve the robustness of codecs #872
Conversation
GraphViewCodec and StylesheetCodec: - fix style property retrieval: correctly loop over the properties of the style of CellState - add tests Other fixes: - Graph codec: only exclude existing fields - exclude and idrefs: enforce type in array - ObjectCodec: use right numeric attributes names for Geometry and Point Improvements: - ModelCodec: improve method signature of encodeObject - use explicit names for object and array codecs - decode/import: generate a warning log when no codec found for a node - types in codecs: use specific types instead of any to better detect type errors - declare more precise types in method signature of EditorToolbar and accept null parameters in some methods - improve JSDoc - add tests for GraphDataModel codec for extra node in addition to root BREAKING CHANGES: the return types of some methods of EditorToolbar are now more precise. - addPrototype(): HTMLImageElement, no longer HTMLImageElement | HTMLButtonElement - addCombo(): HTMLSelectElement instead of HTMLElement
WalkthroughThis update refines type safety and documentation across serialization, editor, and view modules. It introduces new and refactored tests for serialization codecs, enhances method signatures, clarifies JSDoc comments, and improves internal type handling. A new test utility module is added, and several methods now use more precise parameter and return types. Changes
Sequence Diagram(s)sequenceDiagram
participant TestSuite as Test Suite
participant Codec as Codec
participant XML as XML Document
participant Object as Target Object
TestSuite->>Codec: importToObject(Object, xmlString)
Codec->>XML: parseXml(xmlString)
Codec->>Object: decode(XML.root, Object)
Object-->>TestSuite: Object populated
TestSuite->>Codec: exportObject(Object)
Codec->>XML: encode(Object)
Codec->>TestSuite: getPrettyXml(XML.node)
Suggested labels
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
npm error Exit handler never called! 📜 Recent review detailsConfiguration used: CodeRabbit UI 📒 Files selected for processing (3)
✅ Files skipped from review due to trivial changes (1)
🚧 Files skipped from review as they are similar to previous changes (2)
⏰ 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)
✨ Finishing Touches
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. 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed 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)
Other keywords and placeholders
CodeRabbit Configuration File (
|
packages/core/__tests__/serialization/codecs/GraphViewCodec.test.ts
Outdated
Show resolved
Hide resolved
packages/core/__tests__/serialization/codecs/GraphViewCodec.test.ts
Outdated
Show resolved
Hide resolved
| * Lookup table for resolving IDs to elements. | ||
| */ | ||
| elements: any = null; // TODO why not { [key: string]: Element } | null | ||
| elements: Record<string, Element> = {}; |
There was a problem hiding this comment.
thought: not a breaking change, this is an internal property
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (1)
CHANGELOG.md (1)
19-21: LGTM! Changelog accurately documents EditorToolbar API changes.The breaking changes are clearly documented, showing the more precise return types for
addPrototype()andaddCombo()methods. This addresses the previous review feedback and helps users understand the API refinements.
🧹 Nitpick comments (2)
packages/core/src/util/EventUtils.ts (1)
134-138: One lingering JSDoc reference still points tomxGraph.isMouseDown.The comment for
isMiddleMouseButton(lines 136-138) wasn’t updated and now diverges from the earlier fix.- * {@link mxGraph.isMouseDown} property. + * {@link Graph.isMouseDown} property.packages/core/src/serialization/ObjectCodec.ts (1)
29-36: LGTM: Well-defined numeric attribute constants.The constant arrays provide clear, maintainable definitions of numeric attributes for
GeometryandPointobjects. This approach is more reliable than string-based checks and easier to maintain.Note: Line 35 appears to have an incorrect type annotation -
pointNumericAttributesshould beArray<keyof Point>instead ofArray<keyof Geometry>.Fix the type annotation:
-const pointNumericAttributes: Array<keyof Geometry> = ['_x', '_y']; +const pointNumericAttributes: Array<keyof Point> = ['_x', '_y'];
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (28)
CHANGELOG.md(1 hunks)packages/core/__tests__/serialization/codecs/GraphViewCodec.test.ts(1 hunks)packages/core/__tests__/serialization/codecs/StylesheetCodec.test.ts(1 hunks)packages/core/__tests__/serialization/codecs/all-graph-classes.test.ts(4 hunks)packages/core/__tests__/serialization/codecs/shared.ts(1 hunks)packages/core/__tests__/serialization/serialization.xml.test.ts(3 hunks)packages/core/__tests__/serialization/utils.ts(4 hunks)packages/core/src/editor/EditorToolbar.ts(8 hunks)packages/core/src/serialization/Codec.ts(7 hunks)packages/core/src/serialization/CodecRegistry.ts(1 hunks)packages/core/src/serialization/ObjectCodec.ts(7 hunks)packages/core/src/serialization/codecs/BaseGraphCodec.ts(0 hunks)packages/core/src/serialization/codecs/CellCodec.ts(1 hunks)packages/core/src/serialization/codecs/ChildChangeCodec.ts(1 hunks)packages/core/src/serialization/codecs/GraphCodec.ts(1 hunks)packages/core/src/serialization/codecs/GraphViewCodec.ts(6 hunks)packages/core/src/serialization/codecs/ModelCodec.ts(2 hunks)packages/core/src/serialization/codecs/RootChangeCodec.ts(1 hunks)packages/core/src/serialization/codecs/StylesheetCodec.ts(1 hunks)packages/core/src/serialization/codecs/TerminalChangeCodec.ts(1 hunks)packages/core/src/serialization/codecs/editor/EditorCodec.ts(4 hunks)packages/core/src/serialization/codecs/editor/EditorKeyHandlerCodec.ts(1 hunks)packages/core/src/serialization/codecs/editor/EditorPopupMenuCodec.ts(1 hunks)packages/core/src/serialization/codecs/editor/EditorToolbarCodec.ts(10 hunks)packages/core/src/serialization/register-shared.ts(1 hunks)packages/core/src/util/EventUtils.ts(1 hunks)packages/core/src/view/GraphView.ts(1 hunks)packages/core/src/view/cell/CellState.ts(1 hunks)
💤 Files with no reviewable changes (1)
- packages/core/src/serialization/codecs/BaseGraphCodec.ts
🧰 Additional context used
🧠 Learnings (18)
📓 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#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#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#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#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#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/cell/CellState.ts (1)
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.
packages/core/src/util/EventUtils.ts (2)
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.
packages/core/src/serialization/codecs/CellCodec.ts (2)
Learnt from: tbouffard
PR: maxGraph/maxGraph#618
File: packages/website/docs/tutorials/graph.md:56-59
Timestamp: 2025-01-06T09:09:47.410Z
Learning: In the maxGraph library, the classes "Cell" and "GraphDataModel" are valid, while "GraphCell" and "GraphModel" do not exist.
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.
packages/core/__tests__/serialization/codecs/shared.ts (2)
Learnt from: tbouffard
PR: maxGraph/maxGraph#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.
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.
CHANGELOG.md (5)
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#726
File: packages/core/src/editor/EditorPopupMenu.ts:197-197
Timestamp: 2025-03-25T14:13:25.331Z
Learning: The team has decided to keep using `eval()` in EditorPopupMenu.ts as the implementation is loaded from XML configuration. Finding alternatives to `eval()` for improved security is planned for future work.
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#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/serialization/codecs/editor/EditorPopupMenuCodec.ts (1)
Learnt from: tbouffard
PR: maxGraph/maxGraph#726
File: packages/core/src/editor/EditorPopupMenu.ts:197-197
Timestamp: 2025-03-25T14:13:25.331Z
Learning: The team has decided to keep using `eval()` in EditorPopupMenu.ts as the implementation is loaded from XML configuration. Finding alternatives to `eval()` for improved security is planned for future work.
packages/core/src/serialization/codecs/GraphCodec.ts (8)
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#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#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#774
File: packages/ts-example-selected-features/package.json:11-13
Timestamp: 2025-04-22T16:34:40.309Z
Learning: In the maxGraph project, TypeScript is defined as a dependency in the root package.json and not in individual package.json files for example packages. The build scripts in example packages can use `tsc` because it's available through this hoisted dependency in the monorepo structure.
Learnt from: tbouffard
PR: maxGraph/maxGraph#774
File: packages/ts-example-selected-features/package.json:11-13
Timestamp: 2025-04-22T16:34:40.309Z
Learning: In the maxGraph project, TypeScript is defined as a dependency in the root package.json (~5.8.2) and not in individual package.json files for example packages. The build scripts in example packages can use `tsc` commands because the TypeScript dependency is hoisted in the monorepo structure.
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/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.
Learnt from: tbouffard
PR: maxGraph/maxGraph#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__/serialization/codecs/GraphViewCodec.test.ts (6)
Learnt from: tbouffard
PR: maxGraph/maxGraph#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.
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#826
File: packages/js-example-nodejs/src/index.cjs:64-69
Timestamp: 2025-05-13T12:54:55.231Z
Learning: For example code in the maxGraph repository, maintainers prefer to keep scripts simple without error handling to focus on demonstrating core functionality, especially in demonstration scripts like those in packages/js-example-nodejs.
Learnt from: tbouffard
PR: maxGraph/maxGraph#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.
Learnt from: tbouffard
PR: maxGraph/maxGraph#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: maxGraph/maxGraph#791
File: packages/ts-example/vite.config.js:30-30
Timestamp: 2025-04-29T13:25:31.494Z
Learning: In the maxGraph project, each example package (ts-example, ts-example-selected-features, ts-example-without-defaults) implements different use cases with varying features, resulting in different application sizes. Therefore, each package has its own specific chunkSizeWarningLimit value in its vite.config.js file, calibrated to its expected bundle size.
packages/core/__tests__/serialization/codecs/StylesheetCodec.test.ts (4)
Learnt from: tbouffard
PR: maxGraph/maxGraph#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.
Learnt from: tbouffard
PR: maxGraph/maxGraph#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.
Learnt from: tbouffard
PR: maxGraph/maxGraph#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: 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.
packages/core/src/serialization/codecs/editor/EditorToolbarCodec.ts (1)
Learnt from: tbouffard
PR: maxGraph/maxGraph#726
File: packages/core/src/editor/EditorPopupMenu.ts:197-197
Timestamp: 2025-03-25T14:13:25.331Z
Learning: The team has decided to keep using `eval()` in EditorPopupMenu.ts as the implementation is loaded from XML configuration. Finding alternatives to `eval()` for improved security is planned for future work.
packages/core/src/view/GraphView.ts (1)
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.
packages/core/__tests__/serialization/codecs/all-graph-classes.test.ts (10)
Learnt from: tbouffard
PR: maxGraph/maxGraph#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.
Learnt from: tbouffard
PR: maxGraph/maxGraph#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: 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#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#774
File: packages/ts-example-selected-features/package.json:11-13
Timestamp: 2025-04-22T16:34:40.309Z
Learning: In the maxGraph project, TypeScript is defined as a dependency in the root package.json and not in individual package.json files for example packages. The build scripts in example packages can use `tsc` because it's available through this hoisted dependency in the monorepo structure.
Learnt from: tbouffard
PR: maxGraph/maxGraph#826
File: packages/js-example-nodejs/src/index.cjs:64-69
Timestamp: 2025-05-13T12:54:55.231Z
Learning: For example code in the maxGraph repository, maintainers prefer to keep scripts simple without error handling to focus on demonstrating core functionality, especially in demonstration scripts like those in packages/js-example-nodejs.
Learnt from: tbouffard
PR: maxGraph/maxGraph#598
File: packages/website/docs/manual/getting-started.md:70-70
Timestamp: 2024-12-15T18:19:56.236Z
Learning: In code examples within the documentation, such as in `packages/website/docs/manual/getting-started.md`, we assume that the `graph-container` element exists and is an `HTMLElement`, and we avoid adding error handling for its initialization to keep the code simple.
Learnt from: tbouffard
PR: maxGraph/maxGraph#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.
Learnt from: tbouffard
PR: maxGraph/maxGraph#618
File: packages/website/docs/tutorials/graph.md:56-59
Timestamp: 2025-01-06T09:09:47.410Z
Learning: In the maxGraph library, the classes "Cell" and "GraphDataModel" are valid, while "GraphCell" and "GraphModel" do not exist.
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.
packages/core/src/serialization/codecs/GraphViewCodec.ts (2)
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#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.
packages/core/src/serialization/codecs/editor/EditorCodec.ts (1)
Learnt from: tbouffard
PR: maxGraph/maxGraph#726
File: packages/core/src/editor/EditorPopupMenu.ts:197-197
Timestamp: 2025-03-25T14:13:25.331Z
Learning: The team has decided to keep using `eval()` in EditorPopupMenu.ts as the implementation is loaded from XML configuration. Finding alternatives to `eval()` for improved security is planned for future work.
packages/core/__tests__/serialization/serialization.xml.test.ts (3)
Learnt from: tbouffard
PR: maxGraph/maxGraph#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.
Learnt from: tbouffard
PR: maxGraph/maxGraph#618
File: packages/website/docs/tutorials/graph.md:56-59
Timestamp: 2025-01-06T09:09:47.410Z
Learning: In the maxGraph library, the classes "Cell" and "GraphDataModel" are valid, while "GraphCell" and "GraphModel" do not exist.
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.
packages/core/__tests__/serialization/utils.ts (2)
Learnt from: tbouffard
PR: maxGraph/maxGraph#835
File: packages/html/stories/Orthogonal.stories.ts:114-117
Timestamp: 2025-05-26T12:34:54.318Z
Learning: The `null!` assertion pattern exists across multiple stories in the TypeScript migration and should be addressed systematically rather than in individual PRs.
Learnt from: tbouffard
PR: maxGraph/maxGraph#618
File: packages/website/docs/tutorials/graph.md:56-59
Timestamp: 2025-01-06T09:09:47.410Z
Learning: In the maxGraph library, the classes "Cell" and "GraphDataModel" are valid, while "GraphCell" and "GraphModel" do not exist.
packages/core/src/editor/EditorToolbar.ts (1)
Learnt from: tbouffard
PR: maxGraph/maxGraph#726
File: packages/core/src/editor/EditorPopupMenu.ts:197-197
Timestamp: 2025-03-25T14:13:25.331Z
Learning: The team has decided to keep using `eval()` in EditorPopupMenu.ts as the implementation is loaded from XML configuration. Finding alternatives to `eval()` for improved security is planned for future work.
🧬 Code Graph Analysis (14)
packages/core/src/serialization/codecs/TerminalChangeCodec.ts (1)
packages/core/src/view/undoable_changes/TerminalChange.ts (1)
TerminalChange(27-53)
packages/core/src/serialization/codecs/CellCodec.ts (1)
packages/core/src/view/cell/Cell.ts (1)
Cell(66-888)
packages/core/src/serialization/codecs/ModelCodec.ts (1)
packages/core/src/view/cell/Cell.ts (1)
Cell(66-888)
packages/core/__tests__/serialization/codecs/shared.ts (1)
packages/core/src/util/xmlUtils.ts (1)
getPrettyXml(132-220)
packages/core/src/serialization/codecs/editor/EditorKeyHandlerCodec.ts (1)
packages/core/src/editor/EditorKeyHandler.ts (1)
EditorKeyHandler(48-105)
packages/core/src/serialization/codecs/editor/EditorPopupMenuCodec.ts (1)
packages/core/src/editor/EditorPopupMenu.ts (1)
EditorPopupMenu(41-329)
packages/core/src/serialization/codecs/RootChangeCodec.ts (1)
packages/core/src/view/undoable_changes/RootChange.ts (1)
RootChange(27-45)
packages/core/src/serialization/codecs/GraphCodec.ts (1)
packages/core/src/index.ts (1)
AbstractGraph(20-20)
packages/core/__tests__/serialization/codecs/GraphViewCodec.test.ts (4)
packages/core/src/view/Graph.ts (1)
createGraphView(59-61)packages/core/src/view/GraphView.ts (1)
GraphView(99-2393)packages/core/src/serialization/register-other-codecs.ts (2)
unregisterAllCodecs(141-149)registerCoreCodecs(78-97)packages/core/__tests__/serialization/codecs/shared.ts (1)
exportObject(25-28)
packages/core/src/serialization/ObjectCodec.ts (2)
packages/core/src/util/domUtils.ts (1)
write(144-153)packages/core/src/gui/MaxLog.ts (1)
write(321-345)
packages/core/src/serialization/codecs/editor/EditorToolbarCodec.ts (2)
packages/core/src/editor/EditorToolbar.ts (1)
EditorToolbar(59-478)packages/core/src/editor/Editor.ts (1)
Editor(391-2597)
packages/core/src/serialization/codecs/StylesheetCodec.ts (1)
packages/core/src/view/style/Stylesheet.ts (1)
Stylesheet(49-200)
packages/core/__tests__/serialization/codecs/all-graph-classes.test.ts (1)
packages/core/__tests__/serialization/codecs/shared.ts (2)
exportObject(25-28)importToObject(20-23)
packages/core/src/serialization/codecs/GraphViewCodec.ts (2)
packages/core/src/view/GraphView.ts (1)
GraphView(99-2393)packages/core/src/view/cell/Cell.ts (1)
Cell(66-888)
⏰ 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). (2)
- GitHub Check: build (ubuntu-24.04)
- GitHub Check: build (windows-2022)
🔇 Additional comments (65)
packages/core/src/util/EventUtils.ts (1)
116-122: 👍 Reference update toGraph.isMouseDownlooks correct.Thanks for aligning the JSDoc with the renamed symbol.
packages/core/src/view/cell/CellState.ts (1)
92-92: LGTM! JSDoc link formatting improved.The change from
<Point>to{@link Point}follows proper JSDoc linking conventions and will improve documentation generation.packages/core/src/serialization/codecs/TerminalChangeCodec.ts (1)
43-44: LGTM! Type assertions improve type safety.The explicit casting to
Array<keyof TerminalChange>provides stronger type checking and ensures the arrays contain valid property names from theTerminalChangeclass. This aligns with the broader type safety improvements across codec classes in this PR.packages/core/src/serialization/codecs/CellCodec.ts (1)
59-60: LGTM! Type assertions enhance type safety.The explicit casting to
Array<keyof Cell>ensures compile-time verification that the arrays contain validCellproperty names. This is consistent with similar improvements across other codec classes in this PR and strengthens type checking without affecting runtime behavior.packages/core/src/serialization/codecs/editor/EditorPopupMenuCodec.ts (1)
45-47: LGTM! Documentation and type safety improvements.Two positive changes:
- JSDoc improvement from
<EditorPopupMenu>to{@link EditorPopupMenu}follows proper linking conventions- Parameter type refinement from
anytoEditorPopupMenuprovides better type safetyThese changes align with the broader type safety enhancements across editor codec classes in this PR.
packages/core/src/serialization/codecs/editor/EditorKeyHandlerCodec.ts (1)
38-40: LGTM! Good type safety improvement.The method signature now uses specific types (
CodecandEditorKeyHandler) instead of generic types, which aligns with the PR's goal of improving type safety across codec classes.packages/core/src/serialization/codecs/RootChangeCodec.ts (1)
38-40: LGTM! Explicit type assertion improves type safety.The explicit cast to
Array<keyof RootChange>ensures compile-time verification that the property names match the RootChange interface keys, preventing typos and improving type safety.packages/core/src/serialization/codecs/ModelCodec.ts (2)
39-39: LGTM! Type annotation improvement.The method signature now uses the specific
Codectype instead of genericany, improving type safety and clarity.
52-52: LGTM! Clearer method dispatch.Using
super.decodeChild(dec, child, obj)instead ofapplyis more direct and clearer, explicitly indicating that the parent class implementation is being called.packages/core/src/serialization/CodecRegistry.ts (1)
78-81: LGTM! Improved naming consistency.The parameter names now follow camelCase conventions (
classNameandcodecName), which improves code consistency and readability.packages/core/src/serialization/register-shared.ts (2)
37-38: LGTM! Good refactoring to use helper function.Using the
createObjectCodechelper function centralizes codec creation logic and provides consistent naming conventions.
44-48: LGTM! Useful helper function.The
createObjectCodechelper function provides a clean abstraction for creating and naming ObjectCodec instances, reducing code duplication and ensuring consistent codec registration.packages/core/src/serialization/codecs/ChildChangeCodec.ts (3)
45-46: Excellent type safety improvement.The explicit type assertions ensure that the arrays contain only valid keys of
ChildChange, improving compile-time type checking and preventing potential runtime errors.
54-54: Good alignment with superclass signature.The nullable
attrparameter aligns with theObjectCodec.isReferencemethod signature, ensuring consistent type handling across the inheritance hierarchy.
58-58: Improved delegation pattern.The change from direct array lookup to delegating to
super.isReferenceis more maintainable and follows proper inheritance patterns. This ensures consistent reference determination logic across the codec hierarchy.packages/core/src/serialization/codecs/GraphCodec.ts (2)
21-21: Necessary import for type annotation.The
AbstractGraphimport is required to support the type annotation on theexcludedFieldsarray.
23-31: Enhanced type safety with appropriate handling of private property.The explicit typing of
excludedFieldsasArray<keyof AbstractGraph>improves compile-time type checking. The@ts-ignoredirective appropriately handles the privatepluginsproperty that still needs to be excluded from serialization.packages/core/__tests__/serialization/codecs/StylesheetCodec.test.ts (3)
22-30: Excellent test isolation setup.The setup and teardown hooks properly manage codec registration to prevent side effects between tests, following best practices for test isolation.
32-51: Comprehensive import test coverage.The test validates both XML parsing and the correct mapping of style properties, ensuring the import functionality works as expected.
53-89: Thorough export verification.The test verifies the complete XML output including both default styles (
defaultVertex,defaultEdge) and custom styles, providing comprehensive coverage of the export functionality.packages/core/__tests__/serialization/codecs/shared.ts (2)
20-23: Clean and focused utility function.The
importToObjectfunction provides a clean abstraction for XML-to-object deserialization, properly utilizing the existingparseXmlandCodecinfrastructure.
25-28: Well-designed export utility.The
exportObjectfunction encapsulates the encode-and-format pattern used across tests, promoting consistency and code reuse throughout the test suite.packages/core/__tests__/serialization/codecs/GraphViewCodec.test.ts (3)
27-30: Simple and effective helper function.The
createGraphViewhelper provides a clean way to create aGraphViewinstance for testing purposes.
43-43: Helpful explanatory comment.The comment clearly explains why only export functionality is tested, providing valuable context for future maintainers.
44-103: Comprehensive export test with complex graph structure.The test creates a sophisticated graph with vertices, edges, child elements, and various style properties, then validates the complete XML serialization including positioning, styling, and hierarchical relationships. This provides excellent coverage of the
GraphViewCodecexport functionality.packages/core/src/view/GraphView.ts (1)
465-470: LGTM! Enhanced JSDoc documentation.The JSDoc comment has been properly enhanced to document the new
recurseparameter, clearly explaining its purpose and default behavior. This improves code documentation and developer experience.packages/core/__tests__/serialization/codecs/all-graph-classes.test.ts (3)
28-28: LGTM! Improved code reusability.Good refactoring to use shared utility functions
exportObjectandimportToObjectinstead of local helpers. This promotes consistency across serialization codec tests and reduces code duplication.
60-78: Enhanced test coverage with detailed stylesheet.Excellent enhancement to the XML template by expanding the previously empty
<Stylesheet as="stylesheet" />tag into a fully populated element with default vertex and edge style definitions. This provides better test coverage and aligns with the detailed stylesheet structure tested in other codec test suites.
104-104: Test calls correctly updated for shared utilities.The test assertions have been properly updated to use the new shared functions while maintaining the original test logic and expectations.
Also applies to: 113-113
packages/core/src/serialization/codecs/editor/EditorToolbarCodec.ts (3)
29-29: Good type safety improvement with custom type alias.The
HTMLOptionElementWithCellStyletype alias properly handles the customcellStyleattribute on select options, replacing the previous@ts-ignorecomments. This enhances type safety while maintaining functionality.
133-135: Enhanced type safety with explicit typing.Good improvements to type safety by explicitly typing the
intoparameter asEditorToolbarand using non-null assertion oneditor. The non-null assertion is safe here since the code path only executes wheninto != nullis verified.
144-147: Consistent use of non-null assertions improves type safety.The non-null assertions on properties like
toolbar!,icon!,pressedIcon!are correctly applied based on the control flow context. These assertions align with the coordinated updates inEditorToolbar.tswhere method signatures have been refined to return more specific element types and accept nullable parameters.Also applies to: 164-164, 184-184, 201-203, 214-221, 231-231, 235-235, 245-246, 255-262
packages/core/src/serialization/codecs/StylesheetCodec.ts (3)
52-52: LGTM: Improved type safety.The parameter type change from
anytoStylesheetprovides better type safety and aligns with the method's purpose.
55-61: LGTM: Correct Map iteration pattern.The changes properly use Map methods instead of object-like access:
- Using
obj.styles.keys()for iteration over Map keys- Using
obj.styles.get(styleName)for proper Map value access- Checking
styleNametruthiness instead ofi != nullThis aligns with the
Stylesheet.styles: Map<string, CellStateStyle>type definition.
62-69: LGTM: Safer object property iteration.The changes improve property iteration:
- Using
Object.keys(style ?? {})provides safer iteration over object properties- The TypeScript ignore comment is appropriately documented
- Variable naming is more descriptive (
stylePropertyNamevsj)This ensures the code handles potential null/undefined style objects gracefully.
packages/core/src/serialization/codecs/editor/EditorCodec.ts (2)
51-51: LGTM: Explicit typing for transient fields.The explicit type annotation
Array<keyof Editor>ensures compile-time verification that the transient field names are valid Editor properties.
106-106: LGTM: Improved type safety with specific Editor types.Replacing
anywith specificEditortypes provides:
- Better type checking at compile time
- Improved IDE support and autocomplete
- Clear indication of parameter intent
The underscore prefix for unused parameters follows TypeScript conventions.
Also applies to: 128-128, 146-146
packages/core/src/serialization/ObjectCodec.ts (4)
350-351: LGTM: Improved null handling and modern array method.The changes provide:
- Proper null handling with explicit null check
- Use of
includes()instead ofindexOf()for better readability- Clear boolean return logic
This makes the method more robust and easier to understand.
606-607: LGTM: Enhanced numeric attribute detection.Using
instanceofchecks with the predefined constant arrays provides:
- More reliable type checking than string comparisons
- Better maintainability through centralized attribute definitions
- Clearer logic flow
This approach is more robust than previous implementations.
692-692: LGTM: Appropriate non-null assertion.The non-null assertion is justified here since the comment indicates that subsequent calls work correctly even when
idis null.
873-873: LGTM: Safe method invocation.Using optional chaining (
?.) prevents runtime errors if the object doesn't have apushmethod, making the code more defensive.packages/core/__tests__/serialization/utils.ts (1)
25-25: LGTM: Expanded value type support.The new
NullableStringOrObjecttype alias appropriately broadens the supported cell value types to include objects, which aligns with real-world usage where cells can have complex object values beyond just strings.This enhancement enables better test coverage for serialization scenarios involving complex cell values.
Also applies to: 52-52, 65-65, 78-78
packages/core/src/serialization/codecs/GraphViewCodec.ts (4)
24-26: LGTM: Useful helper function for consistency.The
setNodeAttributehelper function ensures consistent string conversion of numeric values across all attribute setting operations. This eliminates code duplication and reduces the risk of inconsistent formatting.
20-22: LGTM: Enhanced type safety.The improvements provide:
- Type-only imports for better tree-shaking and clearer intent
- Explicit
Codecparameter typing- Clear
Element | nullreturn type specificationThese changes enhance IDE support and catch potential type errors at compile time.
Also applies to: 48-48, 64-65
111-113: LGTM: Modern iteration pattern.Using
Object.keys()instead offor...inprovides:
- More predictable iteration behavior
- Better type safety with explicit property names
- Clearer intent with descriptive variable naming
The TypeScript ignore comment is appropriately documented for the indexed access pattern.
92-92: LGTM: Consistent attribute handling.The changes provide several improvements:
- Cached label assignment reduces redundant method calls
- String value
'true'for HTML attribute ensures proper XML serialization- Consistent use of
setNodeAttributefor all numeric values ensures uniform string conversionThis creates more maintainable and reliable XML output.
Also applies to: 95-95, 103-110, 125-125, 143-159
packages/core/src/serialization/Codec.ts (5)
162-162: LGTM: Improved type safety with proper initialization.The change from nullable
anyto typedRecord<string, Element> = {}improves type safety and provides proper initialization. This aligns well with the corresponding logic update inupdateElements().
240-240: LGTM: Consistent emptiness check with new property type.The logic correctly adapts to the new property initialization, checking for empty object instead of null. This is consistent with the type change on line 162.
358-358: LGTM: Enhanced method signature improves type safety.The refined signature with
object | nullreturn type and optional typedintoparameter is a significant improvement over the previousanytypes.
368-371: LGTM: Valuable debugging enhancement.Adding warning logs when no codec is found greatly improves debugging capabilities. The fallback to return a cloned XML element with the
asattribute removed is also a sensible approach.
414-419: LGTM: Robust runtime type checking.The enhanced method signature and runtime verification that
isCellCodecis a function before invocation prevents potential runtime errors. This is a solid defensive programming practice.packages/core/__tests__/serialization/serialization.xml.test.ts (6)
29-29: LGTM: Required import for XML verification.The
getXmlimport is necessary for the new test that verifies XML element preservation in fallback scenarios.
32-32: LGTM: Enhanced helper function supports object values.Extending the
newVertexsignature to acceptstring | objectenables testing of complex cell values, which is covered in the new test cases.
167-196: LGTM: Comprehensive test for extra XML nodes.This test effectively verifies that extra XML nodes outside the root structure are properly attached as properties on the model instance during import, which is an important edge case for robust XML handling.
198-251: LGTM: Thorough test for complex object cell values.The test comprehensively verifies import of cells with nested object values containing arrays and objects, ensuring proper deserialization of complex data structures.
253-283: LGTM: Excellent test for fallback behavior.This test validates the important fallback scenario where XML nodes without matching codecs are preserved as XML elements, demonstrating the robustness improvements mentioned in the PR objectives.
439-488: LGTM: Complete round-trip testing for object values.This export test complements the import tests, ensuring that complex object values can be properly serialized to XML with the correct structure, providing comprehensive coverage.
packages/core/src/editor/EditorToolbar.ts (8)
53-55: LGTM: Corrected JSDoc reference.Updated documentation to reference the correct
EditorToolbarCodecinstead ofDefaultToolbarCodec.
146-151: LGTM: Enhanced flexibility with nullable parameters.Making
title,icon, andpressedparameters nullable improves the API's flexibility while maintaining type safety.
173-173: LGTM: Precise return type as per breaking changes.The return type change from generic to
HTMLSelectElementaligns with the documented breaking changes in the PR objectives, providing more precise typing.
212-215: LGTM: Improved method signature and typing.The optional
valueparameter and preciseHTMLOptionElementreturn type enhance type safety, with appropriate use of non-null assertion on the toolbar property.
263-263: LGTM: Consistent nullable parameter handling.Making the
iconparameter nullable is consistent with other method improvements and enhances API flexibility.
273-273: LGTM: Breaking change aligns with PR objectives.The return type change from generic to
HTMLImageElementmatches the documented breaking change thataddPrototype()now returnsHTMLImageElementinstead ofHTMLImageElement | HTMLButtonElement.
295-295: LGTM: Appropriate use of optional chaining.Using optional chaining (
this.toolbar?.resetMode()) is safer than the previous casting approach.
299-299: LGTM: Consistent non-null assertion usage.The non-null assertion (
this.toolbar!.addMode) is appropriate here since the toolbar existence is guaranteed by the method's context.
|
GraphViewCodec and StylesheetCodec:
Other fixes:
Improvements:
BREAKING CHANGES: the return types of some methods of EditorToolbar are now more precise.
Summary by CodeRabbit
New Features
Bug Fixes
Refactor
Documentation