Skip to content

feat!: improve tree-shaking by isolating SelectionCellsHandler dependencies - #823

Merged
tbouffard merged 6 commits into
mainfrom
refactor/762-edgeHandler_factory_move_out_of_AbstractGraph
Aug 10, 2026
Merged

feat!: improve tree-shaking by isolating SelectionCellsHandler dependencies#823
tbouffard merged 6 commits into
mainfrom
refactor/762-edgeHandler_factory_move_out_of_AbstractGraph

Conversation

@tbouffard

@tbouffard tbouffard commented May 9, 2025

Copy link
Copy Markdown
Member

Move the cell handler creation logic out of AbstractGraph into the SelectionCellsHandler plugin, where it is actually used.

AbstractGraph referenced VertexHandler, EdgeHandler, ElbowEdgeHandler and EdgeSegmentHandler directly, so every application bundled them even when it never selected a cell. They are now reachable only through the plugin, so applications that do not register SelectionCellsHandler (visualization only, for example) no longer pay for them.

Configuring the cell handlers

Customizing a handler used to require subclassing the graph. The plugin now exposes factory setters, so the same customization is done by composition, on the plugin instance:

// Before
class MyGraph extends Graph {
  override createVertexHandler(state: CellState): VertexHandler {
    return new MyVertexHandler(state);
  }
}

// After
const selectionCellsHandler = graph.getPlugin<SelectionCellsHandler>('SelectionCellsHandler')!;
selectionCellsHandler.setVertexHandlerFactory((state) => new MyVertexHandler(state));
Former override on the graph New setter on SelectionCellsHandler
createVertexHandler setVertexHandlerFactory(factory)
createEdgeHandlerInstance setEdgeHandlerFactory('default', factory)
createElbowEdgeHandler setEdgeHandlerFactory('elbow', factory)
createEdgeSegmentHandler setEdgeHandlerFactory('segment', factory)
createEdgeHandler, when overridden to always return the same class setEdgeHandlerFactoryForAllKinds(factory)

The setters only affect handlers created after the call, so they must be called before the first selection occurs.

createHandler and createEdgeHandler remain as dispatch methods, on SelectionCellsHandler instead of AbstractGraph. Overriding the dispatch logic itself is still possible, by extending the plugin and passing the subclass in the plugins option.

Custom edge handlers by configuration only

Which EdgeHandler gets instantiated is driven by the handlerKind of the EdgeStyleMetaData registered in the EdgeStyleRegistry. Previously, only the three built-in kinds were served, and only by overriding the matching create* method. A custom kind can now be handled without subclassing anything:

EdgeStyleRegistry.add('myEdgeStyle', MyEdgeStyle, { handlerKind: 'my-kind' });
selectionCellsHandler.setEdgeHandlerFactory('my-kind', (state) => new MyEdgeHandler(state));

Edge styles whose kind has no registered factory fall back to the 'default' one, so a partial configuration stays functional.

Documentation

A new usage/cell-handlers.md page in the website documents what the vertex and edge handlers do, the global handler configuration objects, how the handler kind is chosen, and how to configure the factories. The edge-styles.md and plugins.md pages link to it.

Notes

Covers #762

Impact on the size of the examples

Example before now delta
js-example 468.70 kB 468.90 kB +0.20 kB
js-example-selected-features 386.38 kB 386.54 kB +0.16 kB
js-example-without-defaults 320.97 kB 240.03 kB -80.94 kB
ts-example 428.51 kB 428.65 kB +0.14 kB
ts-example-selected-features 361.38 kB 361.52 kB +0.14 kB
ts-example-without-defaults 299.46 kB 220.87 kB -78.59 kB

The now column comes from ./scripts/build-all-examples.bash run on this branch. The before column comes from #1126 for the TypeScript examples, which is the previous commit in main and recalibrated them for Vite 8, and from the v0.24.0 release notes for the JavaScript examples, which are unchanged since that release as they are built with webpack and were therefore not affected by the Vite 8 upgrade.

Only the without-defaults examples benefit, as they are the ones that do not register SelectionCellsHandler. The four other examples grow by 0.14 to 0.20 kB, which is the cost of the factory map and its setters now held by the plugin.

ts-example-without-defaults is the only example whose chunkSizeWarningLimit needs an update, from 300 to 221.

For reference, the first measurement in this PR, done on the 0.20.0 development version (commit 4bcc016), reported a 83.5 kB decrease, and the experiment in #449 had shown a potential decrease of 63-66 kB.

Additional measure: without subclasses of EdgeHandler

In the future (#890), we may provide a way to configure the EdgeHandler that are registered in the plugin to reduce the size of application that doesn't require them:

  • no children 7-8 kB
  • Elbow only: 5 kB
  • Segment only: 0 kB. Segment extends Elbow so it is imported as well.

Remaining Tasks

Summary by CodeRabbit

  • Breaking Changes

    • Cell-handler creation and customization now belong to the Selection Cells Handler plugin rather than graph subclasses.
    • Added configurable factories for custom vertex and edge handlers, including edge-style-specific and global options.
    • Added public handler factory types and updated handler creation behavior.
  • Documentation

    • Added comprehensive cell-handler configuration and migration guidance.
    • Documented edge-style handler selection and updated API usage guidance.
  • Tests

    • Expanded coverage for handler creation, customization, lifecycle, replacement, gestures, and cleanup.

@tbouffard tbouffard added the enhancement New feature or request label May 9, 2025
@coderabbitai

coderabbitai Bot commented May 9, 2025

Copy link
Copy Markdown

Review Change Stack

Walkthrough

The change moves cell-handler construction from graph classes to SelectionCellsHandler. It adds typed factory APIs, handler-kind dispatch, lifecycle tests, migrated stories, migration documentation, and repository guidance updates.

Changes

Handler factory migration

Layer / File(s) Summary
SelectionCellsHandler API and implementation
packages/core/src/types.ts, packages/core/src/view/AbstractGraph.ts, packages/core/src/view/plugin/SelectionCellsHandler.ts, packages/core/src/view/handler/*
SelectionCellsHandler now creates handlers, resolves edge handlerKind values, supports configurable factories, and preserves gesture state during handler replacement. Graph-level factory methods were removed.
Handler lifecycle validation
packages/core/__tests__/view/plugin/SelectionCellsHandler.test.ts, packages/core/__tests__/view/BaseGraph.test.ts
Tests cover default and custom factories, handler-kind precedence, selection lifecycle, destruction, updates, and gesture-state transfer.
Story customization migrations
packages/html/stories/*
Stories replace graph subclass handler overrides with SelectionCellsHandler factory registration or method customization.
Handler migration documentation
CHANGELOG.md, packages/website/docs/usage/cell-handlers.md, packages/website/docs/usage/edge-styles.md, packages/website/docs/usage/plugins.md
Release notes and usage documentation describe handler ownership, factory APIs, handler kinds, configuration, and migration steps.

Repository guidance updates

Layer / File(s) Summary
Guidance and build configuration
.claude/rules/architecture/*.md, CLAUDE.md, packages/ts-example-without-defaults/vite.config.js
Repository guidance adds isNullish and object-parameter API rules, CLAUDE.md links the API guidance, and the Vite chunk warning threshold changes to 221.

Estimated code review effort: 4 (Complex) | ~45 minutes

Sequence Diagram(s)

sequenceDiagram
  participant Graph
  participant SelectionCellsHandler
  participant EdgeStyleRegistry
  participant CellHandler
  Graph->>SelectionCellsHandler: refresh selected cell states
  SelectionCellsHandler->>EdgeStyleRegistry: resolve edge handler kind
  SelectionCellsHandler->>CellHandler: create configured handler
  SelectionCellsHandler->>SelectionCellsHandler: update or replace handler
Loading

Possibly related issues

Possibly related PRs

  • maxGraph/maxGraph#797 — Tests the graph-level handler APIs replaced by this migration.
  • maxGraph/maxGraph#819 — Relates directly to moving handler creation from AbstractGraph to SelectionCellsHandler.
  • maxGraph/maxGraph#849 — Migrates story customization from graph-level handler methods to plugin-based configuration.

Suggested labels: refactor

🚥 Pre-merge checks | ✅ 5
✅ Passed checks (5 passed)
Check name Status Explanation
Docstring Coverage ✅ Passed No functions found in the changed files to evaluate docstring coverage. Skipping docstring coverage check.
Linked Issues check ✅ Passed Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check ✅ Passed Check skipped because no linked issues were found for this pull request.
Title check ✅ Passed The title clearly summarizes the tree-shaking change and uses valid Conventional Commits breaking-change syntax.
Description check ✅ Passed The description explains the motivation, breaking API changes, migration, tests, documentation, issue reference, and measured bundle-size impact.

Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out.

❤️ Share

Comment @coderabbitai help to get the list of available commands.

Comment thread packages/core/src/view/plugin/SelectionCellsHandler.ts Outdated
Comment thread packages/core/src/types.ts Outdated
Comment thread packages/core/src/view/plugins/SelectionCellsHandler.ts Outdated
Comment thread packages/core/src/view/plugins/SelectionCellsHandler.ts Outdated
Comment thread packages/core/src/view/plugins/SelectionCellsHandler.ts Outdated
Comment thread packages/core/src/view/plugins/SelectionCellsHandler.ts Outdated
Comment thread packages/core/src/types.ts Outdated
Comment thread packages/html/stories/FixedPoints.stories.ts Outdated
Comment thread packages/core/src/view/plugins/SelectionCellsHandler.ts Outdated
Copilot AI lite review requested due to automatic review settings August 6, 2026 14:45
@redfish4ktc
redfish4ktc force-pushed the refactor/762-edgeHandler_factory_move_out_of_AbstractGraph branch from 4bcc016 to dc4d09a Compare August 6, 2026 14:45

Copilot AI left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Copilot was unable to review this pull request because the user who requested the review is ineligible. To be eligible to request a review, you need a paid Copilot license, or your organization must enable Copilot code review.

Move the cell handler factories out of AbstractGraph into the SelectionCellsHandler plugin, so that AbstractGraph no
longer references VertexHandler, EdgeHandler, ElbowEdgeHandler and EdgeSegmentHandler. Applications that do not
register the plugin, typically read-only or visualization-only ones built on BaseGraph, no longer bundle these
classes: the ts-example-without-defaults bundle drops from ~300 kB to 221 kB.

Customizing the handlers no longer requires subclassing the graph. The plugin exposes factory setters instead, which
are per instance rather than global, and which also accept the custom handler kinds registered in EdgeStyleRegistry,
something the former factory methods could not express.

Document the whole area: a new cell-handlers page on the website covering what the handlers provide, the three
global configuration objects, the mapping from handlerKind to handler and the factory setters; a handlerKind entry
in the EdgeStyleMetaData warning of the edge styles page; and consistent class comments on the four handler classes,
two of which were incomplete or pointed at the wrong configuration object.

BREAKING CHANGE:
- The cell handler factory methods no longer exist on AbstractGraph, Graph or BaseGraph: createHandler,
  createEdgeHandler, createEdgeHandlerInstance, createElbowEdgeHandler, createEdgeSegmentHandler and
  createVertexHandler. Retrieve the plugin with
  graph.getPlugin<SelectionCellsHandler>('SelectionCellsHandler') and use its setters instead:
  setVertexHandlerFactory(factory) replaces an override of createVertexHandler, and
  setEdgeHandlerFactory(handlerKind, factory) replaces an override of createEdgeHandlerInstance (kind 'default'),
  createElbowEdgeHandler (kind 'elbow') and createEdgeSegmentHandler (kind 'segment').
  setEdgeHandlerFactoryForAllKinds(factory) sets a single factory for every kind.
- The dispatch methods createHandler and createEdgeHandler are now defined on SelectionCellsHandler. If you were
  overriding them to change the dispatch logic itself, extend the plugin and pass your subclass in the plugins option.
- SelectionCellsHandler.createHandler returns a non-nullable CellHandler, whereas AbstractGraph.createHandler was
  typed as nullable. TypeScript users can drop the now-useless null checks on the returned value.
@redfish4ktc
redfish4ktc force-pushed the refactor/762-edgeHandler_factory_move_out_of_AbstractGraph branch from dc4d09a to b6afa3e Compare August 8, 2026 23:56
@tbouffard
tbouffard marked this pull request as ready for review August 10, 2026 04:47

@coderabbitai coderabbitai Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 3

🧹 Nitpick comments (5)
packages/core/src/view/plugin/SelectionCellsHandler.ts (2)

269-276: 🩺 Stability & Availability | 🔵 Trivial | 💤 Low value

Confirm the 'default' entry cannot be removed.

The fallback uses a non-null assertion on the 'default' factory. setEdgeHandlerFactory and setEdgeHandlerFactoryForAllKinds always keep or set that key, so the assertion holds for the current public API. If a future API allows removing a kind, this assertion becomes a runtime hazard. Consider a local constant for the default factory instead of re-reading the map.


357-378: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low value

Move the map deletion after the nullish check.

updateHandler deletes the entry before it checks whether a handler exists. The delete is a no-op for an absent key, so behavior is correct. Moving the delete after the guard makes the flow clearer and keeps the map untouched on the early-return path.

♻️ Proposed reorder
   updateHandler(state: CellState): void {
     const handler = this.handlers.get(state.cell);
-    this.handlers.delete(state.cell);
-
     if (isNullish(handler)) {
       return;
     }
+    this.handlers.delete(state.cell);
packages/core/src/view/handler/ElbowEdgeHandler.ts (1)

30-50: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low value

Reword the copied "which the built-in ... are" sentence in both handler doc blocks. Both files repeat the same sentence template to list the edge styles bound to their handler kind. The construction is hard to read and differs from the clearer "which covers ..." form already used in EdgeHandler.ts.

  • packages/core/src/view/handler/ElbowEdgeHandler.ts#L30-L50: change "which the built-in elbowEdgeStyle, loopEdgeStyle, sideToSideEdgeStyle and topToBottomEdgeStyle are" to "which covers the built-in elbowEdgeStyle, loopEdgeStyle, sideToSideEdgeStyle and topToBottomEdgeStyle".
  • packages/core/src/view/handler/EdgeSegmentHandler.ts#L29-L51: change "which the built-in manhattanEdgeStyle, orthogonalEdgeStyle and segmentEdgeStyle are" to "which covers the built-in manhattanEdgeStyle, orthogonalEdgeStyle and segmentEdgeStyle".
packages/core/__tests__/view/plugin/SelectionCellsHandler.test.ts (2)

290-309: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low value

Replace the && side-effect statement with an if block.

Line 295-296 uses handlerKind != 'default' && EdgeStyleRegistry.add(...) as a statement. Lint rules such as no-unused-expressions commonly reject this form, and an if block states the intent directly. Also prefer !== over !=.

♻️ Proposed refactor
-          handlerKind != 'default' && // when not registered, it will use 'default'
-            EdgeStyleRegistry.add('custom', edgeStyle, { handlerKind });
+          if (handlerKind !== 'default') {
+            // when not registered, the 'default' factory is used
+            EdgeStyleRegistry.add('custom', edgeStyle, { handlerKind });
+          }

208-214: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low value

Restore the edge-style registry state for the whole file, not only this block.

unregisterAllEdgeStyles() runs in beforeEach and afterAll for the createEdgeHandler block. The earlier createHandler block does not reset the registry, so it depends on whatever the module import left registered. Adding a top-level beforeEach reset removes that ordering dependency.


ℹ️ Review info
⚙️ Run configuration

Configuration used: Organization UI

Review profile: CHILL

Plan: Pro Plus

Run ID: bfc30660-3adb-4b89-859f-567b11b9904c

📥 Commits

Reviewing files that changed from the base of the PR and between d19ad65 and ddea1d4.

📒 Files selected for processing (27)
  • .claude/rules/architecture/coding-practices.md
  • .claude/rules/architecture/graph-api-usage.md
  • CHANGELOG.md
  • CLAUDE.md
  • TODO.md
  • packages/core/__tests__/view/BaseGraph.test.ts
  • packages/core/__tests__/view/plugin/SelectionCellsHandler.test.ts
  • packages/core/src/types.ts
  • packages/core/src/view/AbstractGraph.ts
  • packages/core/src/view/handler/EdgeHandler.ts
  • packages/core/src/view/handler/EdgeSegmentHandler.ts
  • packages/core/src/view/handler/ElbowEdgeHandler.ts
  • packages/core/src/view/handler/VertexHandler.ts
  • packages/core/src/view/plugin/SelectionCellsHandler.ts
  • packages/html/stories/AutoLayout.stories.ts
  • packages/html/stories/ContextIcons.stories.ts
  • packages/html/stories/DragSource.stories.ts
  • packages/html/stories/FixedPoints.stories.ts
  • packages/html/stories/Handles.stories.ts
  • packages/html/stories/Orthogonal.stories.ts
  • packages/html/stories/PortRefs.stories.ts
  • packages/html/stories/Stencils.stories.ts
  • packages/html/stories/Wires.stories.ts
  • packages/ts-example-without-defaults/vite.config.js
  • packages/website/docs/usage/cell-handlers.md
  • packages/website/docs/usage/edge-styles.md
  • packages/website/docs/usage/plugins.md
💤 Files with no reviewable changes (1)
  • packages/core/tests/view/BaseGraph.test.ts

Comment thread packages/core/__tests__/view/plugin/SelectionCellsHandler.test.ts
Comment thread packages/core/src/view/plugin/SelectionCellsHandler.ts
Comment thread packages/website/docs/usage/cell-handlers.md
@maxGraph maxGraph deleted a comment from sonarqubecloud Bot Aug 10, 2026
@maxGraph maxGraph deleted a comment from sonarqubecloud Bot Aug 10, 2026
…s edges

The former graph.createEdgeHandler override ignored the edge style and returned CustomElbowEdgeHandler for every edge.
Migrating it to setEdgeHandlerFactory('elbow', ...) narrowed it: the two edges inserted in the model do use
'elbowEdgeStyle', but MyCustomConnectionHandler.createEdgeState builds the preview edge with 'orthogonalEdgeStyle',
and ConnectionHandler.connect copies that style onto the inserted edge. Interactively drawn edges therefore resolve to
the 'segment' handler kind and got the built-in EdgeSegmentHandler, losing MyCustomConstraintHandler (snap to fixed
points) and the isConnectableCell override (no floating connections), which are the very features this story
demonstrates.

Use setEdgeHandlerFactoryForAllKinds instead, which restores the previous behavior, and replace the misleading
"all edges use the elbow edge style" comment by one explaining why the per-kind form does not fit here.
@sonarqubecloud

Copy link
Copy Markdown

@tbouffard
tbouffard merged commit adcb78e into main Aug 10, 2026
14 checks passed
@tbouffard
tbouffard deleted the refactor/762-edgeHandler_factory_move_out_of_AbstractGraph branch August 10, 2026 10:20
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

enhancement New feature or request

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants