refactor(graph): move the drill-down members to GroupingMixin - #1136
Conversation
|
Important Review skippedAuto reviews are disabled on base/target branches other than the default branch. 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 |
aed89d8 to
1a9ede4
Compare
GroupingMixin already owns enterGroup and exitGroup, the two entry points of drill-down navigation, and it already had to declare isValidRoot and getCurrentRoot as external dependencies to implement them. Move the members those methods depend on: resetViewOnRootChange, getCurrentRoot, getTranslateForRoot, getChildOffsetForCell, home and isValidRoot. AbstractGraph keeps calling home and resetViewOnRootChange from processChange, and getCurrentRoot from getDefaultParent, which declaration merging supports without any change at the call sites. defaultParent, getDefaultParent and setDefaultParent stay deliberately. They are the most used entry point of the whole API and are read from processChange, so the cohesion gain does not justify the churn. Worth recording for the eventual conversion of this mixin to a plugin: drill-down is view navigation while grouping is model restructuring. They are two concerns sharing a file because no better home exists today, so this move should not be read as a statement that they belong together. AbstractGraph loses 89 lines and its last use of Point, and now stands at 811 lines, down from 1337 at the start of the plan.
1a9ede4 to
fe49375
Compare
|
Note
Last PR of the stack: #1131 → #1132 → #1134 → #1135 → this one. The base is
refactor/move_label_order_page_members_to_mixins, so the diff shown here contains only this step.Fifth and final implementation step of the plan recorded in ADR 0003 (#1130). No behavior change, and the public API is unchanged.
Changes
Moved to
GroupingMixin:resetViewOnRootChange,getCurrentRoot,getTranslateForRoot,getChildOffsetForCell,homeandisValidRoot.GroupingMixinalready ownsenterGroupandexitGroup, the two entry points of drill-down navigation, and it already had to declareisValidRootandgetCurrentRootas external dependencies in order to implement them. Both now disappear from its dependency list.AbstractGraphkeeps callinghomeandresetViewOnRootChangefromprocessChange, andgetCurrentRootfromgetDefaultParent. Declaration merging supports this without any change at the call sites.AbstractGraph.tsloses 89 lines and its last use ofPoint.What stays behind, deliberately
defaultParent,getDefaultParentandsetDefaultParentare not moved, although they belong to the same topic. They are the most used entry point of the whole API and are read fromprocessChange, so the cohesion gain does not justify the churn. This is recorded in Appendix B.2 of the ADR.A caveat worth recording
Drill-down is view navigation, while grouping is model restructuring. They are two concerns sharing a file because no better home exists under the current rules, not because they belong together.
When
GroupingMixinis converted to a plugin, the split should be reconsidered rather than carried over. The commit message records this so it does not get lost.Result of the whole plan
AbstractGraph.tsgoes from 1337 to 811 lines across the five PRs of the stack, with no breaking change other than the accessor binding note already in the changelog.What remains in the class is what Appendix C of the ADR says should remain: the bootstrap contract, the collaborators, the plugin registry, the model-to-view dispatch, the lifecycle, and the properties parked by the shared-state constraint until their host mixin becomes a plugin.
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.