Conversation
Reviewer's GuideThis pull request introduces a new NodeGraph component to the BootstrapBlazor library, implementing a node-based graph editor with extensible node and widget types, service registration, and Blazor/JS interop for graph operations. File-Level Changes
Tips and commandsInteracting with Sourcery
Customizing Your ExperienceAccess your dashboard to:
Getting Help
|
|
Thanks for your PR, @syminomega. Someone from the team will get assigned to your PR shortly and we'll get it reviewed. |
![sourcery-ai[bot]](https://avatars.githubusercontent.com/in/48477?s=60&v=4){kind=small}
There was a problem hiding this comment.
Hey @syminomega - I've reviewed your changes and they look great!
Here's what I looked at during the review
- 🟡 General issues: 21 issues found
- 🟢 Security: all looks good
- 🟢 Testing: all looks good
- 🟡 Complexity: 2 issues found
- 🟢 Documentation: all looks good
Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
| _moduleTask = new Lazy<Task<IJSObjectReference>>(_jsRuntime | ||
| .InvokeAsync<IJSObjectReference>("import", "./_content/BootstrapBlazor.NodeGraph/js/Graph.js") | ||
| .AsTask()); | ||
| _reference = DotNetObjectReference.Create(this); |
There was a problem hiding this comment.
issue (bug_risk): Created DotNetObjectReference but never used
Since _reference isn’t passed to JS, callbacks like OnNodeActionExecuted won’t run. Remove the unused field or pass _reference into your interop calls (e.g. during registration).
| } | ||
| } |
There was a problem hiding this comment.
suggestion (bug_risk): DisposeAsync does not clean up the DotNetObjectReference
Also call _reference.Dispose() after disposing the JS module to release the .NET reference and prevent memory leaks.
| } | |
| } | |
| } | |
| } | |
| // Dispose the DotNetObjectReference to prevent memory leaks | |
| _reference.Dispose(); |
| if (_registeredNodes.ContainsKey(typePath)) | ||
| { | ||
| _registeredNodes.Remove(typePath); | ||
| // TODO: 更新节点 |
There was a problem hiding this comment.
issue (bug_risk): Re-registering a node type only updates local state
After removing the node from _registeredNodes, implement the corresponding JS-side unregister or update logic so both sides stay in sync.
| @@ -0,0 +1,41 @@ | |||
| @namespace BootstrapBlazor.Components | |||
| @using Microsoft.Extensions.Logging | |||
| @attribute [JSModuleAutoLoader("./_content/BootstrapBlazor.NodeGraph/js/Graph.js", AutoInvokeDispose = false, JSObjectReference = true)] | |||
There was a problem hiding this comment.
suggestion (bug_risk): Graph.js is imported both here and in the service
This creates two module instances. Unify to a single import to prevent duplicate downloads and ensure state is shared.
Suggested implementation:
| } | ||
|
|
||
| var jsModule = await _moduleTask.Value; | ||
| await jsModule.InvokeVoidAsync("registerNodeType", graphNodeConfig); |
There was a problem hiding this comment.
issue (bug_risk): Missing .NET reference when registering node type
Include a DotNetObjectReference (e.g., _reference or a new one) when calling registerNodeType so JS can invoke your [JSInvokable] methods.
| var be = (f) => { | ||
| throw TypeError(f); | ||
| }; | ||
| var Je = (f, t, i) => t in f ? Ze(f, t, { enumerable: !0, configurable: !0, writable: !0, value: i }) : f[t] = i; |
There was a problem hiding this comment.
issue (code-quality): Use const or let instead of var. (avoid-using-var)
Explanation
`const` is preferred as it ensures you cannot reassign references (which can lead to buggy and confusing code). `let` may be used if you need to reassign references - it's preferred to `var` because it is block- rather than function-scoped.From the Airbnb JavaScript Style Guide
| var Ie = (f, t, i, n) => ({ | ||
| set _(s) { | ||
| W(f, t, s, i); | ||
| }, | ||
| get _() { | ||
| return L(f, t, n); | ||
| } | ||
| }); |
There was a problem hiding this comment.
issue (code-quality): Use const or let instead of var. (avoid-using-var)
Explanation
`const` is preferred as it ensures you cannot reassign references (which can lead to buggy and confusing code). `let` may be used if you need to reassign references - it's preferred to `var` because it is block- rather than function-scoped.From the Airbnb JavaScript Style Guide
| function T(G, b, R, C) { | ||
| t.fillStyle = C, t.fillText(R, G, b); | ||
| } |
There was a problem hiding this comment.
issue (code-quality): Avoid function declarations, favouring function assignment expressions, inside blocks. (avoid-function-declarations-in-blocks)
Explanation
Function declarations may be hoisted in Javascript, but the behaviour is inconsistent between browsers. Hoisting is generally confusing and should be avoided. Rather than using function declarations inside blocks, you should use function expressions, which create functions in-scope.| function O(G, b, R) { | ||
| t.beginPath(), t.fillStyle = R, A(G, b), t.fill(); | ||
| } |
There was a problem hiding this comment.
issue (code-quality): Avoid function declarations, favouring function assignment expressions, inside blocks. (avoid-function-declarations-in-blocks)
Explanation
Function declarations may be hoisted in Javascript, but the behaviour is inconsistent between browsers. Hoisting is generally confusing and should be avoided. Rather than using function declarations inside blocks, you should use function expressions, which create functions in-scope.| function A(G, b) { | ||
| t.arc(G, b, i, n, s); | ||
| } |
There was a problem hiding this comment.
issue (code-quality): Avoid function declarations, favouring function assignment expressions, inside blocks. (avoid-function-declarations-in-blocks)
Explanation
Function declarations may be hoisted in Javascript, but the behaviour is inconsistent between browsers. Hoisting is generally confusing and should be avoided. Rather than using function declarations inside blocks, you should use function expressions, which create functions in-scope.
2 participants
Link issues
fixes #{issue number}
Summary By Copilot
Regression?
Risk
Verification
Packaging changes reviewed?
☑️ Self Check before Merge
Summary by Sourcery
Add a new NodeGraph component and supporting infrastructure for building and interacting with node-based graphs, including UI, service, data models, and configuration options.
New Features:
Enhancements:
Build: