feat: allow to pass more null and undefined to Multiplicity#914
feat: allow to pass more null and undefined to Multiplicity#914
Conversation
Restore the behavior available in mxGraph to ease the migration. In addition: - Improve nullish management in various related places - Improve JSDoc - Validation mixin type: remove duplicated declaration of `multiplicities` already defined in AbstractGraph - Migrate the Validation story to TypeScript to ease maintenance and detect errors earlier
WalkthroughStandardized null/undefined handling via Changes
Sequence Diagram(s)sequenceDiagram
autonumber
actor User
participant Story as Validation.story
participant Plugins as getDefaultPlugins()
participant Graph as Graph
participant RB as RubberBandHandler
participant Val as Validation Pipeline
User->>Story: load story
Story->>Plugins: getDefaultPlugins()
alt rubberBand enabled
Plugins->>RB: push RubberBandHandler
end
Story->>Graph: new Graph(container, undefined, plugins)
Story->>Graph: insertVertex({ ... }) / insertEdge({ ... })
User->>Graph: interact (create/connect/delete)
Graph->>Val: trigger validation
Val-->>Graph: return error or ok
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related PRs
Pre-merge checks (2 passed, 1 warning)❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
Tip 👮 Agentic pre-merge checks are now available in preview!Pro plan users can now enable pre-merge checks in their settings to enforce checklists before merging PRs.
Please see the documentation for more information. Example: reviews:
pre_merge_checks:
custom_checks:
- name: "Undocumented Breaking Changes"
mode: "warning"
instructions: |
Pass/fail criteria: All breaking changes to public APIs, CLI flags, environment variables, configuration keys, database schemas, or HTTP/GraphQL endpoints must be documented in the "Breaking Change" section of the PR description and in CHANGELOG.md. Exclude purely internal or private changes (e.g., code not exported from package entry points or explicitly marked as internal).Please share your feedback with us on this Discord post. 📜 Recent review detailsConfiguration used: CodeRabbit UI Review profile: CHILL Plan: Pro 📒 Files selected for processing (3)
💤 Files with no reviewable changes (1)
🚧 Files skipped from review as they are similar to previous changes (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). (4)
✨ Finishing touches
🧪 Generate unit tests
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 |
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (6)
packages/html/stories/Guides.stories.ts (1)
145-148: Good breadcrumb to the issue; add a small focus workaround to help KeyHandler in StorybookGiven the iframe focus quirk (see prior learnings), make the container focusable before calling
focus()to improve key handling reliability.Add near Line 141 (outside this hunk):
// Ensure container is focusable in Storybook iframe before focusing container.tabIndex = 0;packages/core/src/view/cell/Cell.ts (1)
236-245: Nit: align remaining tags to use {@link} consistentlyThere are still angle-bracket mentions of geometry. Consider switching to {@link geometry} for consistency.
- * Returns the {@link Geometry} that describes the <geometry>. + * Returns the {@link Geometry} that describes the {@link geometry}. ... - * Sets the {@link Geometry} to be used as the <geometry>. + * Sets the {@link Geometry} to be used as the {@link geometry}.packages/core/src/util/domUtils.ts (2)
224-229: Signature widened correctly; tiny doc follow-upAllowing
attributeName/attributeValueto benullaligns with the PR goals. Consider updating the JSDoc above to explicitly mentionstring | nullfor these params.
230-238: Minor clarity tweaks inisNode
- Prefer a direct type check over
isNaNfornodeType.- Keep the loose equality but document why, to avoid future eqeqeq “fixes” that would change semantics.
- !isNaN(value.nodeType) && + typeof value.nodeType === 'number' && (isNullish(nodeName) || value.nodeName.toLowerCase() == nodeName.toLowerCase()) ) { - return ( - isNullish(attributeName) || value.getAttribute(attributeName) == attributeValue - ); + // Intentionally using loose equality to treat null and undefined as equivalent here. + return isNullish(attributeName) || value.getAttribute(attributeName) == attributeValue; }packages/html/stories/Validation.stories.ts (2)
53-53: Fix arg typing: story args include booleans, not just strings
rubberBand(and others) are booleans; typing asRecord<string, string>is misleading. Relax toanyfor now (stories are JS-driven), or introduce a proper Args type later.-const Template = ({ label, ...args }: Record<string, string>) => { +const Template = ({ label, ...args }: Record<string, any>) => {
124-128: Help the Delete key binding in Storybook by ensuring focusTo mitigate iframe focus issues, make the container focusable and focus it before creating/binding the
KeyHandler.// Removes cells when [DELETE] is pressed // TODO pressing the delete key has no effect in FF and Chrome (https://github.com/maxGraph/maxGraph/issues/910) - const keyHandler = new KeyHandler(graph); + // Ensure the container can receive focus for key events in Storybook iframes + container.tabIndex = 0; + container.focus(); + const keyHandler = new KeyHandler(graph); keyHandler.bindKey(46, function () { if (graph.isEnabled()) { graph.removeCells(); } });
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (8)
packages/core/src/util/domUtils.ts(2 hunks)packages/core/src/view/cell/Cell.ts(1 hunks)packages/core/src/view/mixins/ValidationMixin.ts(3 hunks)packages/core/src/view/mixins/ValidationMixin.type.ts(0 hunks)packages/core/src/view/other/Multiplicity.ts(6 hunks)packages/html/stories/Guides.stories.ts(1 hunks)packages/html/stories/Validation.stories.ts(3 hunks)packages/website/docs/usage/i18n.md(1 hunks)
💤 Files with no reviewable changes (1)
- packages/core/src/view/mixins/ValidationMixin.type.ts
🧰 Additional context used
🧠 Learnings (4)
📓 Common learnings
Learnt from: tbouffard
PR: maxGraph/maxGraph#835
File: packages/html/stories/Orthogonal.stories.ts:114-117
Timestamp: 2025-05-26T12:34:54.318Z
Learning: The `null!` assertion pattern exists across multiple stories in the TypeScript migration and should be addressed systematically rather than in individual PRs.
Learnt from: tbouffard
PR: maxGraph/maxGraph#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-08-28T14:07:42.556Z
Learnt from: tbouffard
PR: maxGraph/maxGraph#908
File: packages/html/stories/Guides.stories.ts:0-0
Timestamp: 2025-08-28T14:07:42.556Z
Learning: KeyHandler does not work properly in Storybook environment due to iframe context - keyboard events added to document.documentElement in iframe don't receive events properly. This affects all maxGraph stories that use KeyHandler, with several stories having KeyHandler commented out with "TODO not working" comments.
Applied to files:
packages/html/stories/Guides.stories.ts
📚 Learning: 2025-08-28T14:07:42.556Z
Learnt from: tbouffard
PR: maxGraph/maxGraph#908
File: packages/html/stories/Guides.stories.ts:0-0
Timestamp: 2025-08-28T14:07:42.556Z
Learning: KeyHandler issues in Storybook are due to iframe isolation - Storybook renders stories in iframes where keyboard events don't properly reach document.addEventListener without explicit focus management. The solution is to ensure the iframe window has focus by calling window.focus() and focusing the target element in the preview configuration or story setup.
Applied to files:
packages/html/stories/Guides.stories.ts
📚 Learning: 2024-12-15T18:19:56.236Z
Learnt from: tbouffard
PR: maxGraph/maxGraph#598
File: packages/website/docs/manual/getting-started.md:70-70
Timestamp: 2024-12-15T18:19:56.236Z
Learning: In code examples within the documentation, such as in `packages/website/docs/manual/getting-started.md`, we assume that the `graph-container` element exists and is an `HTMLElement`, and we avoid adding error handling for its initialization to keep the code simple.
Applied to files:
packages/html/stories/Validation.stories.ts
🧬 Code graph analysis (4)
packages/core/src/util/domUtils.ts (1)
packages/core/src/internal/utils.ts (1)
isNullish(44-45)
packages/core/src/view/mixins/ValidationMixin.ts (1)
packages/core/src/internal/utils.ts (1)
isNullish(44-45)
packages/html/stories/Validation.stories.ts (2)
packages/html/stories/shared/configure.js (1)
createMainDiv(53-63)packages/core/src/view/plugins/index.ts (1)
getDefaultPlugins(46-55)
packages/core/src/view/other/Multiplicity.ts (4)
packages/core/src/internal/i18n-utils.ts (1)
translate(29-35)packages/core/src/view/cell/Cell.ts (1)
Cell(66-886)packages/core/src/internal/utils.ts (1)
isNullish(44-45)packages/core/src/util/domUtils.ts (1)
isNode(224-241)
⏰ 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 (macos-14)
- GitHub Check: build (ubuntu-24.04)
- GitHub Check: build (windows-2022)
🔇 Additional comments (19)
packages/website/docs/usage/i18n.md (1)
74-75: LGTM: link updated to TS storyThe source link now points to
Validation.stories.ts, matching the migration. No further changes needed.packages/core/src/view/cell/Cell.ts (2)
210-214: JSDoc reference fix looks goodUsing {@link value} is consistent with the rest of the file.
217-221: JSDoc reference fix looks goodSame here; consistent and clearer.
packages/core/src/util/domUtils.ts (1)
18-19: ImportingisNullishis appropriate for the new semanticsThis matches the nullish-first approach used across the PR.
packages/html/stories/Validation.stories.ts (2)
68-74: Plugin-based RubberBand integration: LGTMUsing
getDefaultPlugins()and conditionally pushingRubberBandHandleris clean and keeps the story consistent with the plugin model.
141-184: Object-form insert APIs: LGTMUpdated
insertVertex/insertEdgecalls match the new signatures and improve readability.packages/core/src/view/mixins/ValidationMixin.ts (6)
21-21: LGTM! Good use of the centralizedisNullishutility.The import of
isNullishfrom the internal utilities promotes consistency across the codebase for nullish checking.
116-118: LGTM! Consistent nullish checking pattern.Replacing
err != nullwith!isNullish(err)standardizes the nullish checking approach across the validation logic.
123-125: LGTM! Consistent error handling pattern.Good consistency with the nullish checking pattern using
isNullish.
190-190: LGTM! Removed unnecessary type cast.Removing the
<Cell>cellcast and passingcelldirectly is cleaner since the type is already known.
194-197: LGTM! Consistent nullish checking applied.The nullish checking pattern is consistently applied here as well.
122-122: Remove the non-null assertion onedgewhen callingvalidateEdge.
validateEdgeis declared to accept a nullable edge;edge!hides that and can mask cases whereedge === null. Change:
packages/core/src/view/mixins/ValidationMixin.ts
const err = this.validateEdge(edge!, source, target);
to
const err = this.validateEdge(edge, source, target);
Verify there are no overrides that assume a non-nulledgebefore removing the!.packages/core/src/view/other/Multiplicity.ts (7)
23-23: LGTM! Consistent use of centralizedisNullishutility.Good alignment with the validation mixin changes by using the same utility function.
27-27: Documentation correctly updated to referenceAbstractGraph.multiplicities.The JSDoc now accurately reflects where multiplicities are stored.
29-44: Well-documented example with the updated constructor signature.The example clearly demonstrates the new constructor parameters including the nullish values, making it easier for developers to understand the API changes.
50-57: Constructor signature properly widened to accept nullish values.The parameters
attr,value,validNeighbors,countError, andtypeErrornow acceptnull | undefined, which aligns with the PR objective to restore mxGraph behavior for easier migration.
65-67: Good defensive programming with fallback values.The use of nullish coalescing (
??) ensures sensible defaults:
validNeighborsdefaults to an empty arraycountErrorandtypeErrorare normalized to empty strings when nullishThis prevents potential runtime errors while maintaining backward compatibility.
179-179: Parameter naming convention for unused parameters.Good practice using the underscore prefix (
_edge) to indicate unused parameters. This clearly communicates intent to other developers.Also applies to: 204-204
220-233: Robust type checking with early return and proper DOM node detection.The refactored
checkTypemethod:
- Early returns
falsefor nullish values (defensive programming)- Properly checks for DOM nodes using a more explicit approach
- Falls back to strict equality for non-DOM values
The DOM node check is more robust than before, explicitly checking for the presence of
nodeTypeproperty.
|
Restore the behavior available in mxGraph to ease the migration.
In addition:
multiplicitiesalready defined in AbstractGraphSummary by CodeRabbit
New Features
Bug Fixes
Refactor
Documentation