fix: registry tools is not available #4799
Conversation
Summary of ChangesHello @lxfu1, I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed! This pull request addresses an issue where certain registry tools were not properly available by refactoring their definition mechanism. Instead of using a functional Highlights
Using Gemini Code AssistThe full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips. Invoking Gemini You can request assistance from Gemini at any point by creating a comment using either
Customization To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a Limitations & Feedback Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with 👍 and 👎 on @gemini-code-assist comments. If you're interested in giving your feedback about your experience with Gemini Code Assist for Github and other Google products, sign up here. You can also get AI-powered code generation, chat, as well as code reviews directly in the IDE at no cost with the Gemini Code Assist IDE Extension. Footnotes
|
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request refactors the way registry tools are defined, moving from a static define method to a more idiomatic class-based approach with static defaults. This is a great improvement for code clarity and maintainability.
My review focuses on ensuring consistency and preventing potential issues with the new implementation. I've noticed that in some files, the spread syntax (...) is used to extend default options, which performs a shallow copy. This can lead to unintended sharing of nested objects and arrays between base and subclasses. I've suggested using ObjectExt.merge for a deep merge, similar to the approach taken in editor.ts, to make the implementation more robust and consistent across all tool definitions.
| export class SourceAnchor extends Anchor { | ||
| public static defaults: Options = { | ||
| ...Anchor.getDefaults(), | ||
| name: 'source-anchor', | ||
| type: 'source', | ||
| } | ||
| } | ||
|
|
||
| export const TargetAnchor = Anchor.define<Options>({ | ||
| name: 'target-anchor', | ||
| type: 'target', | ||
| }) | ||
| export class TargetAnchor extends Anchor { | ||
| public static defaults: Options = { | ||
| ...Anchor.getDefaults(), | ||
| name: 'target-anchor', | ||
| type: 'target', | ||
| } | ||
| } |
There was a problem hiding this comment.
Using the spread syntax ... for Anchor.getDefaults() performs a shallow copy. The Anchor.defaults object contains nested objects like markup, customAnchorAttrs, and defaultAnchorAttrs. This means that SourceAnchor.defaults and TargetAnchor.defaults will share references to these nested objects with Anchor.defaults.
This could lead to unintended side effects if a subclass modifies one of these nested objects, as it would also modify the base class's defaults.
To ensure that each tool has its own copy of the default options, it's safer to perform a deep merge. You're already doing this in src/registry/tool/editor.ts using ObjectExt.merge. I suggest applying the same pattern here for consistency and safety.
You'll need to import ObjectExt from ../../common.
export class SourceAnchor extends Anchor {
public static defaults: Options = ObjectExt.merge({}, Anchor.getDefaults(), {
name: 'source-anchor',
type: 'source',
})
}
export class TargetAnchor extends Anchor {
public static defaults: Options = ObjectExt.merge({}, Anchor.getDefaults(), {
name: 'target-anchor',
type: 'target',
})
}| export class SourceArrowhead extends Arrowhead { | ||
| public static defaults: Options = { | ||
| ...Arrowhead.getDefaults(), | ||
| name: 'source-arrowhead', | ||
| type: 'source', | ||
| ratio: 0, | ||
| attrs: { | ||
| d: 'M 10 -8 -10 0 10 8 Z', | ||
| fill: '#333', | ||
| stroke: '#fff', | ||
| 'stroke-width': 2, | ||
| cursor: 'move', | ||
| }, | ||
| } | ||
| } | ||
|
|
||
| export class TargetArrowhead extends Arrowhead { | ||
| public static defaults: Options = { | ||
| ...Arrowhead.getDefaults(), | ||
| name: 'target-arrowhead', | ||
| type: 'target', | ||
| ratio: 1, | ||
| attrs: { | ||
| d: 'M -10 -8 10 0 -10 8 Z', | ||
| fill: '#333', | ||
| stroke: '#fff', | ||
| 'stroke-width': 2, | ||
| cursor: 'move', | ||
| }, | ||
| } | ||
| } |
There was a problem hiding this comment.
Using the spread syntax ... for Arrowhead.getDefaults() performs a shallow copy. While Arrowhead.defaults itself doesn't have many nested objects, this pattern can be risky if they are added later. The attrs object is defined here, but if Arrowhead.defaults had its own attrs, they would be shallow-merged.
For consistency with the changes in editor.ts and to prevent potential bugs from shared object references, I recommend using ObjectExt.merge to perform a deep merge. This ensures that SourceArrowhead and TargetArrowhead get their own copies of the default options.
You'll need to import ObjectExt from ../../common.
export class SourceArrowhead extends Arrowhead {
public static defaults: Options = ObjectExt.merge({}, Arrowhead.getDefaults(), {
name: 'source-arrowhead',
type: 'source',
ratio: 0,
attrs: {
d: 'M 10 -8 -10 0 10 8 Z',
fill: '#333',
stroke: '#fff',
'stroke-width': 2,
cursor: 'move',
},
})
}
export class TargetArrowhead extends Arrowhead {
public static defaults: Options = ObjectExt.merge({}, Arrowhead.getDefaults(), {
name: 'target-arrowhead',
type: 'target',
ratio: 1,
attrs: {
d: 'M -10 -8 10 0 -10 8 Z',
fill: '#333',
stroke: '#fff',
'stroke-width': 2,
cursor: 'move',
},
})
}| export class Remove extends Button { | ||
| public static defaults: Options = { | ||
| ...Button.getDefaults(), | ||
| name: 'button-remove', | ||
| markup: [ | ||
| { | ||
| tagName: 'circle', | ||
| selector: 'button', | ||
| attrs: { | ||
| r: 7, | ||
| fill: '#FF1D00', | ||
| cursor: 'pointer', | ||
| }, | ||
| }, | ||
| }, | ||
| { | ||
| tagName: 'path', | ||
| selector: 'icon', | ||
| attrs: { | ||
| d: 'M -3 -3 3 3 M -3 3 3 -3', | ||
| fill: 'none', | ||
| stroke: '#FFFFFF', | ||
| 'stroke-width': 2, | ||
| 'pointer-events': 'none', | ||
| { | ||
| tagName: 'path', | ||
| selector: 'icon', | ||
| attrs: { | ||
| d: 'M -3 -3 3 3 M -3 3 3 -3', | ||
| fill: 'none', | ||
| stroke: '#FFFFFF', | ||
| 'stroke-width': 2, | ||
| 'pointer-events': 'none', | ||
| }, | ||
| }, | ||
| ], | ||
| distance: 60, | ||
| offset: 0, | ||
| useCellGeometry: true, | ||
| onClick({ view, btn }) { | ||
| btn.parent.remove() | ||
| view.cell.remove({ ui: true, toolId: btn.cid }) | ||
| }, | ||
| ], | ||
| distance: 60, | ||
| offset: 0, | ||
| useCellGeometry: true, | ||
| onClick({ view, btn }) { | ||
| btn.parent.remove() | ||
| view.cell.remove({ ui: true, toolId: btn.cid }) | ||
| }, | ||
| }) | ||
| } | ||
| } |
There was a problem hiding this comment.
Using the spread syntax ... for Button.getDefaults() performs a shallow copy. The Remove.defaults includes a markup array of objects. If Button.defaults also contained a markup array, the spread syntax would lead to a shallow merge, which could cause unexpected mutations if the array or its objects are modified in subclasses.
To ensure each tool has its own deep copy of the default options and for consistency with the pattern used in editor.ts, it's safer to use ObjectExt.merge.
You'll need to import ObjectExt from ../../common.
export class Remove extends Button {
public static defaults: Options = ObjectExt.merge({}, Button.getDefaults(), {
name: 'button-remove',
markup: [
{
tagName: 'circle',
selector: 'button',
attrs: {
r: 7,
fill: '#FF1D00',
cursor: 'pointer',
},
},
{
tagName: 'path',
selector: 'icon',
attrs: {
d: 'M -3 -3 3 3 M -3 3 3 -3',
fill: 'none',
stroke: '#FFFFFF',
'stroke-width': 2,
'pointer-events': 'none',
},
},
],
distance: 60,
offset: 0,
useCellGeometry: true,
onClick({ view, btn }) {
btn.parent.remove()
view.cell.remove({ ui: true, toolId: btn.cid })
},
})
}
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## master #4799 +/- ##
==========================================
+ Coverage 82.04% 82.06% +0.01%
==========================================
Files 327 327
Lines 37628 37655 +27
Branches 7592 7597 +5
==========================================
+ Hits 30873 30900 +27
Misses 6731 6731
Partials 24 24
Flags with carried forward coverage won't be shown. Click here to find out more.
🚀 New features to boost your workflow:
|
Description
Motivation and Context
Types of changes
Self Check before Merge