refactor(graph): move the cell, validation and overlay members to their mixins - #1134
Draft
redfish4ktc wants to merge 1 commit into
Conversation
|
Important Review skippedDraft detected. Please check the settings in the CodeRabbit UI or the ⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Plus Run ID: You can disable this status message by setting the Use the checkbox below for a quick retry:
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 |
redfish4ktc
force-pushed
the
refactor/move_cells_validation_overlays_members_to_mixins
branch
from
August 11, 2026 13:31
905582d to
0aeaa4f
Compare
tbouffard
marked this pull request as draft
August 12, 2026 04:20
…ir mixins Continue emptying AbstractGraph of members whose only consumers live in an existing mixin. Each move puts a member next to the code that already uses it, and turns the mixin dependency lists into an accurate description of what each concern actually needs: CellsMixin stops declaring getOverlap, isRecursiveResize, isExportEnabled and isImportEnabled as external dependencies, and ValidationMixin stops declaring isAllowLoops, isMultigraph and the two resource getters. To CellsMixin, whose resizeCells, canExportCell, canImportCell and constrainChild are the only internal callers: defaultOverlap, getOverlap, isAllowOverlapParent, exportEnabled, isExportEnabled, importEnabled, isImportEnabled, recursiveResize, isRecursiveResize and setRecursiveResize. To ValidationMixin, whose getEdgeValidationError and validateCell are the only internal callers: multigraph, isMultigraph, setMultigraph, allowLoops, isAllowLoops, setAllowLoops, and the alreadyConnectedResource and containsValidationErrorsResource keys with their getters. To OverlaysMixin, whose setCellWarning is the only consumer: getWarningImage. The warningImage property itself stays in AbstractGraph, in the per-instance group, because its default is a mutable Image that would be shared across every graph instance if it were installed on the prototype. The getter and its property are therefore split on purpose, until the mixin becomes a plugin and the constraint disappears. AbstractGraph loses 143 lines and its last use of isI18nEnabled. The public API is unchanged, mixin members are merged into the AbstractGraph interface by declaration merging. Four accessors were arrow function properties and become prototype methods, so detached references now need an explicit bind. The changelog entry added for the scrollbar getters is extended to cover them.
tbouffard
force-pushed
the
refactor/move_cells_validation_overlays_members_to_mixins
branch
from
August 12, 2026 04:50
0aeaa4f to
a8714bc
Compare
|
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Note
Stacked on top of #1132, which is itself stacked on #1131. The base of this PR is
refactor/move_viewport_translation_to_panning_mixin, so the diff shown here contains only this step.Third implementation step of the plan recorded in ADR 0003 (#1130). No behavior change, and the public API is unchanged.
Problem
AbstractGraphholds members whose only consumers live in an existing mixin. Each of them forces that mixin to declare the member as an external dependency in itsPick<AbstractGraph, ...>list, which makes the list describe the class rather than the concern.Changes
CellsMixindefaultOverlap,getOverlap,isAllowOverlapParent,exportEnabled,isExportEnabled,importEnabled,isImportEnabled,recursiveResize,isRecursiveResize,setRecursiveResizeresizeCells,canExportCell,canImportCell,constrainChildValidationMixinmultigraph,isMultigraph,setMultigraph,allowLoops,isAllowLoops,setAllowLoops,alreadyConnectedResource,containsValidationErrorsResourceand their gettersgetEdgeValidationError,validateCellOverlaysMixingetWarningImagesetCellWarningThe dependency lists shrink accordingly:
CellsMixinstops declaringgetOverlap,isRecursiveResize,isExportEnabledandisImportEnabledas external, andValidationMixinstops declaringisAllowLoops,isMultigraphand the two resource getters.AbstractGraph.tsloses 143 lines and its last use ofisI18nEnabled.One deliberate asymmetry
getWarningImagemoves toOverlaysMixinbutwarningImagestays inAbstractGraph, in the per-instance group introduced in #1131. Its default is a mutableImage, so installing it on the prototype would share one instance across every graph, which is the failure mode the tests in #1131 now guard against.The getter and its property are therefore split on purpose. They will be reunited when
OverlaysMixinbecomes a plugin, since plugins are instantiated per graph and the constraint disappears. This is step 6 of the ADR plan.API note
Four of the moved accessors were arrow function properties and become prototype methods:
isExportEnabled,isImportEnabled,getAlreadyConnectedResourceandgetContainsValidationErrorsResource.Calling them on the graph is unaffected. Only detached references break, and TypeScript will not report it since both forms share the same type. The changelog entry introduced in #1132 for the scrollbar getters is extended to cover these four.
Validation
Run locally on Node 24 (
.nvmrc):build,test-check, the full suite (505 tests, 60 suites),lint,check:circular-dependencies. All passing, and no test needed changing, which is the expected signal for a pure relocation.