refactor: improve StencilShapeRegistry method signatures
#660
Conversation
Also improve the JSDoc.
WalkthroughThe pull request introduces a new Jest test file for the StencilShapeRegistry and makes several adjustments to how stencil shapes are retrieved. Explicit type assertions have been removed from calls to Changes
Sequence Diagram(s)sequenceDiagram
participant Component
participant Registry as StencilShapeRegistry
Component->>Registry: getStencil(stencilName)
alt Stencil exists
Registry-->>Component: Returns StencilShape
else
Registry-->>Component: Returns undefined
end
📜 Recent review detailsConfiguration used: CodeRabbit UI 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (2)
packages/core/__tests__/view/geometry/node/StencilShapeRegistry.test.ts (1)
20-24: LGTM! Consider adding more test cases.The test structure using
test.eachis clean and efficient. However, consider adding more test cases to cover:
- Valid stencil names
- Empty string input
- Special characters in stencil names
describe('getStencil', () => { test.each([null, undefined, 'unknown'])('pass %s, return undefined', (name) => { expect(StencilShapeRegistry.getStencil(name)).toBeUndefined(); }); + test.each([ + ['', undefined], + ['valid-stencil', undefined], // Update expected value when stencil is registered + ['special@#$%', undefined], + ])('pass %s, return %s', (name, expected) => { + expect(StencilShapeRegistry.getStencil(name)).toBe(expected); + }); });packages/core/src/view/geometry/node/StencilShapeRegistry.ts (1)
25-42: Improve code example in JSDoc.The code example uses outdated syntax and variables. Consider updating it to match the current implementation.
/** * A singleton class that provides a registry for stencils and the methods for painting those stencils onto a canvas or into a DOM. * * Code to add stencils: * ```javascript - const req = load('test/stencils.xml'); - const root = req.getDocumentElement(); - const shape = root.firstChild; + const response = load('test/stencils.xml'); + const root = response.getDocumentElement(); + let shape = root.firstChild; * * while (shape) { * if (shape.nodeType === mxConstants.NODETYPE_ELEMENT) { - * StencilRegistry.addStencil(shape.getAttribute('name'), new mxStencil(shape)); + * StencilShapeRegistry.addStencil(shape.getAttribute('name'), new StencilShape(shape)); * } * * shape = shape.nextSibling; * } * ``` */
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (5)
packages/core/__tests__/view/geometry/node/StencilShapeRegistry.test.ts(1 hunks)packages/core/src/view/cell/CellRenderer.ts(1 hunks)packages/core/src/view/geometry/node/StencilShape.ts(1 hunks)packages/core/src/view/geometry/node/StencilShapeRegistry.ts(1 hunks)packages/html/stories/Stencils.stories.ts(1 hunks)
✅ Files skipped from review due to trivial changes (1)
- packages/core/src/view/geometry/node/StencilShape.ts
🔇 Additional comments (2)
packages/html/stories/Stencils.stories.ts (1)
98-98: Consider handling undefined stencil shape.The removal of the nullish coalescing operator could lead to undefined access. Consider adding a fallback or explicit handling.
- const stencil = StencilShapeRegistry.getStencil(this.state.style.shape); + const stencil = StencilShapeRegistry.getStencil(this.state.style.shape) ?? null;✅ Verification successful
Action Required: Safeguard Undefined Stencil Shapes
The code calls
const stencil = StencilShapeRegistry.getStencil(this.state.style.shape);
which may return undefined if the provided shape is not registered. Not handling this scenario could lead to runtime errors. It’s recommended to add explicit handling (e.g., a fallback with the nullish coalescing operator) so that undefined access is prevented:• File: packages/html/stories/Stencils.stories.ts, line 98
Replace with:
const stencil = StencilShapeRegistry.getStencil(this.state.style.shape) ?? null;This ensures that stencil is never undefined and allows subsequent code to check for a null value safely.
packages/core/src/view/cell/CellRenderer.ts (1)
181-182: LGTM! Type-safe stencil retrieval.The removal of type assertion and addition of explanatory comment improves code clarity and type safety.
StencilShapeRegistry method signatures
|
Also improve the JSDoc.
Summary by CodeRabbit
Tests
getStencilmethod with various input values.Refactor
Style