docs: add ADRs on mixins, plugins and moving members out of AbstractGraph - #1130
Draft
redfish4ktc wants to merge 10 commits into
Draft
docs: add ADRs on mixins, plugins and moving members out of AbstractGraph#1130redfish4ktc wants to merge 10 commits into
redfish4ktc wants to merge 10 commits into
Conversation
WalkthroughAdded an Architecture Decision Records section to the README and added ADR documentation for Graph mixins, plugins, and AbstractGraph member extraction. ChangesArchitecture Decision Records
Estimated code review effort: 2 (Simple) | ~10 minutes Possibly related PRs
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
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 |
…raph AbstractGraph is 1337 lines and still holds three kinds of members: those that genuinely belong to the class, those that belong to an existing mixin and were never moved, and those forming coherent groups with no home yet. Deciding where each one goes requires knowing why mixins exist, why plugins replace them, and which constraints block a given move. None of that is recoverable from the code, so record it rather than rediscover it at every refactoring. Establish docs/adr/ with its conventions, since the repository had no architecture decision record directory, and link it from the root README so contributors find it before proposing a change to the Graph class hierarchy. ADR 0001 documents the mixin approach retrospectively, and above all what it costs: mutable defaults become shared state, nothing is tree-shaken since all 21 mixins are applied unconditionally, and mixInto leaving enumerable at false means mixin properties are not serialized by Codecs while class properties are. Evidence that the API was preserved is negative: "mixin" does not appear once in the mxGraph migration guide. ADR 0002 documents plugins as the extension unit, for two purposes weighted equally: the destination for code leaving AbstractGraph and the mixins, and the home for new features, which is what stops the class from growing back. It records discussion #51 as the first mention of the system and Apache ECharts as its inspiration. ADR 0003 applies both to the current content of AbstractGraph, analysed on commit 5c8cf90 during the development of 0.25.0. Seven decisions, an inventory of every member with its target, and a six pull request plan bringing the class to roughly 800 lines with no breaking change. Groups already covered by issue #762 are deliberately left untouched.
redfish4ktc
force-pushed
the
docs/analyze_move_members_out_of_AbstractGraph
branch
from
August 10, 2026 05:29
72e420f to
3cf3fda
Compare
tbouffard
marked this pull request as ready for review
August 10, 2026 05:31
There was a problem hiding this comment.
Actionable comments posted: 3
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro Plus
Run ID: 3491f215-38e1-4500-8b23-519db04ab4ce
📒 Files selected for processing (5)
README.mddocs/adr/0001-use-mixins-to-split-the-graph-class.mddocs/adr/0002-use-plugins-for-optional-and-new-features.mddocs/adr/0003-move-members-out-of-abstract-graph.mddocs/adr/README.md
This was referenced Aug 10, 2026
Step 6 tied the four blocked properties to a mixin-to-plugin conversion of their host, which turned a documented constraint into planned work of a completely different nature: a breaking change of a whole feature API, not a member relocation. That is not the intent of this plan. Drop step 6. pageFormat, warningImage, multiplicities and alternateEdgeStyle stay in AbstractGraph, and step 1 already does everything that can be done for them, grouping them where the constraint is visible and pinning the per-instance guarantee with a test. Appendix A keeps recording their target mixin, since the cohesion argument stays valid for the day the constraint disappears, but the rows are now explicitly an inventory rather than a backlog.
The ADR index and ADR 0001 carried "Accepted, superseded in direction by 0002", a value the conventions section of the ADR README does not define. Reported by CodeRabbit on #1130. The wording was also wrong on substance. ADR 0001 is not superseded: plugins replace mixins for new code, but the mixin design still explains most of the current codebase and its recorded costs are precisely what justify the shift. Both places now read Accepted, and the relationship with ADR 0002 is stated in prose instead of being smuggled into a status field. Add the rule that caused the slip to the conventions, so the distinction is explicit: Superseded by ADR-NNNN means the decision no longer holds, while an approach merely no longer applied to new code stays Accepted.
The column headed "Target mixin" named a destination for every row, including the ones D3 blocks, so the table read as a backlog and contradicted the decision it sits under. Follow-up to the CodeRabbit review on #1130. The four affected rows now say what happens rather than what would be ideal: multiplicities and alternateEdgeStyle stay in AbstractGraph with their natural home recorded as context, and the two mixed rows spell out which part moves and which part stays, since only getWarningImage and the page getters were relocated while warningImage and pageFormat remain. Add a sentence before the table so the convention is explicit rather than inferred from each row.
Add an implementation feedback section to ADR 0003 and fold its two findings back into the sections that should have carried them from the start. Two consequences of a mixin move were not anticipated. Declaration order in AbstractGraph is observable through the Codec, so regrouping two properties changed the exported XML element order, in a step the analysis described as having no effect at all. And an accessor written as an arrow function property loses its binding when it becomes a prototype method, which is the only user-visible break of the whole plan and the one thing that needed a changelog entry. The ADR claimed flatly that mixin moves are not breaking, which holds for the type surface but not for every runtime behaviour. Also record the numbers, since the estimates are only worth anything if their accuracy is checked: 811 lines against roughly 800 predicted, 24 files against about 25, and per-step line counts within a few percent. The total churn came out 4% above the upper bound, and one pull request exceeded the review ceiling because it also carried a rename. Finally, promote a signal discovered while working: an import becoming unused in AbstractGraph is the cheapest evidence that a concern left wholly, and the compiler does not report it.
The section still announced three technical facts after the serialization one was added, and now also says which came from the analysis and which from the implementation.
There was a problem hiding this comment.
Actionable comments posted: 1
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro Plus
Run ID: d57e4b35-1e35-48d2-a8f9-681122309986
📒 Files selected for processing (1)
docs/adr/0003-move-members-out-of-abstract-graph.md
redfish4ktc
marked this pull request as draft
August 11, 2026 14:20
…able The pull request table declared every relocation pull request non-breaking, while the retrospective section reports that six arrow-function accessors became prototype methods, breaking detached references and requiring a changelog entry under Breaking Changes. A single "Breaking" column can only be read as source compatibility, so the two pull requests that carry the runtime break looked safe. Split the column in two, mark the two affected pull requests, and note which changelog entry covers them, so a reader cannot take "not breaking" as covering behaviour as well as the type surface.
Apache ECharts was not a source of inspiration for the plugin system. The claim came from a passing analogy in the discussion that first proposed the system, which is not the same as the design having been drawn from it, so the ADR overstated it in both the Related list and the Context. Replace it with what that discussion actually establishes: a configurable plugin system was named as the eventual replacement for the namespace split then being used to make tree-shaking possible.
tbouffard
reviewed
Aug 18, 2026
…bers_out_of_AbstractGraph
Review feedback asked for no figures on the two bullets that counted the mixins. A count baked into ADR prose drifts on every mixin added, merged or converted to a plugin, and it is not what the reasoning rests on: the arguments hold for one mixin or thirty. Drop the counted mentions of the mixins across the three ADRs, including the file count of the assembled AbstractGraph interface, and drop the class size the growing-class bullet quoted in passing. The mxGraph size stays, since it points at a released tag and cannot drift, and so do the measurements of ADR 0003, which are pinned to its analysis commit and are the outcome it records.
|
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
2 participants
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.
Why
AbstractGraphwas 1337 lines. It inherits the God-object design ofmxGraphand held three kinds of members: those that genuinely belong to the class, those that belong to an existing mixin and were never moved, and those forming coherent groups with no home yet.Deciding where each one goes requires knowing why mixins exist, why plugins replace them, and which constraints block a given move. None of that is recoverable from the code, so this PR records it rather than have it rediscovered at every refactoring.
Documentation only, no source change. The analysis was performed on commit 5c8cf90, during the development of version 0.25.0.
What
Establishes
docs/adr/and its conventions, since the repository had no architecture decision record directory, and links it from the root README so contributors find it before proposing a change to theGraphclass hierarchy.AbstractGraph0001 and 0002 record the two structural approaches and why the second replaces the first. 0003 applies them to the members still sitting in
AbstractGraph.ADR 0001 is written retrospectively from the code and from the discussions referenced in
view/mixin/_README.md. It records the four consequences that motivated the shift to plugins: mutable defaults becoming shared state, the absence of tree-shaking since every mixin is applied unconditionally, the serialization asymmetry caused bymixIntoleavingenumerableatfalse, and the navigation cost of assembling one interface from the class plus one type file per mixin.ADR 0002 records plugins as the extension unit, for two purposes weighted equally: the destination for code leaving
AbstractGraphand the mixins, and the home for new features, which is what stops the class from growing back. Discussion #51 is the first mention of the system, where a configurable plugin system is named as the eventual replacement for the namespace split then used to make tree-shaking possible.Decisions recorded in ADR 0003
AbstractGraph, grouped in the dedicated block, with a regression test eachsizeDidChangestays inAbstractGraph, and the reason gets an in-body commentPageBreaksMixinis renamed toPageMixinsetTooltipsis not deprecatedFour facts that drove most of the decisions
applyGraphMixinsruns unconditionally and each.type.tsaugments theAbstractGraphinterface through declaration merging, so the public API is unchanged. Behaviour is a separate question: an accessor written as an arrow function property becomes a prototype method, which breaks detached references at runtime while still compiling. The ADR records both, and the pull request table in it separates source from runtime breakage.mixIntocopies members onto the prototype, so an object or array default becomes shared state across every graph instance. This is silent: no compile error, and no failing test until D3 lands.AbstractGraphis observable output. The Codec walks the own fields in declaration order, so relocating or reordering a property changes the child element order of the XML produced byexportObject. Decoding is unaffected, elements are matched by theirasattribute, but a consumer comparing exported XML as text sees a diff. Found while implementing, not during the analysis.Implementation
The plan is five steps, all implemented as a stack of pull requests. All of them are source-compatible. Two, #1132 and #1134, break behaviour at runtime: they turn six arrow function accessors into prototype methods, so a detached reference such as
const f = graph.isIgnoreScrollbarsnow needs an explicitbind. That break carries a changelog entry under Breaking Changes.pageFormat/warningImage, and thesizeDidChangecommentPanningMixinCellsMixin+ValidationMixin+OverlaysMixinbatchLabelMixin+OrderMixinbatch, plus the page cluster and thePageMixinrenameGroupingMixinResult:
AbstractGraph.tsgoes from 1337 to 811 lines, against the roughly 800 the ADR projected. Every step passed the full local CI. Four of the five relocation pull requests modified no test at all, which is the signal the ADR asks for on a pure relocation. The exception is #1131, which adds the regression tests and had to updateall-graph-classes.test.tsfor the serialization order noted above.What remains in the class matches Appendix C of ADR 0003: the bootstrap contract, the collaborators, the plugin registry, the model-to-view dispatch, the lifecycle, and the properties held back by the shared-state constraint.
Those last four,
pageFormat,warningImage,multiplicitiesandalternateEdgeStyle, stay inAbstractGraphand this plan does not move them. Step 1 already does everything possible for them: they sit together where the constraint is visible at the declaration, and a test pins the per-instance guarantee. Appendix A still records their target mixin, as an inventory rather than a backlog.Related
Issue #762 is the umbrella for moving mixin code to plugins. It also covers the handler factories,
defaultLoopStyleandisOrthogonal, which this plan deliberately avoids touching.Summary by CodeRabbit