refactor: prefer includes to indexOf #952
Conversation
From typescript-eslint Prior to ES2015, Array#indexOf and String#indexOf comparisons against -1 were the standard ways to check whether a value exists in an array or string, respectively. Alternatives that are easier to read and write now exist: ES2015 added String#includes and ES2016 added Array#includes .
WalkthroughAdded ESLint rule enforcing Changes
Estimated code review effort🎯 2 (Simple) | ⏱️ ~10 minutes
Possibly related PRs
Suggested labels
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
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 (1)
packages/core/src/Client.ts (1)
77-83: Membership checks correctly migrated toincludes, but consider guardingIS_FFThe refactors to use
includes/!includesforIS_NS,IS_ANDROID,IS_MT,NO_FO, andIS_LOCALpreserve the original indexOf logic and improve readability. One thing to consider:IS_FFnow (and likely previously) dereferencesnavigator.userAgentwithout thetypeof window !== 'undefined'/ null guard used by nearby flags, which can throw in non‑browser/SSR environments; aligning it with the other feature flags would make this safer:- static IS_FF = navigator.userAgent.toLowerCase().includes('firefox'); + static IS_FF = + typeof window !== 'undefined' && + navigator.userAgent != null && + navigator.userAgent.toLowerCase().includes('firefox');Based on learnings
Also applies to: 93-95, 122-122, 129-141, 153-159, 196-199
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (17)
eslint.config.mjs(1 hunks)packages/core/src/Client.ts(6 hunks)packages/core/src/gui/MaxLog.ts(1 hunks)packages/core/src/gui/MaxWindow.ts(1 hunks)packages/core/src/i18n/Translations.ts(1 hunks)packages/core/src/serialization/ObjectCodec.ts(1 hunks)packages/core/src/util/cloneUtils.ts(1 hunks)packages/core/src/util/domUtils.ts(1 hunks)packages/core/src/util/mathUtils.ts(5 hunks)packages/core/src/view/GraphSelectionModel.ts(1 hunks)packages/core/src/view/cell/Cell.ts(1 hunks)packages/core/src/view/layout/hierarchical/GraphHierarchyModel.ts(2 hunks)packages/core/src/view/layout/hierarchical/SwimlaneModel.ts(2 hunks)packages/core/src/view/mixins/CellsMixin.ts(2 hunks)packages/core/src/view/mixins/SwimlaneMixin.ts(1 hunks)packages/core/src/view/shape/Shape.ts(1 hunks)packages/core/src/view/style/edge/Manhattan.ts(2 hunks)
🧰 Additional context used
🧠 Learnings (3)
📓 Common learnings
Learnt from: tbouffard
Repo: maxGraph/maxGraph PR: 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
Repo: maxGraph/maxGraph PR: 776
File: packages/core/src/view/AbstractGraph.ts:492-495
Timestamp: 2025-04-24T12:34:56.726Z
Learning: PR #776 focuses on refactoring to introduce AbstractGraph as a base class and moving code from Graph to AbstractGraph. Type improvements and other refinements should be addressed in future PRs to keep this one focused on the core architectural change.
Learnt from: tbouffard
Repo: maxGraph/maxGraph PR: 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: 2024-11-23T16:12:51.380Z
Learnt from: tbouffard
Repo: maxGraph/maxGraph PR: 582
File: packages/core/src/util/config.ts:0-0
Timestamp: 2024-11-23T16:12:51.380Z
Learning: The `shallowCopy` function in `cloneUtils.ts` is intentionally designed to perform a shallow copy on objects that contain only primitive properties, and deep copying is not necessary.
Applied to files:
packages/core/src/util/cloneUtils.ts
📚 Learning: 2025-02-17T06:01:50.745Z
Learnt from: tbouffard
Repo: maxGraph/maxGraph PR: 679
File: packages/html/stories/Drop.stories.js:71-94
Timestamp: 2025-02-17T06:01:50.745Z
Learning: maxGraph does not support Internet Explorer, making IE-specific browser compatibility checks unnecessary.
Applied to files:
packages/core/src/Client.ts
🧬 Code graph analysis (1)
packages/core/src/view/mixins/CellsMixin.ts (1)
packages/html/stories/Clipboard.stories.js (1)
cells(190-190)
⏰ 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 (macos-14)
- GitHub Check: build (ubuntu-24.04)
- GitHub Check: build (windows-2022)
- GitHub Check: build
🔇 Additional comments (17)
packages/core/src/view/GraphSelectionModel.ts (1)
114-114: LGTM! Clean refactor toincludes.The change from
indexOf(cell) >= 0toincludes(cell)improves readability while maintaining identical behavior for the membership check.packages/core/src/view/mixins/SwimlaneMixin.ts (1)
269-269: LGTM! Correct negation withincludes.The transformation from
indexOf(parentCell) < 0to!includes(parentCell)is correct and improves code clarity.packages/core/src/view/shape/Shape.ts (1)
848-848: LGTM! Improved readability in rounded corners logic.The change to
!exclude.includes(i - 1)maintains the correct logic while being more idiomatic.packages/core/src/view/cell/Cell.ts (1)
518-518: LGTM! Clear membership check before insertion.The refactor to
!this.edges.includes(edge)correctly preserves the duplicate-prevention logic.packages/core/src/util/domUtils.ts (1)
71-71: LGTM! More readable block element check.The change to
blocks.includes(elts[i + 1].nodeName)is correct and improves code clarity.packages/core/src/util/cloneUtils.ts (1)
46-46: LGTM! Clearer transient field check.The refactor to
!transients.includes(i)maintains the correct cloning logic with improved readability.packages/core/src/view/layout/hierarchical/GraphHierarchyModel.ts (1)
107-107: LGTM! Consistent edge membership checks.Both changes correctly refactor the duplicate-edge prevention logic to use
!includes(internalEdge), improving code consistency and readability.Also applies to: 241-241
packages/core/src/i18n/Translations.ts (1)
119-119: LGTM! More idiomatic language support check.The change to
languages.includes(lan)is correct and aligns with modern JavaScript best practices.packages/core/src/view/style/edge/Manhattan.ts (1)
113-116: String/array membership refactor looks behavior‑preservingUsing
!pointString.includes('@')for the delimiter andendPointsKeys.includes(currentKey)for endpoint membership keeps the original semantics while improving readability.Also applies to: 479-489
packages/core/src/view/layout/hierarchical/SwimlaneModel.ts (1)
100-110:includesusage keeps the edge de‑duplication semantics intactSwitching from
indexOf(...) < 0to!array.includes(...)forconnectsAsTargetandconnectsAsSourcechecks maintains the original “only push if not already present” behavior and is easier to read.Also applies to: 253-258
eslint.config.mjs (1)
94-109: ESLint rule addition aligns config with the refactorEnabling
'unicorn/prefer-includes': 'error'forpackages/core/src/**/*is consistent with this PR’s changes and will prevent regressions back toindexOf-based membership checks.packages/core/src/gui/MaxWindow.ts (1)
388-396: Presto user‑agent guard refactor preserves the workaroundThe updated condition
!navigator.userAgent || !navigator.userAgent.includes('Presto/2.5')keeps the original “skip overflow styling only on Presto 2.5.x” behavior while making the intent clearer.packages/core/src/serialization/ObjectCodec.ts (1)
335-337:isExcludedrefactor is semantically equivalentUsing
this.exclude.includes(attr)instead of an indexOf comparison preserves the exclusion behavior and keeps the code consistent with the rest of the PR.packages/core/src/gui/MaxLog.ts (1)
333-343: Presto textarea refresh condition correctly migrated toincludesThe updated
navigator.userAgent && navigator.userAgent.includes('Presto/2.5')check retains the original workaround behavior while following the new includes style.packages/core/src/util/mathUtils.ts (1)
236-320:getPortConstraintsandisNumericrefactors keep existing behaviorThe direction checks in
getPortConstraintsnow usedirections.includes('north' | 'west' | 'south' | 'east'), which is equivalent to the old indexOf checks, and theisNumericguard(typeof n !== 'string' || !n.toLowerCase().includes('0x'))preserves the prior “exclude hex-like strings” logic while matching the new includes style.Also applies to: 673-678
packages/core/src/view/mixins/CellsMixin.ts (2)
2057-2057: LGTM! Clean refactor to modern array membership check.The change from
indexOf(parent) >= 0toincludes(parent)is correct and improves readability while preserving the existing logic.
2066-2066: LGTM! Consistent refactor for array membership check.The change from
indexOf(parent) >= 0toincludes(parent)is correct and maintains consistency with the refactor pattern applied throughout this PR.
|
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
packages/core/src/util/domUtils.ts (1)
150-150: These null check changes appear unrelated to the PR's objective.The changes from
if (parent != null)toif (parent)refactor explicit null checks to truthiness checks. While low-risk given theElementtype, this represents a different type of refactoring than theindexOf→includesmodernization that is the focus of this PR.Consider either reverting these changes to keep the PR focused on its stated objective, or documenting why they're included.
Based on learnings (user prefers focused PRs).
Also applies to: 168-168
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
packages/core/src/gui/MaxWindow.ts(1 hunks)packages/core/src/util/domUtils.ts(4 hunks)
🧰 Additional context used
🧠 Learnings (1)
📓 Common learnings
Learnt from: tbouffard
Repo: maxGraph/maxGraph PR: 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
Repo: maxGraph/maxGraph PR: 776
File: packages/core/src/view/AbstractGraph.ts:492-495
Timestamp: 2025-04-24T12:34:56.726Z
Learning: PR #776 focuses on refactoring to introduce AbstractGraph as a base class and moving code from Graph to AbstractGraph. Type improvements and other refinements should be addressed in future PRs to keep this one focused on the core architectural change.
Learnt from: tbouffard
Repo: maxGraph/maxGraph PR: 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 (windows-2022)
- GitHub Check: build (ubuntu-24.04)
- GitHub Check: build (macos-14)
🔇 Additional comments (3)
packages/core/src/gui/MaxWindow.ts (1)
388-397: LGTM! Clean refactor with optional chaining.The refactor from
indexOftoincludeswith optional chaining correctly preserves the original behavior while improving readability. The logic properly skips overflow styling whennavigator.userAgentis present and contains'Presto/2.5', and enables it otherwise.packages/core/src/util/domUtils.ts (2)
20-35: Excellent modernization to use Set for membership checks.The change from an array to a Set for
blocksis a solid improvement. Set provides O(1) lookup time compared to O(n) for array-basedindexOf, and it's semantically clearer for membership testing of this static collection.
72-72: LGTM: Correct use of Set.has() for membership check.This change aligns perfectly with the PR's objective. Using
blocks.has()is more readable and efficient than the previousindexOfcomparison.
From typescript-eslint
Prior to ES2015, Array#indexOf and String#indexOf comparisons against -1 were the standard ways to check whether a
value exists in an array or string, respectively.
Alternatives that are easier to read and write now exist: ES2015 added String#includes and ES2016 added Array#includes
Summary by CodeRabbit
Refactor
Chores