refactor: simplify the reset config functions #582
Conversation
Streamline the logic for resetting configuration functions: do not manually reset properties of the configuration value objects. Improve code readability and maintainability: prevent from missing properties Remove redundant code: use a shared function for the properties copy.
WalkthroughThis pull request introduces new test files for configuration reset functionality in the core package and modifies existing utility and configuration files. Two test files are added to validate the reset operations for style defaults and various handler configurations. Additionally, a new utility function Changes
📜 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 (
|
coderabbitai
bot
changed the title
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Outside diff range and nitpick comments (2)
packages/core/src/util/cloneUtils.ts (1)
71-75: Consider the impact on configuration reset patternsThe introduction of this utility function aligns well with the PR's goal of simplifying configuration reset operations. However, consider the following architectural points:
- Since this function is crucial for configuration management, consider adding specific error messages that help identify which configuration reset operation failed
- Consider creating a dedicated configuration utilities module if more configuration-related functions are added in the future
- Document any assumptions about the structure of configuration objects that this function expects
packages/core/__tests__/view/handler/config.test.ts (1)
27-40: Consider enhancing test coverage and maintainability.While the test correctly verifies the reset functionality, consider these improvements:
- Test more properties to ensure comprehensive coverage
- Extract magic values like '#0000FF' into named constants
- Add negative test cases (e.g., invalid color values)
Example refactor:
+const DEFAULT_CONNECT_FILL_COLOR = '#0000FF'; +const DEFAULT_SELECTION_STROKE_WIDTH = 1; test('resetEdgeHandlerConfig', () => { // Ensure that some default values are correctly set - expect(EdgeHandlerConfig.connectFillColor).toBe('#0000FF'); - expect(EdgeHandlerConfig.selectionStrokeWidth).toBe(1); + expect(EdgeHandlerConfig.connectFillColor).toBe(DEFAULT_CONNECT_FILL_COLOR); + expect(EdgeHandlerConfig.selectionStrokeWidth).toBe(DEFAULT_SELECTION_STROKE_WIDTH); // Add more property checks + expect(EdgeHandlerConfig.handleSize).toBeDefined(); + expect(EdgeHandlerConfig.strokeWidth).toBeDefined();
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (5)
packages/core/__tests__/util/config.test.ts(1 hunks)packages/core/__tests__/view/handler/config.test.ts(1 hunks)packages/core/src/util/cloneUtils.ts(1 hunks)packages/core/src/util/config.ts(3 hunks)packages/core/src/view/handler/config.ts(6 hunks)
🔇 Additional comments (13)
packages/core/__tests__/util/config.test.ts (3)
1-15: LGTM: License header is properly formatted
The Apache 2.0 license header is correctly implemented with appropriate copyright information.
17-18: LGTM: Imports are appropriate and minimal
The imports are well-structured, importing only the necessary test utilities and the specific configuration components being tested.
1-33: Verify complete test coverage of reset functionality
While this test file covers the StyleDefaultsConfig reset, we should ensure all reset configurations mentioned in the PR objectives are properly tested.
Let's check for other related test files:
✅ Verification successful
Let me check the content of the other test file to understand its coverage.
Let me find all the reset configuration functions to ensure complete coverage.
Test coverage for reset configuration functions is complete
All reset configuration functions are properly tested:
resetStyleDefaultsConfiginpackages/core/__tests__/util/config.test.tsresetEdgeHandlerConfig,resetHandleConfig, andresetVertexHandlerConfiginpackages/core/__tests__/view/handler/config.test.ts
Each test follows the same pattern of verifying default values, modifying them, and ensuring they are correctly reset. The test coverage aligns perfectly with all reset functions found in the source code.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Find all test files related to reset configuration
fd -e test.ts -e spec.ts | xargs rg -l "reset.*Config"
# Find all reset configuration functions that might need tests
ast-grep --pattern 'function reset$_Config'
Length of output: 198
Script:
#!/bin/bash
# Check content of the other test file
cat packages/core/__tests__/view/handler/config.test.ts
# Also find all resetConfig functions to ensure we're testing everything
rg "export function reset.*Config" -A 2
Length of output: 2450
Script:
#!/bin/bash
# Search for all exported reset config functions in the source
rg "export (const|function) reset.*Config" --type ts --glob "!**/*.test.ts"
# Also check for any Config interfaces/types to ensure we haven't missed any
rg "export (interface|type) \w+Config" --type ts
Length of output: 491
packages/core/src/util/cloneUtils.ts (2)
60-70: Documentation looks good!
The JSDoc is well-structured and includes:
- Clear description
- Type template parameter documentation
- Parameter descriptions
- Private API marking
- Version information
71-75: 🛠️ Refactor suggestion
Consider enhancing the robustness of the copyProperties function
While the implementation is concise, it could benefit from additional safety checks and proper property handling.
Consider this enhanced implementation:
-export const copyProperties = <T>(source: T, target: T) => {
- for (const key in source) {
- target[key] = source[key];
- }
-};
+export const copyProperties = <T extends object>(source: T, target: T): void => {
+ if (!source || !target) {
+ throw new Error('Source and target objects must be defined');
+ }
+
+ Object.keys(source).forEach((key) => {
+ if (Object.prototype.hasOwnProperty.call(source, key)) {
+ target[key] = source[key];
+ }
+ });
+};Changes explained:
- Added
extends objectconstraint to ensure T is an object type - Added null/undefined checks for parameters
- Used
Object.keys()instead offor...into avoid inherited properties - Added
hasOwnPropertycheck for extra safety against prototype pollution - Added return type annotation for better type safety
Note: Since this function is used for config reset operations, you might also want to consider:
- Adding deep cloning for nested objects if needed
- Adding specific type constraints based on your config object structures
Let's verify the usage context:
packages/core/__tests__/view/handler/config.test.ts (2)
1-26: LGTM! Well-structured imports and proper license header.
The file starts with a properly formatted Apache 2.0 license header and well-organized imports.
1-70: Verify complete coverage of reset functionality.
The tests provide good basic coverage of the reset functions, but let's verify if all reset functionality mentioned in the PR is covered.
✅ Verification successful
Tests cover all reset functionality
After reviewing the core configuration files, I can confirm that all reset functions are properly tested. The test file covers:
resetEdgeHandlerConfigresetHandleConfigresetVertexHandlerConfig
The only other reset function in the codebase is resetStyleDefaultsConfig, but it's in a different module (util/config.ts) and should be tested separately.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for any other reset functions in the codebase that might need tests
ast-grep --pattern 'function reset$_Config'
# Look for config-related files that might need reset functionality
fd -e ts -e js "config\.(ts|js)$"
Length of output: 407
Script:
#!/bin/bash
# Let's check the content of the core config files to see all config-related functionality
cat packages/core/src/util/config.ts packages/core/src/view/handler/config.ts
# Also search for any Config class declarations to ensure we're testing all resettable configs
ast-grep --pattern 'class $_Config'
Length of output: 8569
packages/core/src/util/config.ts (2)
86-86: LGTM! Shallow copy is sufficient here.
The spread operator creates a shallow copy of StyleDefaultsConfig, which is appropriate since all properties are primitives (strings and numbers).
95-95: Verify complete property coverage in the reset operation
The refactored reset implementation looks cleaner. Let's verify it resets all properties defined in StyleDefaultsConfig.
✅ Verification successful
Let me gather more information about the default configuration to ensure complete coverage.
Let me try a different pattern to locate these implementations.
Based on the gathered information, I can now provide the final response:
Property coverage is properly implemented in the reset operation
The implementation is correct and safe because:
StyleDefaultsConfigcontains all required properties (shadowColor, shadowOffsetX, shadowOffsetY, shadowOpacity)defaultStyleDefaultsConfigis created as a shallow copy ofStyleDefaultsConfigcopyPropertiesutility performs a complete property transfer using a for-in loop- Test coverage verifies the reset functionality for multiple properties
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Verify all StyleDefaultsConfig properties are properly reset
# Expected: All properties in StyleDefaultsConfig should be included in tests
# Find all properties of StyleDefaultsConfig
ast-grep --pattern 'export const StyleDefaultsConfig = {
$$$
}'
# Search for corresponding test coverage
rg -A 5 "resetStyleDefaultsConfig"
Length of output: 3291
Script:
#!/bin/bash
# Find the default configuration object
ast-grep --pattern 'export const defaultStyleDefaultsConfig = {
$$$
}'
# Find the copyProperties implementation
ast-grep --pattern 'function copyProperties($_, $_) {
$$$
}'
Length of output: 139
Script:
#!/bin/bash
# Find the default configuration and copyProperties
rg -A 10 "defaultStyleDefaultsConfig ="
rg -A 10 "copyProperties.*="
rg -A 10 "function copyProperties"
Length of output: 1216
packages/core/src/view/handler/config.ts (4)
31-31: LGTM! Import of shared utility function.
The addition of copyProperties aligns with the PR's goal of reducing redundancy in configuration reset logic.
Line range hint 34-40: Verify consistent experimental API marking.
All configuration objects and reset functions are correctly marked as experimental with appropriate warnings about potential changes.
Let's verify that all exported entities have consistent experimental marking:
Also applies to: 78-84, 119-125, 166-172
✅ Verification successful
Let me search for all configuration objects and their experimental marking.
Consistent experimental API marking confirmed.
All configuration objects in the codebase are properly marked with @experimental and include appropriate warnings about potential changes. This includes:
- EdgeHandlerConfig and related reset functions
- HandleConfig and related reset functions
- VertexHandlerConfig and related reset functions
- StencilShapeConfig
- GlobalConfig
- StyleDefaultsConfig and related reset functions
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for exported entities and their experimental marking
rg -B 3 "export const" | rg -B 3 "@experimental|export"
Length of output: 93354
Script:
#!/bin/bash
# Search for configuration objects and their experimental marking
rg -B 3 "@category Configuration" | rg -B 3 "@experimental|@category"
Length of output: 5913
73-74: LGTM! Consistent implementation of reset functions.
The refactoring successfully:
- Uses the shared
copyPropertiesutility - Maintains consistent implementation across all reset functions
- Adds explicit return type annotations
Let's verify that all reset functions follow the same pattern and no old manual property assignments remain:
Also applies to: 123-124, 171-172
✅ Verification successful
Reset functions implementation verified to be consistent
All reset functions in the codebase follow the same pattern:
- Use
copyPropertiesutility - Have explicit
: voidreturn type - Follow identical implementation structure
Found in:
packages/core/src/view/handler/config.ts:resetEdgeHandlerConfigresetHandleConfigresetVertexHandlerConfig
packages/core/src/util/config.ts:resetStyleDefaultsConfig
No manual property assignments were found in any reset function.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for any remaining direct property assignments in reset functions
rg -U "export const reset\w+Config.*?\{.*?=.*?\}" -A 5
Length of output: 56
Script:
#!/bin/bash
# Let's try a different approach to find all reset functions and their implementations
ast-grep --pattern 'export const reset$_Config = (): void => {
$$$
}'
Length of output: 73
Script:
#!/bin/bash
# Let's try a simpler search to find all reset functions first
rg "export const reset.*Config" -A 2
Length of output: 984
65-65: Consider deep cloning for nested configuration objects.
The spread operator (...) creates shallow copies of the config objects. If any of these configurations contain nested objects in the future, they would still share references, potentially leading to unintended modifications.
Let's verify if there are any nested objects in the configurations:
Consider using a deep clone utility if nested objects are present or anticipated:
-const defaultEdgeHandlerConfig = { ...EdgeHandlerConfig };
+const defaultEdgeHandlerConfig = structuredClone(EdgeHandlerConfig);Also applies to: 115-115, 163-163
tbouffard
changed the title
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
1 participant
Streamline the logic for resetting configuration functions: do not manually reset properties of the configuration value objects.
Improve code readability and maintainability: prevent from missing properties
Remove redundant code: use a shared function for the properties copy.
Summary by CodeRabbit
New Features
Bug Fixes
Documentation