refactor!: convert ImageMixin to ImageBundlePlugin#1050
Conversation
Replace ImageMixin with a dedicated ImageBundlePlugin (id 'image-bundle').
Mixins share a single state tree across all Graph instances and pollute AbstractGraph with domain-specific methods;
plugins are per-instance, opt-in on BaseGraph, and keep AbstractGraph lean.
The plugin ships in getDefaultPlugins() so Graph users see no behavioral change. BaseGraph consumers must now opt
in explicitly by adding ImageBundlePlugin to the plugins option. CellsMixin.postProcessCellStyle delegates bundle
key resolution to the plugin via graph.getPlugin<ImageBundlePlugin>('image-bundle'); when the plugin is not
registered the raw CellStateStyle.image is used as-is, matching the pre-existing "no bundle matched" path.
Also:
- Add characterization tests for CellsMixin.postProcessCellStyle covering all 12 paths prior to the refactor so
behavior preservation is verified, then merge them into the existing CellsMixin.test.ts suite.
- Add unit tests for ImageBundlePlugin covering per-graph isolation, add/remove semantics, first-match-wins
lookup, and onDestroy bundle clearing.
- Document the plugin-id kebab-case naming convention in .claude/rules/architecture/coding-practices.md.
- Reduce chunkSizeWarningLimit in ts-example-without-defaults now that the example no longer pulls the
image-bundle code path into its bundle.
- Record the migration under Unreleased breaking changes in CHANGELOG.md.
BREAKING CHANGES:
- AbstractGraph.addImageBundle, removeImageBundle, getImageFromBundles and the imageBundles property no longer
exist on AbstractGraph, Graph, or BaseGraph. Migrate to
graph.getPlugin<ImageBundlePlugin>('image-bundle')?.addImageBundle(bundle) and siblings.
- BaseGraph no longer ships image-bundle support by default; add ImageBundlePlugin to the plugins option to opt in.
- XML serialization of <Graph>/<BaseGraph> no longer emits <Array as="imageBundles" />. Existing XML documents
containing that element still decode without error but the field is silently ignored. Consumers that persist
bundles as part of the graph XML must now serialize the plugin state separately.
- CellsMixin.postProcessCellStyle silently skips bundle key resolution when ImageBundlePlugin is not registered.
This only affects BaseGraph consumers that omit the plugin.
The explore/plan/tasks files served their purpose during implementation; drop them from the branch now that the migration has landed. Keeping them in-tree would clutter the repo with one-off process artifacts.
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
WalkthroughImage-bundle behavior was moved out of AbstractGraph/ImageMixin into a new ImageBundlePlugin ( Changes
Sequence Diagram(s)sequenceDiagram
participant Cells as CellsMixin.postProcessCellStyle
participant Graph as Graph/BaseGraph
participant Plugin as ImageBundlePlugin
participant Bundle as ImageBundle
Cells->>Graph: getPlugin('image-bundle')
Graph-->>Cells: ImageBundlePlugin | undefined
alt Plugin present
Cells->>Plugin: getImageFromBundles(key)
Plugin->>Bundle: bundle.getImage(key) (in insertion order)
Bundle-->>Plugin: image | null
Plugin-->>Cells: first non-null image | null
alt image found
Cells->>Cells: normalize data-URI if needed\nset style.image = resolved image
else no image
Cells->>Cells: preserve original style.image (still run normalization)
end
else Plugin absent
Cells->>Cells: preserve original style.image (still run normalization)
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related issues
Possibly related PRs
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. 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: 2
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/image/ImageBundle.ts (1)
34-45:⚠️ Potential issue | 🟡 MinorUpdate documentation example to use TypeScript and modernize API calls.
Lines 34–45 mix a
javascriptcode fence (line 34) with TypeScript generics syntax on line 45 (<ImageBundlePlugin>), which is invalid JavaScript and would fail if copied. Additionally, the example inconsistently uses the legacymxImageBundle(line 35) alongside the newImageBundlePluginAPI (line 45).Change the fence to
typescriptand update all API references to the current API:Proposed documentation fix
- * ```javascript - * let bundle = new mxImageBundle(alt); + * ```typescript + * const bundle = new ImageBundle(alt);
🧹 Nitpick comments (2)
packages/ts-example-without-defaults/vite.config.js (1)
30-30: LGTM! Bundle size limit appropriately adjusted.The 1 kB reduction aligns with the PR's bundle-size impact (~0.3–0.5 kB shrinkage for examples omitting ImageBundlePlugin). Since
ts-example-without-defaultslikely omits the plugin, this calibration is appropriate.📊 Optional: Verify the actual bundle size matches the new limit
After building, you may want to confirm the actual
@maxgraph/corechunk size to ensure the limit is precisely calibrated:#!/bin/bash # Description: Build the ts-example-without-defaults package and check the maxgraph chunk size cd packages/ts-example-without-defaults npm run build 2>&1 | grep -i "maxgraph" || echo "Build output or chunk size info not found in build logs"packages/core/__tests__/view/plugin/index.test.ts (1)
17-31: Assert the new default plugin explicitly.The length check detects list-size drift, but it would still pass if
ImageBundlePluginwere accidentally omitted and another plugin kept the count at 9. Add a targeted assertion for the behavior this PR depends on.Proposed test strengthening
-import { getDefaultPlugins } from '../../../src'; +import { getDefaultPlugins, ImageBundlePlugin } from '../../../src'; @@ const plugins = getDefaultPlugins(); // detect any changes in default plugins, order does not matter expect(plugins).toHaveLength(9); + expect(plugins).toContain(ImageBundlePlugin);
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: b281f3af-21d7-4715-abeb-3764bbf2ef8d
📒 Files selected for processing (18)
CHANGELOG.mdpackages/core/__tests__/serialization/codec/all-graph-classes.test.tspackages/core/__tests__/view/BaseGraph.test.tspackages/core/__tests__/view/mixin/CellsMixin.test.tspackages/core/__tests__/view/plugin/ImageBundlePlugin.test.tspackages/core/__tests__/view/plugin/index.test.tspackages/core/src/types.tspackages/core/src/view/AbstractGraph.tspackages/core/src/view/image/ImageBundle.tspackages/core/src/view/mixin/CellsMixin.tspackages/core/src/view/mixin/CellsMixin.type.tspackages/core/src/view/mixin/ImageMixin.tspackages/core/src/view/mixin/ImageMixin.type.tspackages/core/src/view/mixin/_graph-mixins-apply.tspackages/core/src/view/mixin/_graph-mixins-types.tspackages/core/src/view/plugin/ImageBundlePlugin.tspackages/core/src/view/plugin/index.tspackages/ts-example-without-defaults/vite.config.js
💤 Files with no reviewable changes (7)
- packages/core/src/view/mixin/_graph-mixins-types.ts
- packages/core/tests/serialization/codec/all-graph-classes.test.ts
- packages/core/tests/view/BaseGraph.test.ts
- packages/core/src/view/mixin/_graph-mixins-apply.ts
- packages/core/src/view/AbstractGraph.ts
- packages/core/src/view/mixin/ImageMixin.type.ts
- packages/core/src/view/mixin/ImageMixin.ts
- Fix typos, switch the code fence to typescript, and convert legacy <name>
doc links to {@link name} in the class-level JSDoc.
- Add a note flagging the fallback mechanism for potential removal in a future
release (legacy mxGraph feature for IE6/7).
- Make fallback optional on the ImageMap entry and on putImage() so callers no
longer need to supply one when there is no legacy-browser story.
The previous 'no bundle registered' test used a graph with the plugin registered but no bundles added, so it only exercised the getImageFromBundles()-returns-null branch. Split it into two tests so the optional-chain branch (plugin not registered on the graph) is also covered — this is the BaseGraph path that did not exist before the mixin-to-plugin migration, and a regression dropping the ?. would otherwise go undetected.
The helper was nested inside the 'isValidAncestor' describe but is a pure, reusable fixture — Sonar flags nested function declarations that don't need to live inside the block. Moving it to module scope matches the rest of the helpers in this file and keeps scope aligned with lifetime.
…uidance
Switch the CHANGELOG migration example and the ImageBundle JSDoc snippet from optional chaining to a non-null
assertion when registering a bundle (graph.getPlugin<ImageBundlePlugin>('image-bundle')!.addImageBundle(bundle)).
With '?.' a missing plugin silently drops the bundle registration, which is exactly the kind of silent regression
callers migrating from the previous 'graph.addImageBundle(bundle)' method want to catch — if they rely on image
bundles, the plugin must be registered and the call must fail fast otherwise. The '?.' form remains appropriate
for read-only accessors (e.g. imageBundles, getImageFromBundles), where 'undefined' is a meaningful 'no plugin,
no bundle matched' result already relied on by CellsMixin.postProcessCellStyle.
There was a problem hiding this comment.
🧹 Nitpick comments (2)
packages/core/src/view/image/ImageBundle.ts (1)
34-50: JSDoc example aligns with the new plugin API.The updated snippet correctly reflects the removal of
graph.addImageBundleand the move tograph.getPlugin<ImageBundlePlugin>('image-bundle')!.addImageBundle(bundle). The explicit note thatBaseGraphusers must registerImageBundlePluginviapluginsis useful given the opt-in behavior change.One optional nit: the example still references a free-standing
fallbackidentifier (lines 39 and 44) that is not declared in the snippet. Now thatfallbackis optional onputImage, you could simplify the example by omitting the third argument, which would also make the snippet copy-paste runnable.✏️ Optional doc tweak
- * 'laSSASFQpIV5HJBDyHpqK2ejVRm2AAgZCdmCGO9CIBADs=', fallback); + * 'laSSASFQpIV5HJBDyHpqK2ejVRm2AAgZCdmCGO9CIBADs='); @@ - * '<rect fill="url(`#gradient`)" width="100%" height="100%"/></svg>'), fallback); + * '<rect fill="url(`#gradient`)" width="100%" height="100%"/></svg>'));packages/core/__tests__/view/mixin/CellsMixin.test.ts (1)
315-325: Minor: helper could honor the now-optionalfallback.Since
ImageBundle.putImageno longer requires afallback,registerBundlecould passfallbackthrough as-is instead of substituting`${key}-fallback`. Not functionally wrong (the tests usealt=falseso the fallback is never read), but the current default masks the optional-parameter contract and slightly complicates reasoning if a future test wants to assert undefined fallback behavior. Also, the returnedImageBundleis currently unused by any caller and could be dropped for simplicity.✂️ Optional tightening
const registerBundle = ( graph: BaseGraph, entries: Array<{ key: string; value: string; fallback?: string }> - ): ImageBundle => { + ): void => { const bundle = new ImageBundle(); for (const { key, value, fallback } of entries) { - bundle.putImage(key, value, fallback ?? `${key}-fallback`); + bundle.putImage(key, value, fallback); } graph.getPlugin<ImageBundlePlugin>('image-bundle')!.addImageBundle(bundle); - return bundle; };
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: ecd01409-d238-45a1-99b3-773f756ca8de
📒 Files selected for processing (3)
CHANGELOG.mdpackages/core/__tests__/view/mixin/CellsMixin.test.tspackages/core/src/view/image/ImageBundle.ts
🚧 Files skipped from review as they are similar to previous changes (1)
- CHANGELOG.md
Add a Breaking Changes section describing the convention used in ae05746: `!` after the type/scope plus an uppercase `BREAKING CHANGES:` footer listing each break as a bullet. The prior standalone `!`-suffix line under Types is removed — the new section now carries the full rule, avoiding a duplicate mention. Also align the Body wrap guideline with the broader convention (120 columns) already used across the project.
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
.claude/rules/git/commit-conventions.md (1)
25-25: ChangeBREAKING CHANGES:toBREAKING CHANGE:to align with the official Conventional Commits specification.The Conventional Commits specification (conventionalcommits.org) uses the singular form
BREAKING CHANGE:in footer format, not plural. While projects can define custom conventions, adopting the standard form reduces confusion for contributors familiar with the specification. This affects lines 25 and 35.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 6555ec6d-48cf-41a5-b43d-25bc86306e94
📒 Files selected for processing (3)
.claude/rules/git/commit-conventions.mdpackages/html/stories/ImageBundle.stories.tspackages/website/docs/usage/image-bundles.md
✅ Files skipped from review due to trivial changes (1)
- packages/website/docs/usage/image-bundles.md
There was a problem hiding this comment.
Actionable comments posted: 1
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 49919225-e422-4575-aa9e-3e08bbbb6cf6
📒 Files selected for processing (3)
.claude/rules/git/commit-conventions.mdpackages/core/__tests__/view/plugin/ImageBundlePlugin.test.tspackages/core/src/view/plugin/ImageBundlePlugin.ts
🚧 Files skipped from review as they are similar to previous changes (2)
- packages/core/src/view/plugin/ImageBundlePlugin.ts
- packages/core/tests/view/plugin/ImageBundlePlugin.test.ts
There was a problem hiding this comment.
♻️ Duplicate comments (1)
.claude/rules/git/commit-conventions.md (1)
35-35:⚠️ Potential issue | 🟠 MajorUse the same breaking-change footer token in the example (
BREAKING CHANGE:).The rule text (Line 25) and example (Line 35) currently disagree; this can lead to commits using the wrong footer token for automated parsing.
In Conventional Commits v1.0.0, which footer token is valid for breaking changes: "BREAKING CHANGE:" or "BREAKING CHANGES:"?
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 32c64526-797b-40fb-b3c4-046fe3ec35ef
📒 Files selected for processing (1)
.claude/rules/git/commit-conventions.md
2 participants
Replace ImageMixin with a dedicated ImageBundlePlugin (id
'image-bundle'). Mixins share a single state tree across all Graph instances and pollute AbstractGraph with domain-specific methods; plugins are per-instance, opt-in on BaseGraph, and keep AbstractGraph lean.The plugin ships in
getDefaultPlugins()so Graph users see no behavioral change. BaseGraph consumers must now opt in explicitly by adding ImageBundlePlugin to thepluginsoption.CellsMixin.postProcessCellStyledelegates bundle key resolution to the plugin viagraph.getPlugin<ImageBundlePlugin>('image-bundle'); when the plugin is not registered the rawCellStateStyle.imageis used as-is, matching the pre-existing "no bundle matched" path.Also:
CellsMixin.postProcessCellStylecovering all 12 paths prior to the refactor so behavior preservation is verified, then merge them into the existingCellsMixin.test.tssuite, including an explicit plugin-absent case that guards the new optional-chain lookup.onDestroybundle clearing.packages/html/stories/ImageBundle.stories.ts) demonstrating bundle registration and usage.packages/website/docs/usage/image-bundles.mdcovering registration, BaseGraph opt-in, and the XML-serialization change..claude/rules/architecture/coding-practices.md.chunkSizeWarningLimitints-example-without-defaultsnow that the example no longer pulls the image-bundle code path into its bundle.CHANGELOG.md.Non-breaking API changes
ImageBundle.putImage()and theImageMapentry type now treatfallbackas optional. The legacy mxGraph IE6/7 fallback path is preserved but flagged for potential removal in a future release; callers without a legacy-browser story no longer need to supply one.BREAKING CHANGE:
AbstractGraph.addImageBundle,removeImageBundle,getImageFromBundlesand theimageBundlesproperty no longer exist onAbstractGraph,Graph, orBaseGraph. Migrate mutating call sites fromgraph.addImageBundle(bundle)(and siblings) tograph.getPlugin<ImageBundlePlugin>('image-bundle')!.addImageBundle(bundle). The non-null assertion is deliberate: if the plugin is not registered, the call must fail fast rather than silently drop the bundle registration. For read-only access,graph.getPlugin<ImageBundlePlugin>('image-bundle')?.imageBundlesis appropriate because returningundefinedwhen the plugin is absent matches the "no bundle matched" path already relied on byCellsMixin.postProcessCellStyle.BaseGraphno longer ships image-bundle support by default. AddImageBundlePluginto thepluginsoption to opt in.Graphcontinues to work unchanged becauseImageBundlePluginis part ofgetDefaultPlugins().<Graph>/<BaseGraph>no longer emits<Array as="imageBundles" />. Existing XML documents containing that element still decode without error but the field is silently ignored. Consumers that persist bundles as part of the graph XML must now serialize the plugin state separately.CellsMixin.postProcessCellStylesilently skips bundle key resolution whenImageBundlePluginis not registered — the rawCellStateStyle.imagevalue is used as the image path. This matches the pre-existing "no bundle matched" behavior and is only relevant forBaseGraphconsumers that do not add the plugin.Notes
Covers #762
Bundle size impact (tree-shaking)
Measured against
mainat6bc0f020ffa628f880701cc251f2e5cf9d0bb305:6bc0f020)Every example that does not bundle
ImageBundlePlugin(*-without-defaultsand*-selected-features) shrinks by roughly 0.3–0.5 kB, confirming that the image-bundle code path is now tree-shakable — which was not possible while it lived insideImageMixinonAbstractGraph. The full-featuredGraphexamples are essentially flat (within ±0.05 kB), as expected: the plugin still ships by default there, only the wiring layer changed.Out of scope (bundled)
.claude/rules/git/commit-conventions.md: documents the!+ uppercaseBREAKING CHANGE:footer convention actually used by this PR's main commit, and realigns body wrap to 120 columns. A split-out PR is not worth the overhead since the rule change is exactly what this PR demonstrates.🤖 Generated with Claude Code
Summary by CodeRabbit
Breaking Changes
New Features
Tests
Documentation
Stories