refactor: simplify implementation of ParallelEdgeLayout #909
Conversation
WalkthroughTyped and behavioral-tightening updates to ParallelEdgeLayout: Changes
Sequence Diagram(s)Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Suggested labels
✨ 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. 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. CodeRabbit Commands (Invoked using PR/Issue comments)Type Other keywords and placeholders
CodeRabbit Configuration File (
|
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (5)
packages/core/src/view/layout/ParallelEdgeLayout.ts (5)
38-58: Use strict equality in the JS example.Small tweak for best practice and to avoid accidental type coercion in examples.
- * return !(model.isEdge(edge2) && ((src == src2 && trg == trg2) || (src == trg2 && trg == src2))); + * return !(model.isEdge(edge2) && ((src === src2 && trg === trg2) || (src === trg2 && trg === src2)));
74-77: Minor grammar and tag consistency.Use “whether” and align with @DefaultValue.
- * Specifies if only overlapping edges should be considered parallel. - * @default false + * Specifies whether only overlapping edges should be considered parallel. + * @defaultValue false
100-100: Guard against prototype pollution in lookup map.Using a plain object is fine, but Object.create(null) avoids inherited keys. Also prefer the shorter
Cell[]syntax.- const lookup: Record<string, Array<Cell>> = {}; + const lookup: Record<string, Cell[]> = Object.create(null);Optionally, iterate without for..in (outside this hunk):
for (const [id, parallels] of Object.entries(lookup)) { if (parallels.length > 1) this.layout(parallels); }
133-135: Tighten wording: pluralize “terminals” and capitalize “ID”.Small doc polish.
- * Returns a unique ID for the given edge. - * The id is independent of the edge direction and is built using the visible terminal of the given edge. + * Returns a unique ID for the given edge. + * The ID is independent of the edge direction and is built using the visible terminals of the given edge.Also, minor grammar fix in the method body comment (outside this hunk):
// Cannot use cached visible terminal because this could be triggered in BEFORE_UNDO
68-71: Retain@defaulttag and clarify units. Update only the description to “Defines the spacing between parallel edges (in pixels).” and keep@default 20to match the repo’s existing JSDoc convention.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
💡 Knowledge Base configuration:
- MCP integration is disabled by default for public repositories
- Jira integration is disabled by default for public repositories
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (1)
packages/core/src/view/layout/ParallelEdgeLayout.ts(5 hunks)
🧰 Additional context used
🧠 Learnings (1)
📓 Common learnings
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.
⏰ 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 (windows-2022)
- GitHub Check: build (macos-14)
🔇 Additional comments (1)
packages/core/src/view/layout/ParallelEdgeLayout.ts (1)
27-30: Doc intro improvements look good.Accurate summary and clearer scoping for parallel edges.
There was a problem hiding this comment.
Actionable comments posted: 0
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
packages/core/src/view/layout/ParallelEdgeLayout.ts (1)
139-139: Fix grammar in comment.- // Cannot used cached visible terminal because this could be triggered in BEFORE_UNDO + // Cannot use cached visible terminal because this could be triggered in BEFORE_UNDO
🧹 Nitpick comments (4)
packages/core/src/view/layout/ParallelEdgeLayout.ts (4)
38-38: Example preface: tiny polish.Consider adding an inline note that InternalEvent must be imported in the example to avoid confusion for readers integrating it.
43-55: Align example with runtime: use visible terminals like getEdgeId does.findParallels/getEdgeId use view.getVisibleTerminal; the example filters with model.getTerminal, which can diverge when terminals aren’t visible. Suggest this doc tweak for consistency.
-graph.addListener(InternalEvent.CELL_CONNECTED, (sender, evt) => { +graph.addListener(InternalEvent.CELL_CONNECTED, (sender, evt) => { const model = graph.getDataModel(); + const view = graph.getView(); const edge = evt.getProperty('edge'); - const src = model.getTerminal(edge, true); - const trg = model.getTerminal(edge, false); + const src = view.getVisibleTerminal(edge, true); + const trg = view.getVisibleTerminal(edge, false); layout.isEdgeIgnored = (edge2) => { - const src2 = model.getTerminal(edge2, true); - const trg2 = model.getTerminal(edge2, false); + const src2 = view.getVisibleTerminal(edge2, true); + const trg2 = view.getVisibleTerminal(edge2, false); return !(model.isEdge(edge2) && ((src === src2 && trg === trg2) || (src === trg2 && trg === src2))); };
99-129: Typed return is good; tighten dictionary usage and iteration.Minor safety/readability tweaks:
- Use a null-prototype object to avoid inherited enumerable keys.
- Use nullish coalescing assignment when populating.
- Iterate with Object.values to avoid for..in pitfalls.
-findParallels(parent: Cell, cells: Cell[] | null = null): Record<string, Cell[]> { - const lookup: Record<string, Cell[]> = {}; +findParallels(parent: Cell, cells: Cell[] | null = null): Record<string, Cell[]> { + const lookup = Object.create(null) as Record<string, Cell[]>; … - if (id != null) { - if (lookup[id] == null) { - lookup[id] = []; - } - - lookup[id].push(cell); - } + if (id != null) { + (lookup[id] ??= []).push(cell); + }And in execute:
- this.graph.batchUpdate(() => { - for (const i in lookup) { - const parallels = lookup[i]; - - if (parallels.length > 1) { - this.layout(parallels); - } - } - }); + this.graph.batchUpdate(() => { + for (const parallels of Object.values(lookup)) { + if (parallels.length > 1) this.layout(parallels); + } + });
133-135: Doc wording improvement is accurate. Add explicit return type to the method.getEdgeId returns string | null; make it explicit for public API clarity.
- getEdgeId(edge: Cell) { + getEdgeId(edge: Cell): string | null {
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
💡 Knowledge Base configuration:
- MCP integration is disabled by default for public repositories
- Jira integration is disabled by default for public repositories
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (1)
packages/core/src/view/layout/ParallelEdgeLayout.ts(5 hunks)
🧰 Additional context used
🧠 Learnings (1)
📓 Common learnings
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.
🧬 Code graph analysis (1)
packages/core/src/view/layout/ParallelEdgeLayout.ts (1)
packages/core/src/view/cell/Cell.ts (1)
Cell(66-888)
⏰ 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 (windows-2022)
- GitHub Check: build (ubuntu-24.04)
- GitHub Check: build (macos-14)
- GitHub Check: build
🔇 Additional comments (3)
packages/core/src/view/layout/ParallelEdgeLayout.ts (3)
27-30: JSDoc scope clarification reads well.The updated class-level description improves clarity without altering semantics.
68-71: @default annotations: good addition.Explicit defaults help TS/IDE tooling.
74-77: @default annotations: good addition (checkOverlap).Docs now accurately reflect the runtime default.
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (2)
packages/core/src/view/layout/ParallelEdgeLayout.ts (2)
39-56: Example: switch to InternalEvent and strict equality is good; consider showing the importTo avoid confusion for readers, add the import in the snippet, e.g.,
import { InternalEvent } from '@maxgraph/core';.
134-134: Nit: use localviewconsistentlyMinor consistency polish; no behavior change.
- const state = this.graph.view.getState(edge); + const state = view.getState(edge);
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
💡 Knowledge Base configuration:
- MCP integration is disabled by default for public repositories
- Jira integration is disabled by default for public repositories
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (1)
packages/core/src/view/layout/ParallelEdgeLayout.ts(6 hunks)
🧰 Additional context used
🧠 Learnings (1)
📓 Common learnings
Learnt from: tbouffard
PR: maxGraph/maxGraph#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
PR: maxGraph/maxGraph#715
File: packages/html/stories/ZoomAndFit.stories.ts:0-0
Timestamp: 2025-03-14T06:01:22.326Z
Learning: For future improvements to the Graph component in maxGraph, tbouffard is interested in refactoring the `fit` method to use an object parameter pattern instead of multiple positional parameters, as the current approach makes the API harder to use when many parameters need to be specified with default values.
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.
🧬 Code graph analysis (1)
packages/core/src/view/layout/ParallelEdgeLayout.ts (1)
packages/core/src/internal/utils.ts (1)
isNullish(44-45)
⏰ 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 (windows-2022)
- GitHub Check: build (macos-14)
- GitHub Check: build (ubuntu-24.04)
🔇 Additional comments (8)
packages/core/src/view/layout/ParallelEdgeLayout.ts (8)
25-25: LGTM: dedicated nullish check importImport is correct and used appropriately. Keeping scope in mind, no further change needed.
28-31: Docs clarity improvedMore precise description; no functional impact.
69-71: Good: @default for spacingMatches implementation.
75-77: Good: @default for checkOverlapDocs match behavior.
111-114: Truthiness check simplification is fine
[]is truthy; behavior preserved.
98-101: No findParallels overrides detected; signature change safe
Typed return and null-prototype lookup are appropriate, and no subclasses override findParallels in the codebase.
128-132: No externalgetEdgeIdcallers; null is already handled Internal search shows only the self-call at line 103, which is guarded byisNullish(id). No downstream code assumes a non-nullable string.
87-87: Iteration via Object.values is cleaner. packages/core inherits from the root tsconfig (target ES2020, lib ES2020), so compile-time support for Object.values is already present; ensure your runtime supports ES2017 features or include a polyfill.
| if (!isNullish(id)) { | ||
| (lookup[id] ??= []).push(cell); | ||
| } |
There was a problem hiding this comment.
💡 Verification agent
🧩 Analysis chain
Nullish coalescing assignment (??=) may require ES2021 target
If build target < ES2021, replace with an older-safe pattern.
Check precedent and tsconfig:
Fallback change if needed:
- (lookup[id] ??= []).push(cell);
+ const bucket = lookup[id] || (lookup[id] = []);
+ bucket.push(cell);🏁 Script executed:
#!/bin/bash
rg -nF '??=' --glob '!**/node_modules/**'
rg -n --glob '!**/node_modules/**' -S '\b"target"\s*:\s*"ES' tsconfig*.json packages/**/tsconfig*.jsonLength of output: 1108
🏁 Script executed:
#!/bin/bash
rg -n '"target"' --glob 'tsconfig*.json' tsconfig.json packages/**/tsconfig*.jsonLength of output: 501
Nullish coalescing assignment (??=) requires ES2021
Our root tsconfig.json and several package configs target ES2020 (and ES2015 in ts-support), so ??= will be emitted unchanged and isn’t supported at runtime. Either bump "target" to "ES2021" in tsconfig.json (and package configs) or replace this line with a safe pattern:
- (lookup[id] ??= []).push(cell);
+ const bucket = lookup[id] || (lookup[id] = []);
+ bucket.push(cell);📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| if (!isNullish(id)) { | |
| (lookup[id] ??= []).push(cell); | |
| } | |
| if (!isNullish(id)) { | |
| const bucket = lookup[id] || (lookup[id] = []); | |
| bucket.push(cell); | |
| } |
🤖 Prompt for AI Agents
In packages/core/src/view/layout/ParallelEdgeLayout.ts around lines 105-107, the
use of the nullish coalescing assignment operator (??=) is not supported by our
ES2020 runtime; either update the project tsconfig targets to ES2021 (and
package configs) so ??= is emitted safely, or replace the expression with an
explicit initialization: check whether lookup[id] is present and if not assign
an empty array, then push the cell; ensure the null check for id remains
unchanged.
There was a problem hiding this comment.
This is not a problem with the current TS code.
TypeScript generates the following JS code which is compatible with ES2020 (this is the target version).
(lookup[id] ?? (lookup[id] = [])).push(cell);There was a problem hiding this comment.
@tbouffard Thank you for the clarification! You're absolutely correct, and I apologize for the confusion in my analysis.
I failed to consider that TypeScript automatically transpiles the nullish coalescing assignment operator (??=) to ES2020-compatible code. As you've shown, the TypeScript compiler generates (lookup[id] ?? (lookup[id] = [])), which uses the nullish coalescing operator (??) that is indeed supported in ES2020, combined with standard assignment.
The original code using (lookup[id] ??= []).push(cell) is perfectly fine and doesn't require any changes. TypeScript handles the transpilation seamlessly while preserving the exact same runtime behavior.
(\ /)
( ._.)
o_(")(")
learned something new
🔥 Failed to add learnings
Learnings cannot be added due to server error. Please try again.
🧠 Learnings used
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.
Learnt from: tbouffard
PR: maxGraph/maxGraph#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
PR: maxGraph/maxGraph#715
File: packages/html/stories/ZoomAndFit.stories.ts:0-0
Timestamp: 2025-03-14T06:01:22.326Z
Learning: For future improvements to the Graph component in maxGraph, tbouffard is interested in refactoring the `fit` method to use an object parameter pattern instead of multiple positional parameters, as the current approach makes the API harder to use when many parameters need to be specified with default values.
|
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
packages/core/src/view/layout/ParallelEdgeLayout.ts (1)
134-160: Remove type assertions in getEdgeId by narrowing onceAvoids repeated
<string>casts and tightens types locally.Apply:
- let src: Cell | string | null = view.getVisibleTerminal(edge, true); - let trg: Cell | string | null = view.getVisibleTerminal(edge, false); + const srcTerm = view.getVisibleTerminal(edge, true); + const trgTerm = view.getVisibleTerminal(edge, false); @@ - if (src != null && trg != null) { - src = ObjectIdentity.get(src); - trg = ObjectIdentity.get(trg); + if (srcTerm != null && trgTerm != null) { + const srcId = ObjectIdentity.get(srcTerm); + const trgId = ObjectIdentity.get(trgTerm); @@ - return (<string>src > <string>trg ? `${trg}-${src}` : `${src}-${trg}`) + pts; + return (srcId > trgId ? `${trgId}-${srcId}` : `${srcId}-${trgId}`) + pts;
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
💡 Knowledge Base configuration:
- MCP integration is disabled by default for public repositories
- Jira integration is disabled by default for public repositories
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (1)
packages/core/src/view/layout/ParallelEdgeLayout.ts(7 hunks)
🧰 Additional context used
🧠 Learnings (1)
📓 Common learnings
Learnt from: tbouffard
PR: maxGraph/maxGraph#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
PR: maxGraph/maxGraph#715
File: packages/html/stories/ZoomAndFit.stories.ts:0-0
Timestamp: 2025-03-14T06:01:22.326Z
Learning: For future improvements to the Graph component in maxGraph, tbouffard is interested in refactoring the `fit` method to use an object parameter pattern instead of multiple positional parameters, as the current approach makes the API harder to use when many parameters need to be specified with default values.
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.
🧬 Code graph analysis (1)
packages/core/src/view/layout/ParallelEdgeLayout.ts (2)
packages/core/src/view/cell/Cell.ts (1)
Cell(66-888)packages/core/src/internal/utils.ts (1)
isNullish(44-45)
⏰ 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 (ubuntu-24.04)
- GitHub Check: build (macos-14)
- GitHub Check: build (windows-2022)
- GitHub Check: build
🔇 Additional comments (9)
packages/core/src/view/layout/ParallelEdgeLayout.ts (9)
25-25: Importing isNullish improves clarity and intent—LGTMGood choice to centralize null/undefined checks.
28-31: JSDoc clarifications read better and match behaviorNo functional change; improved readability.
39-56: Sample update to InternalEvent and strict equality is correctThe example now reflects the current event API and safe equality semantics.
69-79: Adding @default tags is helpful for API consumersNice doc enhancement without code noise.
86-93: Object.values over for-in on a null-proto dict—LGTMSafer enumeration (no inherited props) and more direct iteration of buckets.
98-100: Typed return and prototype-less lookup are appropriate
Record<string, Cell[]>plusObject.create(null)avoids prototype pitfalls and clarifies usage.
105-107: Bucket init with nullish coalescing assignment is concise and correctClean push-path; no redundant checks.
111-114: for-of over provided cells—LGTMKeeps intent clear; empty array remains truthy so behavior is unchanged.
127-132: Ignore nullability concern—no external callers ofgetEdgeId
Ripgrep showsgetEdgeIdis only invoked withinParallelEdgeLayout.tsat line 103, and its return value is already guarded byisNullish(id)before use. No downstream updates required.Likely an incorrect or invalid review comment.
Improve types in method signatures for clarity, and simplify implementation (default values and for loop).
Also improve JSDoc
Summary by CodeRabbit
Documentation
Refactor
Style
Note: No runtime behavior changes for end users.