Conversation
Such functions are now easier to identify, so this should help for the maintenance and should prevent from exporting them by mistake. BREAKING CHANGES: `Utils.copyTextToClipboard` is no longer available. It was intended to be internal and had been made public by mistake.
WalkthroughThe pull request restructures several utility functions across the codebase. Functions formerly exposed in public modules—such as copyTextToClipboard and matchBinaryMask—have been removed or relocated. New implementations for clipboard copy, shallow copying, elapsed time messaging, and additional utility functions were introduced in internal modules, with corresponding updates to JSDoc comments and visibility annotations. Import paths have been updated in many files across editors, views, serialization, and tests. Deprecated functions were removed from legacy utility modules, and a dedicated test suite for matchBinaryMask was added. Changes
Sequence Diagram(s)sequenceDiagram
participant User as User
participant MaxLog as MaxLog.ts
participant Clipboard as navigator.clipboard
User->>MaxLog: Trigger copy text action
MaxLog->>Clipboard: writeText(text)
Clipboard-->>MaxLog: Return success/error
MaxLog->>MaxLog: Log result and provide feedback
MaxLog-->>User: Update UI based on copy outcome
Possibly related PRs
📜 Recent review detailsConfiguration used: CodeRabbit UI 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
⏰ Context from checks skipped due to timeout of 90000ms (1)
🪧 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: 2
🧹 Nitpick comments (2)
packages/core/src/view/mixins/_graph-mixins-apply.ts (1)
17-17: Updated Import Path for Internal Utilities
ThemixIntofunction is now imported from the new internal location (../../internal/utils), which is consistent with the overall project refactor. Please verify that the new module correctly exportsmixInto.packages/core/src/internal/clone-utils.ts (1)
36-37: Consider using a more type-safe approach for array assignment.The current implementation uses a type assertion (
@ts-ignore) to bypass TypeScript's type checking for array assignment. Consider using a more type-safe approach or providing a more specific explanation in the comment about why the type assertion is necessary.- // @ts-ignore source and target are of the same type - target[key] = [...sourceValue]; + // TypeScript cannot infer that the key in target will also be an array + // when source and target are of the same type + (target[key] as unknown[]) = [...sourceValue];
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (37)
CHANGELOG.md(1 hunks)packages/core/__tests__/internal/utils.test.ts(1 hunks)packages/core/__tests__/util/styleUtils.test.ts(0 hunks)packages/core/src/editor/Editor.ts(1 hunks)packages/core/src/editor/EditorPopupMenu.ts(1 hunks)packages/core/src/gui/MaxLog.ts(1 hunks)packages/core/src/i18n/config.ts(1 hunks)packages/core/src/index.ts(1 hunks)packages/core/src/internal/clone-utils.ts(1 hunks)packages/core/src/internal/time-utils.ts(1 hunks)packages/core/src/internal/types.ts(0 hunks)packages/core/src/internal/utils.ts(1 hunks)packages/core/src/serialization/Codec.ts(1 hunks)packages/core/src/serialization/ObjectCodec.ts(1 hunks)packages/core/src/serialization/codecs/CellCodec.ts(1 hunks)packages/core/src/serialization/codecs/ChildChangeCodec.ts(1 hunks)packages/core/src/serialization/codecs/RootChangeCodec.ts(1 hunks)packages/core/src/serialization/codecs/StylesheetCodec.ts(1 hunks)packages/core/src/serialization/codecs/editor/EditorToolbarCodec.ts(1 hunks)packages/core/src/util/Utils.ts(0 hunks)packages/core/src/util/cloneUtils.ts(0 hunks)packages/core/src/util/config.ts(1 hunks)packages/core/src/util/domUtils.ts(0 hunks)packages/core/src/util/logger.ts(1 hunks)packages/core/src/util/mathUtils.ts(1 hunks)packages/core/src/util/styleUtils.ts(1 hunks)packages/core/src/util/xmlUtils.ts(1 hunks)packages/core/src/view/Graph.ts(1 hunks)packages/core/src/view/canvas/SvgCanvas2D.ts(1 hunks)packages/core/src/view/cell/Cell.ts(1 hunks)packages/core/src/view/geometry/Shape.ts(1 hunks)packages/core/src/view/geometry/node/StencilShape.ts(1 hunks)packages/core/src/view/geometry/node/TextShape.ts(2 hunks)packages/core/src/view/handler/config.ts(1 hunks)packages/core/src/view/mixins/_graph-mixins-apply.ts(1 hunks)packages/core/src/view/style/config.ts(1 hunks)packages/core/src/view/undoable_changes/CellAttributeChange.ts(1 hunks)
💤 Files with no reviewable changes (5)
- packages/core/tests/util/styleUtils.test.ts
- packages/core/src/util/cloneUtils.ts
- packages/core/src/internal/types.ts
- packages/core/src/util/domUtils.ts
- packages/core/src/util/Utils.ts
🧰 Additional context used
🧠 Learnings (2)
packages/core/src/util/config.ts (1)
Learnt from: tbouffard
PR: maxGraph/maxGraph#582
File: packages/core/src/util/config.ts:0-0
Timestamp: 2025-03-27T09:34:54.875Z
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.
packages/core/src/internal/clone-utils.ts (1)
Learnt from: tbouffard
PR: maxGraph/maxGraph#582
File: packages/core/src/util/config.ts:0-0
Timestamp: 2025-03-27T09:34:54.875Z
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.
🧬 Code Definitions (2)
packages/core/__tests__/internal/utils.test.ts (1)
packages/core/src/internal/utils.ts (1)
matchBinaryMask(68-70)
packages/core/src/internal/utils.ts (2)
packages/core/src/internal/types.ts (1)
UserObject(20-27)packages/core/src/util/config.ts (1)
GlobalConfig(34-54)
🪛 LanguageTool
CHANGELOG.md
[style] ~14-~14: ‘by mistake’ might be wordy. Consider a shorter alternative.
Context: ...to be internal and had been made public by mistake. ## 0.16.0 Release date: 2025-03-02...
(EN_WORDINESS_PREMIUM_BY_MISTAKE)
🪛 Biome (1.9.4)
packages/core/src/internal/utils.ts
[error] 26-26: eval() exposes to security risks and performance issues.
See the MDN web docs for more details.
Refactor the code so that it doesn't need to call eval().
(lint/security/noGlobalEval)
🔇 Additional comments (40)
CHANGELOG.md (1)
14-14: Breaking change properly documentedThe removal of
Utils.copyTextToClipboardis correctly documented as a breaking change, with clear explanation that it was intended to be internal and had been made public by mistake.🧰 Tools
🪛 LanguageTool
[style] ~14-~14: ‘by mistake’ might be wordy. Consider a shorter alternative.
Context: ...to be internal and had been made public by mistake. ## 0.16.0 Release date:2025-03-02...(EN_WORDINESS_PREMIUM_BY_MISTAKE)
packages/core/src/util/logger.ts (1)
19-19: Import from internal directoryThe function
getElapseMillisecondsMessagehas been properly moved to an internal utility module and is now imported here. This is consistent with the PR's objective of reorganizing internal utility functions.packages/core/src/gui/MaxLog.ts (5)
25-25: Import path updated to use internal utilities directoryThe import path for
getElapseMillisecondsMessagehas been updated to use the new internal utilities directory structure, which aligns with the PR objective of moving internal utility functions to a dedicated directory.
27-27: Added import for GlobalConfigNew import for
GlobalConfigto access the logger for the clipboard operations, which is a good practice for consistent logging throughout the application.
29-38: Reimplemented copyTextToClipboard as an internal functionThis function previously existed in
../util/Utilsand was mistakenly exposed as a public API. The new implementation:
- Uses modern
navigator.clipboard.writeTextAPI- Properly handles success and error cases with logging
- Is now properly scoped as an internal function within this file
This change directly addresses the PR objective of removing the public
Utils.copyTextToClipboardfunction.
119-125: Updated to use the local copyTextToClipboard implementationThe "Copy" button handler now uses the locally defined clipboard function, maintaining the same functionality while removing the dependency on the previously public utility.
284-287: Utilizing the relocated utility functionThis section now uses the
getElapseMillisecondsMessagefunction from its new location in the internal utilities directory, maintaining the same functionality with the reorganized structure.packages/core/src/util/config.ts (1)
25-25: Update Import Path for shallowCopy
The import ofshallowCopyhas been updated from its old location to use the new internal module path (../internal/clone-utils). This change aligns well with the overall internal reorganization and maintains consistency across the codebase.packages/core/src/view/handler/config.ts (1)
31-31: Update shallowCopy Import in Handler Config
The import statement forshallowCopyis now referencing../../internal/clone-utilsinstead of the old path. This update is consistent with the broader refactor and helps ensure that internal utilities are properly encapsulated.packages/core/src/serialization/codecs/ChildChangeCodec.ts (1)
20-20: Reorganize isElement Import
The import forisElementhas been moved to../../internal/utils, reflecting the new home for internal utility functions. This prevents accidental public exposure and consolidates the utilities.packages/core/src/view/Graph.ts (1)
64-64: Update isNullish Import
TheisNullishfunction is now imported from../internal/utils, which is in line with the internal reorganization. This change does not impact functionality but improves the overall module structure.packages/core/src/view/geometry/node/StencilShape.ts (2)
31-31: Retain Legacy DOM Utilities for Child Nodes
The import forgetChildNodesandgetTextContentremains from'../../../util/domUtils'. This is appropriate because these functions are still maintained in their current module and are separate from the internal utility functions now consolidated ininternal/utils.
35-35: Update Utility Imports for doEval, isElement, and isNullish
The import ofdoEval,isElement, andisNullishhas been updated to come from../../../internal/utils. This reorganization consolidates these utility functions under a single internal module, enhancing maintainability and encapsulation.packages/core/src/util/mathUtils.ts (1)
24-24: Updated import path forisNullishfunction.The import path for
isNullishhas been correctly updated to source from the new internal utils directory, which aligns with the PR objective of relocating internal utility functions to a dedicated directory.packages/core/src/serialization/Codec.ts (1)
24-25: Refactored imports to separate internal from utility functions.The imports have been properly reorganized, keeping
importNodeandisNodein the existingdomUtilswhile movingisElementto the new internal utils module. This clean separation helps clarify which utilities are meant for internal use.packages/core/src/serialization/codecs/RootChangeCodec.ts (1)
20-20: Updated import path forisElementfunction.The import path for
isElementhas been correctly updated to source from the internal utils directory, consistent with the same change in other files and aligning with the PR objective.packages/core/src/view/style/config.ts (1)
18-18: Updated import path forshallowCopyfunction.The import path for
shallowCopyhas been properly updated to use the new location in the internal directory. The naming convention change fromcloneUtilstoclone-utilsis also consistent with internal module naming standards.packages/core/__tests__/internal/utils.test.ts (2)
17-20: New test file correctly imports dependencies.The imports are properly set up to test the
matchBinaryMaskfunction that has been moved to the internal utils module, with appropriate test dependencies and constants.
21-34: Well-structured tests for thematchBinaryMaskfunction.The test suite provides good coverage for the binary mask matching functionality, including both positive and negative test cases. This ensures that the function behaves correctly after being moved to the internal utils module.
packages/core/src/i18n/config.ts (1)
17-18: Updated Imports to Use Internal Utilities for i18n Configuration
The import statements forisNullishandshallowCopyhave been updated to use the new internal paths. This change aligns with the objective of consolidating internal utilities. Ensure that both../internal/utilsand../internal/clone-utilscorrectly export these functions and that downstream functionality remains unaffected.packages/core/src/editor/Editor.ts (1)
61-61: Refactored Import forisNullish
TheisNullishfunction import has been modified to use../internal/utilsinstead of the old public utility path. This change is in line with the refactoring objective. Ensure that any other usages expecting the old implementation (if any) are updated accordingly.packages/core/src/view/undoable_changes/CellAttributeChange.ts (1)
17-17: Revised Import forisNullishin CellAttributeChange
The import ofisNullishis now sourced from../../internal/utils, which is consistent with the internal reorganization. The logic inexecute()remains intact, so no functional changes are expected.packages/core/src/serialization/ObjectCodec.ts (1)
24-27: Updated Utility Imports in ObjectCodec
The changes update the import ofdoEvalandisElementfrom the new internal utilities location (../internal/utils). This ensures consistency across the codebase. Please verify that theisElementfunction correctly distinguishes element nodes and thatdoEvalbehaves as expected when evaluating expressions during decoding.packages/core/src/view/geometry/node/TextShape.ts (1)
35-35: Import reorganization as part of internal utils refactoring.The
matchBinaryMaskutility function has been moved fromstyleUtilsto the new dedicatedinternal/utilsmodule as part of the codebase reorganization.Also applies to: 51-51
packages/core/src/editor/EditorPopupMenu.ts (1)
25-25: Consolidated import statement for internal utilities.The
doEvalandisNullishutility functions are now both imported from theinternal/utilsmodule, aligning with the refactoring effort to move internal utility functions to a dedicated directory.packages/core/src/serialization/codecs/editor/EditorToolbarCodec.ts (1)
25-27: Reorganized import forisElementutility function.The
isElementfunction has been moved fromdomUtilstointernal/utilsas part of the refactoring effort to consolidate internal utility functions, whilegetChildNodesandgetTextContentremain in their original location.packages/core/src/util/xmlUtils.ts (1)
26-28: RelocatedisElementutility function import.The
isElementfunction is now imported frominternal/utilsinstead ofdomUtils, consistent with the refactoring to move internal utility functions to a dedicated directory, making them explicitly marked as internal utilities.packages/core/src/serialization/codecs/CellCodec.ts (1)
21-23: Import reorganization aligns with the refactoring goal.The change moves
isElementfrom a public utility location to the new internal utilities directory while keepingimportNodein its original location. This supports the PR's objective of relocating internal utility functions to improve maintenance and prevent accidental exports.packages/core/src/serialization/codecs/StylesheetCodec.ts (1)
24-25: Import reorganization aligns with internal utilities refactoring.The refactoring moves
doEvalandisElementto the internal utilities directory while maintaininggetTextContentin its original location. This is consistent with the PR's objective of better organizing utility functions and ensuring they aren't unintentionally exposed.packages/core/src/view/geometry/Shape.ts (1)
20-20: Import path updated to use internal utils directory.The
isNullishutility has been correctly moved fromUtilsto the new internal utilities directory, which aligns with the PR's refactoring goals.packages/core/src/internal/clone-utils.ts (1)
17-43: Well-implemented utility with proper documentation and security considerations.The new
shallowCopyutility function is well-documented with JSDoc comments including warnings about its shallow copy behavior. It also includes proper security measures to prevent prototype pollution by checking for own properties.The implementation correctly handles arrays by creating shallow copies while directly assigning other property types. The
@privateannotation appropriately signals that this is an internal utility not intended for public consumption.packages/core/src/util/styleUtils.ts (1)
34-34: Refactoring successfully relocates utility function.The import of
matchBinaryMaskfrom the internal utils module aligns with the PR's objective to relocate internal utilities to a dedicated directory, improving organization and reducing the risk of inadvertently exporting them.packages/core/src/view/cell/Cell.ts (1)
25-26: Import paths successfully updated.The import path changes for
UserObjectand utility functionsisElementandisNullishalign with the PR objective of moving internal utilities to a dedicated directory, enhancing maintainability.packages/core/src/view/canvas/SvgCanvas2D.ts (1)
19-21: Properly updated utility import paths.The imports have been correctly reorganized to reflect the new location of utility functions, with
isNullishandmatchBinaryMasknow coming from the internal utils module while maintaining the import ofgetAlignmentAsPointfrom styleUtils.packages/core/src/internal/utils.ts (6)
17-20: Appropriate imports for new utility module.These imports correctly bring in the necessary dependencies for the utility functions that follow. The import of
GlobalConfigenables proper error logging in themixIntofunction.
29-35: Well-documented Element type check implementation.This utility function properly checks if a node is an Element by comparing its nodeType to the Element constant, with appropriate TypeScript type narrowing. The
@privateannotation clearly indicates this is an internal utility.
36-41: Simple but useful nullish check utility.This function provides a standardized way to check for null or undefined values across the codebase, which helps maintain consistency. The
@privateannotation properly indicates its internal nature.
42-60: Robust mixin implementation with error handling.The
mixIntoutility provides a clean way to merge properties from a mixin into a destination class. It includes proper error handling by logging errors throughGlobalConfig.loggerrather than failing silently or throwing exceptions.
62-70: Binary mask utility properly implemented.The
matchBinaryMaskfunction, which was moved from styleUtils, is correctly implemented to check if a value matches a binary mask. This is essential for font style checks and other bitwise operations throughout the codebase.
21-27:Details
❓ Verification inconclusive
Security consideration with eval usage.
The
doEvalfunction useseval(), which poses security risks as identified by static analysis. While the eslint comment acknowledges this, consider if this function is truly necessary or if there are safer alternatives.
🏁 Script executed:
#!/bin/bash # Check for usage of the doEval function in the codebase echo "Checking for doEval usage in the codebase..." rg -A 3 -B 3 "doEval\("Length of output: 5734
Security Alert: Revisit and Validate All Eval Usages
The
doEvalfunction inpackages/core/src/internal/utils.tsstill useseval(), which introduces critical security risks if inputs aren’t strictly controlled. Our search shows thatdoEvalis invoked in multiple modules (e.g., inStylesheetCodec.ts,ObjectCodec.ts,EditorToolbarCodec.ts,StencilShape.ts,GraphView.ts, andEditorPopupMenu.ts). Although each use is gated by corresponding flags (likeallowEval), this pattern warrants extra scrutiny.Please consider the following:
- Input Sanitization: Double-check that all strings passed to
doEvalare properly sanitized before evaluation.- Necessity versus Alternatives: Reassess if using
eval()is truly necessary. If possible, explore safer alternatives such as a controlled DSL or custom parser.- Documentation & Flag Enforcement: Enhance documentation around the use of
doEvaland ensure that the associatedallowEvalflags reliably prevent misuse.Review these aspects across all impacted modules to mitigate potential security vulnerabilities.
🧰 Tools
🪛 Biome (1.9.4)
[error] 26-26: eval() exposes to security risks and performance issues.
See the MDN web docs for more details.
Refactor the code so that it doesn't need to call eval().(lint/security/noGlobalEval)
|
Such functions are now easier to identify, so this should help for the maintenance and should prevent from exporting
them by mistake.
BREAKING CHANGES:
Utils.copyTextToClipboardis no longer available. It was intended to be internal and had been made public by mistake.Summary by CodeRabbit