refactor!: move tooltip methods to TooltipHandler #640
Conversation
WalkthroughThis PR moves tooltip resolution from the graph mixin into the TooltipHandler plugin, adds AbstractGraph.setTooltips(enabled), removes TooltipMixin and its type augmentations, updates tests, and migrates story files to override tooltip methods on the TooltipHandler plugin. Changes
Sequence Diagram(s)mermaid Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20–30 minutes
Possibly related issues
Possibly related PRs
Pre-merge checks and finishing touches✅ Passed checks (3 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
📜 Recent review detailsConfiguration used: Organization UI Review profile: CHILL Plan: Pro 📒 Files selected for processing (3)
🚧 Files skipped from review as they are similar to previous changes (1)
🧰 Additional context used📓 Path-based instructions (2)**/*.{ts,tsx,js,jsx,json,md}📄 CodeRabbit inference engine (.github/copilot-instructions.md)
Files:
**/*.{ts,tsx,js,jsx}📄 CodeRabbit inference engine (.github/copilot-instructions.md)
Files:
🧠 Learnings (3)📓 Common learnings📚 Learning: 2025-01-28T16:22:25.804ZApplied to files:
📚 Learning: 2025-05-26T12:34:54.318ZApplied to files:
🧬 Code graph analysis (1)packages/html/stories/UserObject.stories.ts (2)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (3)
🔇 Additional comments (2)
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 |
fee8b53 to
da69908
Compare
Move `getTooltip` and `getTooltipForCell` from `AbstractGraph` to the `TooltipHandler` plugin as this is the only place where they are used. This improves modularity and tree-shaking, clarifies responsibilities, and reduces the amount of code in `AbstractGraph` by removing the `TooltipMixin`. The stories have been updated accordingly and can be used as examples for adapting application code. BREAKING CHANGES: The `getTooltip` and `getTooltipForCell` methods have been moved from `AbstractGraph` to the `TooltipHandler` plugin. If you were overriding these methods in a `AbstractGraph` subclass, you should now extend `TooltipHandler` instead.
b211e94 to
86457f7
Compare
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (1)
packages/core/src/view/plugins/TooltipHandler.ts (1)
362-369: Consider usingArray.prototype.some()for early exit.The
LATERcomment indicates awareness of this, but usingsome()would allow breaking early once a tooltip is found. Given the PR focus on refactoring, this can be deferred.- if (!tip && state.overlays) { - state.overlays.forEach((shape: Shape) => { - // LATER: Exit loop if tip is not null - if (!tip && (node === shape.node || node.parentNode === shape.node)) { - tip = shape.overlay ? (shape.overlay.toString() ?? null) : null; - } - }); - } + if (!tip && state.overlays) { + state.overlays.some((shape: Shape) => { + if (node === shape.node || node.parentNode === shape.node) { + tip = shape.overlay ? (shape.overlay.toString() ?? null) : null; + return tip !== null; // exit early if found + } + return false; + }); + }
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (15)
CHANGELOG.md(2 hunks)packages/core/__tests__/view/Graph.test.ts(1 hunks)packages/core/__tests__/view/handler/TooltipHandler.test.ts(1 hunks)packages/core/src/view/AbstractGraph.ts(2 hunks)packages/core/src/view/mixins/TooltipMixin.ts(0 hunks)packages/core/src/view/mixins/TooltipMixin.type.ts(0 hunks)packages/core/src/view/mixins/_graph-mixins-apply.ts(0 hunks)packages/core/src/view/mixins/_graph-mixins-types.ts(0 hunks)packages/core/src/view/plugins/TooltipHandler.ts(4 hunks)packages/html/stories/Events.stories.ts(2 hunks)packages/html/stories/HelloPort.stories.ts(2 hunks)packages/html/stories/Stencils.stories.ts(2 hunks)packages/html/stories/Stylesheet.stories.ts(2 hunks)packages/html/stories/UserObject.stories.ts(2 hunks)packages/html/stories/Wires.stories.ts(2 hunks)
💤 Files with no reviewable changes (4)
- packages/core/src/view/mixins/_graph-mixins-apply.ts
- packages/core/src/view/mixins/TooltipMixin.type.ts
- packages/core/src/view/mixins/TooltipMixin.ts
- packages/core/src/view/mixins/_graph-mixins-types.ts
🧰 Additional context used
📓 Path-based instructions (7)
**/*.{ts,tsx,js,jsx,json,md}
📄 CodeRabbit inference engine (.github/copilot-instructions.md)
Format code using Prettier with tab width 2, trailing comma ES5, print width 90, and end of line auto.
Files:
CHANGELOG.mdpackages/core/src/view/AbstractGraph.tspackages/html/stories/Stylesheet.stories.tspackages/html/stories/HelloPort.stories.tspackages/core/__tests__/view/handler/TooltipHandler.test.tspackages/html/stories/Stencils.stories.tspackages/html/stories/Wires.stories.tspackages/core/src/view/plugins/TooltipHandler.tspackages/html/stories/UserObject.stories.tspackages/html/stories/Events.stories.tspackages/core/__tests__/view/Graph.test.ts
packages/core/src/**/*.ts
📄 CodeRabbit inference engine (.github/copilot-instructions.md)
packages/core/src/**/*.ts: Include.jsfile extensions in all import statements within packages/core/src/ to comply with ESLint rulen/file-extension-in-import.
Do not use console statements in any code. Use proper logging mechanisms instead (ESLint rule:no-console: error).
Do not useeval()function in any code (ESLint rule:no-eval: error).
Do not use const enums. Convert all const enums to regular enums (ESLint rule forbids const enums).
TypeScript source files in core package must strictly maintain typing with no implicit types and all functions having explicit return type annotations.
Files:
packages/core/src/view/AbstractGraph.tspackages/core/src/view/plugins/TooltipHandler.ts
**/*.{ts,tsx,js,jsx}
📄 CodeRabbit inference engine (.github/copilot-instructions.md)
Use single quotes in TypeScript/JavaScript code (Prettier config:
singleQuote: true).
**/*.{ts,tsx,js,jsx}: UseisNullishfunction when checking fornullorundefinedon variables that can have falsy values (numbers, strings, booleans) to avoid treating0,"", orfalseas nullish.
Wrap multiple model changes inbatchUpdate()to ensure efficient batch processing.
Files:
packages/core/src/view/AbstractGraph.tspackages/html/stories/Stylesheet.stories.tspackages/html/stories/HelloPort.stories.tspackages/core/__tests__/view/handler/TooltipHandler.test.tspackages/html/stories/Stencils.stories.tspackages/html/stories/Wires.stories.tspackages/core/src/view/plugins/TooltipHandler.tspackages/html/stories/UserObject.stories.tspackages/html/stories/Events.stories.tspackages/core/__tests__/view/Graph.test.ts
packages/core/src/**/*.{ts,tsx}
📄 CodeRabbit inference engine (CLAUDE.md)
Define styles as objects conforming to
CellStyletype as documented inpackages/core/src/types.ts.
Files:
packages/core/src/view/AbstractGraph.tspackages/core/src/view/plugins/TooltipHandler.ts
packages/core/src/**/*.{ts,tsx,js,jsx}
📄 CodeRabbit inference engine (CLAUDE.md)
Code must conform to ES2020 standard and target modern browsers (Chrome, Edge, Firefox, Safari, Chromium-based browsers). No IE support.
Files:
packages/core/src/view/AbstractGraph.tspackages/core/src/view/plugins/TooltipHandler.ts
packages/core/__tests__/**/*.test.ts
📄 CodeRabbit inference engine (.github/copilot-instructions.md)
Test files must follow the naming pattern
**/*.test.tsand be located inpackages/core/__tests__/directory.
Files:
packages/core/__tests__/view/handler/TooltipHandler.test.tspackages/core/__tests__/view/Graph.test.ts
packages/core/__tests__/**/*.test.{ts,tsx}
📄 CodeRabbit inference engine (CLAUDE.md)
packages/core/__tests__/**/*.test.{ts,tsx}: Tests should use Jest with jsdom environment and be located inpackages/core/__tests__/mirroring thesrc/directory structure.
Import paths in tests should omit.jsextension (handled by moduleNameMapper).
Files:
packages/core/__tests__/view/handler/TooltipHandler.test.tspackages/core/__tests__/view/Graph.test.ts
🧠 Learnings (15)
📓 Common learnings
Learnt from: tbouffard
Repo: maxGraph/maxGraph PR: 776
File: packages/core/src/view/AbstractGraph.ts:492-495
Timestamp: 2025-04-24T12:34:56.726Z
Learning: PR #776 focuses on refactoring to introduce AbstractGraph as a base class and moving code from Graph to AbstractGraph. Type improvements and other refinements should be addressed in future PRs to keep this one focused on the core architectural change.
Learnt from: tbouffard
Repo: maxGraph/maxGraph PR: 857
File: packages/core/src/view/plugins/SelectionHandler.ts:1081-1092
Timestamp: 2025-06-26T13:27:19.012Z
Learning: User tbouffard prefers to keep PRs focused on their main objective and defer implementation improvements or logic fixes to future dedicated PRs. For maxGraph, refactoring PRs like Dictionary→Map migration should not include unrelated logic improvements even when valid issues are identified in the migrated code.
Learnt from: tbouffard
Repo: maxGraph/maxGraph PR: 740
File: packages/core/src/util/requestUtils.ts:79-106
Timestamp: 2025-03-31T08:32:17.055Z
Learning: PR #740 is focused on reorganizing code into namespaces (guiUtils and requestUtils) without changing any implementations. Implementation improvements should be suggested for future PRs.
Learnt from: tbouffard
Repo: maxGraph/maxGraph PR: 849
File: packages/html/stories/DragSource.stories.js:98-101
Timestamp: 2025-06-13T07:48:10.300Z
Learning: User tbouffard prefers answers in English; avoid switching to other languages in future replies.
📚 Learning: 2025-01-28T16:22:25.804Z
Learnt from: tbouffard
Repo: maxGraph/maxGraph PR: 593
File: .github/workflows/create-github-release.yml:42-42
Timestamp: 2025-01-28T16:22:25.804Z
Learning: In the GitHub release notes template for maxGraph, TODO comments are intentionally included to mark sections that must be reviewed by maintainers before publishing the release. These TODOs should not be removed or converted to validation steps as they are part of the release review process.
Applied to files:
CHANGELOG.md
📚 Learning: 2025-12-11T08:46:43.160Z
Learnt from: CR
Repo: maxGraph/maxGraph PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-12-11T08:46:43.160Z
Learning: Applies to packages/core/src/**/*.{ts,tsx} : Define styles as objects conforming to `CellStyle` type as documented in `packages/core/src/types.ts`.
Applied to files:
packages/html/stories/Stylesheet.stories.ts
📚 Learning: 2025-04-22T16:34:40.309Z
Learnt from: tbouffard
Repo: maxGraph/maxGraph PR: 774
File: packages/ts-example-selected-features/package.json:11-13
Timestamp: 2025-04-22T16:34:40.309Z
Learning: In the maxGraph project, TypeScript is defined as a dependency in the root package.json (~5.8.2) and not in individual package.json files for example packages. The build scripts in example packages can use `tsc` commands because the TypeScript dependency is hoisted in the monorepo structure.
Applied to files:
packages/html/stories/Stylesheet.stories.ts
📚 Learning: 2025-04-29T13:29:45.122Z
Learnt from: tbouffard
Repo: maxGraph/maxGraph PR: 791
File: packages/core/src/view/style/edge/Segment.ts:26-35
Timestamp: 2025-04-29T13:29:45.122Z
Learning: In the maxGraph project, JSDoc `{link}` references to symbols don't need to be imported in the current file to work correctly. TypeDoc can resolve symbols across the entire project namespace, not just within the current file, which is common in modern documentation systems and IDEs.
Applied to files:
packages/html/stories/Stylesheet.stories.ts
📚 Learning: 2025-04-28T08:24:39.831Z
Learnt from: tbouffard
Repo: maxGraph/maxGraph PR: 785
File: packages/core/src/view/style/register.ts:19-19
Timestamp: 2025-04-28T08:24:39.831Z
Learning: In the maxGraph project, the Perimeter namespace is defined in the './builtin-style-elements' file as a namespace export ('export * as Perimeter'), making this the correct import location for accessing the Perimeter namespace.
Applied to files:
packages/html/stories/Stylesheet.stories.tspackages/html/stories/Stencils.stories.ts
📚 Learning: 2025-04-28T08:24:39.831Z
Learnt from: tbouffard
Repo: maxGraph/maxGraph PR: 785
File: packages/core/src/view/style/register.ts:19-19
Timestamp: 2025-04-28T08:24:39.831Z
Learning: In the maxGraph project, the Perimeter namespace is defined in the 'builtin-style-elements.ts' file using the statement 'export * as Perimeter from './perimeter';'. To access the Perimeter namespace in other files, they must import from './builtin-style-elements' instead of directly from './perimeter', as part of the tree-shaking optimization.
Applied to files:
packages/html/stories/Stylesheet.stories.tspackages/html/stories/Stencils.stories.ts
📚 Learning: 2025-05-13T12:54:55.231Z
Learnt from: tbouffard
Repo: maxGraph/maxGraph PR: 826
File: packages/js-example-nodejs/src/index.cjs:64-69
Timestamp: 2025-05-13T12:54:55.231Z
Learning: For example code in the maxGraph repository, maintainers prefer to keep scripts simple without error handling to focus on demonstrating core functionality, especially in demonstration scripts like those in packages/js-example-nodejs.
Applied to files:
packages/html/stories/Stylesheet.stories.ts
📚 Learning: 2025-04-24T12:34:10.366Z
Learnt from: tbouffard
Repo: maxGraph/maxGraph PR: 776
File: packages/core/src/view/Graph.ts:85-92
Timestamp: 2025-04-24T12:34:10.366Z
Learning: The Graph class intentionally doesn't use collaborators (cellRenderer, selectionModel, view) from options in initializeCollaborators, as it's designed to replicate the previous implementation using only its factory methods for backward compatibility.
Applied to files:
packages/html/stories/Stylesheet.stories.tspackages/html/stories/Events.stories.ts
📚 Learning: 2024-11-24T17:28:08.023Z
Learnt from: tbouffard
Repo: maxGraph/maxGraph PR: 582
File: packages/core/__tests__/view/handler/config.test.ts:0-0
Timestamp: 2024-11-24T17:28:08.023Z
Learning: In `packages/core/__tests__/view/handler/config.test.ts`, when writing Jest tests in TypeScript for configuration reset functions, small duplication is acceptable if it maintains test clarity and readability. Avoid refactoring tests in ways that could make them harder to understand, especially when future use cases might require specific checks.
Applied to files:
packages/core/__tests__/view/handler/TooltipHandler.test.tspackages/core/__tests__/view/Graph.test.ts
📚 Learning: 2025-12-11T08:46:43.160Z
Learnt from: CR
Repo: maxGraph/maxGraph PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-12-11T08:46:43.160Z
Learning: Applies to packages/core/__tests__/**/*.test.{ts,tsx} : Tests should use Jest with jsdom environment and be located in `packages/core/__tests__/` mirroring the `src/` directory structure.
Applied to files:
packages/core/__tests__/view/handler/TooltipHandler.test.ts
📚 Learning: 2025-12-11T08:46:43.160Z
Learnt from: CR
Repo: maxGraph/maxGraph PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-12-11T08:46:43.160Z
Learning: Applies to packages/core/__tests__/**/*.test.{ts,tsx} : Import paths in tests should omit `.js` extension (handled by moduleNameMapper).
Applied to files:
packages/core/__tests__/view/handler/TooltipHandler.test.tspackages/core/__tests__/view/Graph.test.ts
📚 Learning: 2025-04-24T12:33:36.243Z
Learnt from: tbouffard
Repo: maxGraph/maxGraph PR: 776
File: packages/core/src/view/Graph.ts:77-83
Timestamp: 2025-04-24T12:33:36.243Z
Learning: All register functions in maxGraph (`registerDefaultShapes`, `registerDefaultEdgeStyles`, `registerDefaultPerimeters`, and `registerDefaultEdgeMarkers`) have internal state flags to prevent duplicate registrations, making them inherently idempotent.
Applied to files:
packages/html/stories/Stencils.stories.ts
📚 Learning: 2025-02-08T16:39:11.178Z
Learnt from: tbouffard
Repo: maxGraph/maxGraph PR: 660
File: packages/core/src/view/geometry/node/StencilShapeRegistry.ts:0-0
Timestamp: 2025-02-08T16:39:11.178Z
Learning: The StencilShapeRegistry.getStencil method's implementation using non-null assertion (!) is validated by dedicated tests to handle null/undefined inputs correctly, returning undefined in such cases.
Applied to files:
packages/html/stories/Stencils.stories.ts
📚 Learning: 2025-12-11T08:46:43.160Z
Learnt from: CR
Repo: maxGraph/maxGraph PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-12-11T08:46:43.160Z
Learning: Use `Graph` class for prototyping/demos with automatic setup of defaults.
Applied to files:
packages/html/stories/Events.stories.ts
🧬 Code graph analysis (2)
packages/html/stories/HelloPort.stories.ts (1)
packages/core/src/view/plugins/TooltipHandler.ts (1)
getTooltipForCell(412-419)
packages/core/__tests__/view/handler/TooltipHandler.test.ts (1)
packages/core/__tests__/utils.ts (1)
createGraphWithoutPlugins(29-29)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (4)
- GitHub Check: build
- GitHub Check: build (ubuntu-24.04)
- GitHub Check: build (macos-14)
- GitHub Check: build (windows-2022)
🔇 Additional comments (12)
packages/core/src/view/AbstractGraph.ts (1)
532-535: LGTM! Method correctly handles missing plugin.The optional chaining ensures that calling
setTooltips(true)on a graph without the TooltipHandler plugin does not throw an error, which aligns with the test coverage in Graph.test.ts.packages/html/stories/Events.stories.ts (1)
109-114: LGTM! Correctly migrated to plugin-based tooltip customization.The tooltip customization now uses the TooltipHandler plugin instead of overriding graph methods, which aligns with the PR's goal of moving tooltip logic to the plugin. The non-null assertion is safe here since
getDefaultPlugins()includes the TooltipHandler.packages/html/stories/Stylesheet.stories.ts (1)
66-76: LGTM! Plugin-based tooltip correctly implemented.The migration to TooltipHandler-based tooltips is correct. Note that this story overrides
getTooltip(which receives a CellState) rather thangetTooltipForCell, which is appropriate given the tooltip logic needs access to the cell state. The references tothis.graph.getLabel(...)are correct sincethisrefers to the TooltipHandler instance.packages/html/stories/Stencils.stories.ts (1)
196-199: LGTM! Consistent with the plugin-based tooltip pattern.The tooltip customization correctly uses the TooltipHandler plugin, maintaining consistency with the other migrated stories. The non-null assertion is safe given the use of
getDefaultPlugins().packages/html/stories/Wires.stories.ts (2)
381-405: LGTM! Excellent use of plugin subclassing.This story demonstrates a cleaner pattern for customizing tooltips by extending TooltipHandler rather than overriding methods at runtime. This approach is more type-safe and maintainable. The recursive tooltip building logic correctly uses
super.getTooltipForCell(cell)to get the base tooltip.
732-732: Consistent with the subclassing approach.Replacing TooltipHandler with MyCustomTooltipHandler in the plugins array correctly integrates the custom tooltip logic. This demonstrates that plugin-based customization can be done through either:
- Runtime method override (as in Events/Stylesheet/Stencils stories), or
- Subclassing (as done here)
Both patterns are valid; subclassing may be preferable for complex customizations.
packages/core/__tests__/view/Graph.test.ts (1)
20-24: LGTM! Test correctly validates setTooltips with missing plugin.The test verifies that calling
setTooltips(true)on a graph without the TooltipHandler plugin does not throw an error, which confirms the optional chaining implementation in AbstractGraph.setTooltips works as intended.packages/html/stories/UserObject.stories.ts (1)
170-180: Plugin-based tooltip override implementation looks good.The pattern correctly captures the original method, overrides it with proper
thisbinding via regular function, and delegates to the original for non-edge cases.packages/html/stories/HelloPort.stories.ts (1)
79-94: Well-structured plugin-based tooltip override.The implementation correctly handles the null-safety check for edge terminals (line 86) and properly delegates to the original method. This serves as a good reference pattern for other stories.
packages/core/src/view/plugins/TooltipHandler.ts (3)
29-34: Imports correctly structured with.jsextensions and type-only imports.The new imports follow the coding guidelines:
.jsfile extensions for ESM compliance, andtypekeyword for type-only imports likeEventSource,Shape,SelectionCellsHandler, andCell.
395-419: Well-documented method with backward compatibility.The JSDoc includes a clear usage example, and the implementation maintains backward compatibility with mxGraph by checking for the cell's
getTooltipmethod before falling back toconvertValueToString.
36-45: Class documentation addresses the SelectionCellsHandler dependency.The JSDoc now includes the important note about SelectionCellsHandler requirement for full edge tooltip support, addressing the concern from the previous review.
|
Move
getTooltipandgetTooltipForCellfromAbstractGraphto theTooltipHandlerplugin as this is the only place where they are used.This improves modularity and tree-shaking, clarifies responsibilities, and reduces the amount of code in
AbstractGraphby removing theTooltipMixin.The stories have been updated accordingly and can be used as examples for adapting application code.
BREAKING CHANGES: The
getTooltipandgetTooltipForCellmethods have been moved fromAbstractGraphto theTooltipHandlerplugin.If you were overriding these methods in a
AbstractGraphsubclass, you should now extendTooltipHandlerinstead.Covers #762
Impacts on the size of the examples
Examples that don't use TooltipHander: -0.7kB
Remaining tasks
Events: refactor: migrate the Events story to TypeScript #702Stylesheetrefactor: migrate the Stylesheet story to TypeScript #878Wiresrefactor: fix most problems in the Wires story #893Summary by CodeRabbit
Breaking Changes
New Features
✏️ Tip: You can customize this high-level summary in your review settings.