refactor!: move getViewXml from xmlUtils to xmlViewUtils#992
Conversation
The getViewXml function depends on high-level APIs (Codec, TemporaryCellStates) and is only used by the Editor class. Moving it to a dedicated xmlViewUtils module improves separation of concerns: - xmlUtils remains focused on low-level XML parsing utilities - xmlViewUtils handles view-specific XML serialization BREAKING CHANGE: xmlUtils.getViewXml moved to xmlViewUtils.getViewXml.
WalkthroughMoved view-serialization logic into a new module Changes
Sequence Diagram(s)mermaid Estimated code review effort🎯 3 (Moderate) | ⏱️ ~22 minutes 🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing touches🧪 Generate unit tests (beta)
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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (3)
packages/core/src/util/xmlUtils.ts (1)
28-30: Add explicit return type annotation.Per coding guidelines, TypeScript source files in the core package must have explicit return type annotations on all functions.
Suggested fix
-export const createXmlDocument = () => { +export const createXmlDocument = (): Document => { return document.implementation.createDocument('', '', null); };packages/core/src/util/xmlViewUtils.ts (2)
26-32: Add explicit return type annotation.Per coding guidelines, TypeScript functions in the core package must have explicit return type annotations. The current signature lacks a return type.
Suggested fix
export const getViewXml = ( graph: AbstractGraph, scale = 1, cells: Cell[] | null = null, x0 = 0, y0 = 0 -) => { +): Element | null => {Note: The actual return type depends on what
Codec.encode()returns. If it returnsany, you may want to keep it asanyor use a more specific type based on the Codec's actual behavior.
33-36: Null handling forgetRoot()could be more defensive.The type assertion
<Cell>model.getRoot()assumes the root is never null. While this is typically true for initialized graphs, a defensive check could improve robustness.This is a minor concern since the original code had the same pattern, and per learnings, this PR is focused on code reorganization without implementation changes.
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
packages/core/src/editor/Editor.tspackages/core/src/index.tspackages/core/src/util/xmlUtils.tspackages/core/src/util/xmlViewUtils.ts
🧰 Additional context used
📓 Path-based instructions (6)
packages/core/src/**/*.ts
📄 CodeRabbit inference engine (.github/copilot-instructions.md)
packages/core/src/**/*.ts: Include.jsfile extensions in all import statements within packages/core/src/ to comply with ESLint rulen/file-extension-in-import.
Do not use console statements in any code. Use proper logging mechanisms instead (ESLint rule:no-console: error).
Do not useeval()function in any code (ESLint rule:no-eval: error).
Do not use const enums. Convert all const enums to regular enums (ESLint rule forbids const enums).
TypeScript source files in core package must strictly maintain typing with no implicit types and all functions having explicit return type annotations.Use singular form for folder names (e.g.,
handler/nothandlers/) and kebab-case for folder names (e.g.,my-feature/notmyFeature/ormy_feature/)
Files:
packages/core/src/util/xmlUtils.tspackages/core/src/index.tspackages/core/src/util/xmlViewUtils.tspackages/core/src/editor/Editor.ts
**/*.{ts,tsx,js,jsx}
📄 CodeRabbit inference engine (.github/copilot-instructions.md)
Use single quotes in TypeScript/JavaScript code (Prettier config:
singleQuote: true).
Files:
packages/core/src/util/xmlUtils.tspackages/core/src/index.tspackages/core/src/util/xmlViewUtils.tspackages/core/src/editor/Editor.ts
**/*.{ts,tsx,js,jsx,json,md}
📄 CodeRabbit inference engine (.github/copilot-instructions.md)
Format code using Prettier with tab width 2, trailing comma ES5, print width 90, and end of line auto.
Files:
packages/core/src/util/xmlUtils.tspackages/core/src/index.tspackages/core/src/util/xmlViewUtils.tspackages/core/src/editor/Editor.ts
**/*.ts
📄 CodeRabbit inference engine (CLAUDE.md)
**/*.ts: Use theisNullish()function when checking fornullorundefinedon variables that can have falsy values (numbers, strings, booleans) to avoid bugs where0,"", orfalseare incorrectly treated as nullish
Use thelog()function frominternal/utils.jsinstead of accessingGlobalConfig.loggerdirectly for logging
Use functionstranslate()andisI18nEnabled()frominternal/i18n-utils.jsinstead of accessingGlobalConfig.i18ndirectly for internationalization
Always wrap multiple model changes inbatchUpdate()to ensure efficient graph updates
Files:
packages/core/src/util/xmlUtils.tspackages/core/src/index.tspackages/core/src/util/xmlViewUtils.tspackages/core/src/editor/Editor.ts
**/*.{ts,js}
📄 CodeRabbit inference engine (CLAUDE.md)
Prefer using
BaseGraphin production with explicit feature registration for optimal tree-shaking and minimal bundle size; useGraphonly for prototyping and demos with automatic defaults
Files:
packages/core/src/util/xmlUtils.tspackages/core/src/index.tspackages/core/src/util/xmlViewUtils.tspackages/core/src/editor/Editor.ts
packages/core/src/**
📄 CodeRabbit inference engine (CLAUDE.md)
Organize code using the mixin pattern where each mixin provides a specific capability with two files: implementation (.ts) and type definitions (.type.ts), with unified type definitions in
_graph-mixins-types.ts
Files:
packages/core/src/util/xmlUtils.tspackages/core/src/index.tspackages/core/src/util/xmlViewUtils.tspackages/core/src/editor/Editor.ts
🧠 Learnings (2)
📓 Common learnings
Learnt from: tbouffard
Repo: maxGraph/maxGraph PR: 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
Repo: maxGraph/maxGraph PR: 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
Repo: maxGraph/maxGraph 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
Repo: maxGraph/maxGraph PR: 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-03-31T08:32:17.055Z
Learnt from: tbouffard
Repo: maxGraph/maxGraph PR: 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.
Applied to files:
packages/core/src/index.ts
🧬 Code graph analysis (1)
packages/core/src/util/xmlViewUtils.ts (1)
packages/core/src/util/Constants.ts (1)
NS_SVG(52-52)
⏰ 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). (3)
- GitHub Check: build (windows-2022)
- GitHub Check: build (macos-14)
- GitHub Check: build (ubuntu-24.04)
🔇 Additional comments (5)
packages/core/src/util/xmlUtils.ts (1)
19-23: Clean import cleanup after refactoring.The imports are correctly updated to remove dependencies that were only needed by the moved
getViewXmlfunction. TheStyleValuetype import is correctly preserved as a type-only import.packages/core/src/index.ts (1)
183-186: New public API export follows existing conventions.The new
xmlViewUtilsexport is correctly placed after the existingxmlUtilsexport, follows the same pattern with JSDoc category annotation, and uses the.jsextension in the import path as required.packages/core/src/editor/Editor.ts (1)
48-49: Import update correctly reflects module reorganization.The imports are properly split between
xmlUtils(forgetXml) andxmlViewUtils(forgetViewXml), both with correct.jsextensions. The usage in theexportImageaction remains unchanged.packages/core/src/util/xmlViewUtils.ts (2)
74-85: Well-structured cleanup ensures view state restoration.The
try/finallyblock properly ensures all temporary state is cleaned up regardless of whether encoding succeeds or fails. This includes destroying temporary cell states, restoring the translate, removing temporary panes, restoring original panes, and re-enabling events.
19-24: Imports correctly structured with.jsextensions.All imports use the required
.jsextension and appropriately usetypeimports forCellandAbstractGraphsince they are only used in type positions.
|
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
CHANGELOG.md (1)
15-15: Consider wrapping the line to fit the print width guideline.Line 15 exceeds the Prettier print width of 90 characters (currently ~165 characters). While the content is accurate and well-documented, wrapping it would align with the project's formatting standards.
Suggested line wrap
-- `xmlUtils.getViewXml` moved to `xmlViewUtils.getViewXml`. The impact should be limited as this function was not widely used (only in the Editor class in the maxGraph code). +- `xmlUtils.getViewXml` moved to `xmlViewUtils.getViewXml`. The impact should be + limited as this function was not widely used (only in the Editor class in the + maxGraph code).
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
CHANGELOG.md
🧰 Additional context used
📓 Path-based instructions (1)
**/*.{ts,tsx,js,jsx,json,md}
📄 CodeRabbit inference engine (.github/copilot-instructions.md)
Format code using Prettier with tab width 2, trailing comma ES5, print width 90, and end of line auto.
Files:
CHANGELOG.md
🧠 Learnings (1)
📓 Common learnings
Learnt from: tbouffard
Repo: maxGraph/maxGraph PR: 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
Repo: maxGraph/maxGraph 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
Repo: maxGraph/maxGraph PR: 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
Repo: maxGraph/maxGraph PR: 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.
⏰ 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 (windows-2022)
- GitHub Check: build (ubuntu-24.04)
- GitHub Check: build
1 participant
The getViewXml function depends on high-level APIs (Codec, TemporaryCellStates) and is only used by the Editor class.
Moving it to a dedicated xmlViewUtils module improves separation of concerns:
BREAKING CHANGE: xmlUtils.getViewXml moved to xmlViewUtils.getViewXml.
Summary by CodeRabbit
Refactor
New Features
Improvements
✏️ Tip: You can customize this high-level summary in your review settings.