chore: generateDataTreeWidget Fix #37172
Conversation
WalkthroughThe changes in this pull request involve a significant refactoring of the Changes
Possibly related PRs
Suggested labels
Suggested reviewers
📜 Recent review detailsConfiguration used: .coderabbit.yaml 📒 Files selected for processing (1)
💤 Files with no reviewable 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
Documentation and Community
|
github-actions
bot
added
the
skip-changelog
|
This PR has increased the number of cyclic dependencies by 4, when compared with the release branch. Refer this document to identify the cyclic dependencies introduced by this PR. |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Outside diff range and nitpick comments (3)
app/client/src/entities/DataTree/dataTreeWidget.ts (2)
Line range hint
134-137: Consider enhancing error handling in getSetterConfigCurrently, errors are only logged and swallowed. Consider propagating critical errors to allow proper error handling by the caller, especially for configuration-related issues that could affect widget functionality.
} catch (e) { - error("Error while generating setter config", e); + error("Error while generating setter config:", e); + throw new Error(`Failed to generate setter config: ${e.message}`); }
Memoization cache size needs to be increased
The current memoization size of 1000 is insufficient as there are 2138 widget instances in the codebase. This could lead to cache invalidation and performance degradation.
- Consider increasing the memoization cache size to at least 2500 to accommodate the current widget count with room for growth
- Update the TODO comment with the actual widget count analysis
🔗 Analysis chain
Line range hint
337-343: Verify memoization cache size based on widget countThe fixed memoization size of 1000 might need adjustment. As noted in the TODO comment, this should be dynamic based on the number of widgets to prevent cache invalidation issues in larger applications.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Count the number of widget instances to determine if memoization size is sufficient echo "Analyzing widget usage..." rg -g "*.{ts,tsx}" -l "widgetId:" | xargs cat | grep -c "widgetId:" || echo "0"Length of output: 131
app/client/src/entities/DataTree/dataTreeFactory.ts (1)
Line range hint
86-128: Simplify boolean checks by removing redundant!!operatorsThe double negation
!!is unnecessary in the conditionals. Simplifying them enhances code readability.Apply this diff:
- if (!!configEntity && !!unEvalEntity) { + if (configEntity && unEvalEntity) {
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
📒 Files selected for processing (5)
app/client/src/entities/DataTree/dataTreeFactory.ts(2 hunks)app/client/src/entities/DataTree/dataTreeWidget.ts(3 hunks)app/client/src/entities/Widget/utils.ts(1 hunks)app/client/src/reducers/evaluationReducers/loadingEntitiesReducer.ts(2 hunks)app/client/src/selectors/dataTreeSelectors.ts(4 hunks)
🔇 Additional comments (12)
app/client/src/reducers/evaluationReducers/loadingEntitiesReducer.ts (2)
4-4: LGTM: Good choice using lodash's isEqual.
The addition of lodash's isEqual is appropriate for deep comparison of Set objects.
14-23: Performance optimization looks good, but verify the impact.
The optimization to prevent unnecessary state updates is well-implemented. The comment about Set complexity is helpful.
Let's verify the usage of this reducer and potential impact:
✅ Verification successful
Performance optimization is correctly implemented and safe to use
The optimization is well-justified as loadingEntities is used in critical UI rendering paths:
- Used in widget tree building for determining loading states
- Accessed via selectors for widget rendering decisions
- Integrated with Redux state management properly
The isEqual check prevents unnecessary re-renders in the widget tree, which is valuable given the frequent state updates in the loading entities reducer.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for components that might be affected by this optimization
rg -l "loadingEntities" --type ts
# Look for potential performance tests
rg -l "loadingEntitiesReducer.*test" --type ts
Length of output: 618
Script:
#!/bin/bash
# Let's check the usage of loadingEntities in the reducer and its integration
ast-grep --pattern 'const loadingEntitiesReducer = createReducer<LoadingEntitiesState, ReduxAction<$_>>($_)'
# Check how loadingEntities is used in components
rg "loadingEntities" -A 5 -B 5 app/client/src/utils/widgetRenderUtils.tsx app/client/src/selectors/dataTreeSelectors.ts
# Check the state shape and integration
ast-grep --pattern 'interface LoadingEntitiesState {
$$$
}'
Length of output: 9384
app/client/src/entities/DataTree/dataTreeWidget.ts (2)
2-2: LGTM: Import changes align with new functionality
The addition of omit from lodash and WIDGET_PROPS_TO_SKIP_FROM_EVAL constant supports the new widget property filtering mechanism.
Also applies to: 24-24
180-190: LGTM: Improved parameter naming and widget property filtering
The renaming from widget to pureWidget better indicates the unmodified nature of the input. The filtering of evaluation-specific properties using WIDGET_PROPS_TO_SKIP_FROM_EVAL is properly implemented.
app/client/src/entities/Widget/utils.ts (1)
370-370: Verify the memoization cache size limit.
The addition of a maxSize limit of 1000 entries for memoization could impact performance if there are widgets with many unique property configurations. Consider monitoring cache hit rates in production to validate this limit.
✅ Verification successful
Based on the analysis of the codebase:
- The
getAllPathsFromPropertyConfigfunction is used extensively across different widget types (Table, Chart, Form, etc.) to process widget configurations - The function is used in critical paths like widget operations, data tree evaluation, and migrations
- The memoization with maxSize 1000 appears in multiple places, suggesting it's a common pattern
- The function is used to process property paths, binding paths, validation paths, and trigger paths
The memoization cache size of 1000 is appropriate.
The limit of 1000 entries is reasonable because:
- It's used consistently across similar memoized functions in the codebase
- Widget configurations are typically finite and reused
- The cache size provides a good balance between memory usage and performance
- The function is called frequently during widget operations
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Check for the number of unique widget configurations to validate cache size
# Test: Search for widget property configurations
rg -l "propertyPaneConfig" | xargs -I {} ast-grep --pattern 'export const propertyPaneConfig = {
$$$
}'
# Test: Check for widget types to estimate configuration variety
rg -o 'type: "[^"]*"' | sort | uniq -c
Length of output: 107
Script:
#!/bin/bash
# Let's try a different approach to analyze widget configurations and property paths
# Check for widget configuration patterns and their complexity
rg -A 5 "getAllPathsFromPropertyConfig"
# Look for property configuration patterns
rg -A 5 "propertyConfig"
# Search for actual usage of the memoized function
rg -B 2 -A 2 "getAllPathsFromPropertyConfig\("
# Check for any existing memoization patterns
rg "memoize.*maxSize"
Length of output: 91222
Script:
#!/bin/bash
# Let's analyze the usage patterns and complexity of getAllPathsFromPropertyConfig
# Check implementation details of the function
ast-grep --pattern 'const getAllPathsFromPropertyConfigWithoutMemo = (
$$$
) {
$$$
}'
# Look for similar memoization patterns in the codebase
rg "memoize.*\{.*maxSize" -A 1
# Check test coverage for this function
rg -A 10 "describe.*getAllPathsFromPropertyConfig"
Length of output: 59010
Script:
#!/bin/bash
# Let's analyze the usage and complexity of the function to validate the cache size
# Look for widget property configurations to understand potential unique combinations
rg -l "propertyPaneConfig" --type ts
# Check for widget types and their configurations
rg "type:\s+['\"].*['\"]" --type ts
# Look for the actual implementation to understand memory usage
ast-grep --pattern 'const getAllPathsFromPropertyConfigWithoutMemo = (
$_
) {
$$$
}'
Length of output: 329319
app/client/src/entities/DataTree/dataTreeFactory.ts (3)
28-54: LGTM
The static metaWidgets method is structured appropriately and the implementation is clear.
56-84: LGTM
The static widgets method effectively handles widget data tree generation with clear parameter usage.
130-151: LGTM
The static jsActions method correctly processes JS actions with appropriate use of reduction.
app/client/src/selectors/dataTreeSelectors.ts (4)
72-81: Ensure all references to 'getCurrentJSActionsEntities' are updated
The function getCurrentJSActionsEntities consolidates JS actions correctly. Confirm that all instances of the old function name are replaced to avoid any unresolved references.
Run the following script to locate outdated references:
#!/bin/bash
# Description: Search for any remaining usages of the old function name.
rg 'getCurrentJSActionEntities'137-158:
Update function reference in 'getWidgetsFromUnevaluatedTree'
The selector getWidgetsFromUnevaluatedTree should reference the corrected function name.
Apply this diff to update the function reference:
- getModuleComponentsFromUnEvaluatedTree,
+ getModuleComponentsFromUnevaluatedTree,Likely invalid or redundant comment.
121-134:
Correct the capitalization in 'getModuleComponentsFromUnEvaluatedTree'
The function name getModuleComponentsFromUnEvaluatedTree has inconsistent capitalization in 'Unevaluated'. Rename it to getModuleComponentsFromUnevaluatedTree for consistency.
Apply this diff to correct the function name:
-const getModuleComponentsFromUnEvaluatedTree = createSelector(
+const getModuleComponentsFromUnevaluatedTree = createSelector(Ensure all references are updated accordingly.
Run the following script to find and replace the incorrect function name:
62-71: Verify consistency in function naming for 'getCurrentActionsEntities'
The function name has been updated to getCurrentActionsEntities. Please ensure all references to the previous getCurrentActionEntities are updated to maintain consistency and prevent potential errors.
Run the following script to check for any remaining references:
| public static actions( | ||
| actions: ActionDataState, | ||
| editorConfigs: Record<string, unknown[]>, | ||
| pluginDependencyConfig: Record<string, DependencyMap>, | ||
| ) { | ||
| const actionsSpan = startRootSpan("DataTreeFactory.actions"); | ||
|
|
||
| const res = actions.reduce( | ||
| (acc, action) => { | ||
| const editorConfig = editorConfigs[action.config.pluginId]; | ||
| const dependencyConfig = pluginDependencyConfig[action.config.pluginId]; | ||
| const { configEntity, unEvalEntity } = generateDataTreeAction( | ||
| action, | ||
| editorConfig, | ||
| dependencyConfig, | ||
| ); | ||
|
|
||
| const end = performance.now(); | ||
| acc.dataTree[action.config.name] = unEvalEntity; | ||
| acc.configTree[action.config.name] = configEntity; | ||
|
|
||
| const out = { | ||
| total: end - start, | ||
| widgets: endWidgets - startWidgets, | ||
| actions: endActions - startActions, | ||
| jsActions: endJsActions - startJsActions, | ||
| metaWidgets: endMetaWidgets - startMetaWidgets, | ||
| }; | ||
| return acc; | ||
| }, | ||
| { dataTree: {} as UnEvalTree, configTree: {} as ConfigTree }, | ||
| ); | ||
|
|
||
| log.debug("### Create unevalTree timing", out); | ||
| endSpan(actionsSpan); | ||
|
|
||
| return { unEvalTree: dataTree, configTree }; | ||
| return res; |
There was a problem hiding this comment.
Ensure safe access to editorConfig and dependencyConfig
If editorConfigs or pluginDependencyConfig do not contain the pluginId, accessing them may result in undefined. Consider providing default values to prevent potential runtime errors.
Apply this diff to add default values:
const editorConfig = editorConfigs[action.config.pluginId] || [];
const dependencyConfig = pluginDependencyConfig[action.config.pluginId] || {};
const { configEntity, unEvalEntity } = generateDataTreeAction(
action,
editorConfig,
dependencyConfig,
);📝 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.
| public static actions( | |
| actions: ActionDataState, | |
| editorConfigs: Record<string, unknown[]>, | |
| pluginDependencyConfig: Record<string, DependencyMap>, | |
| ) { | |
| const actionsSpan = startRootSpan("DataTreeFactory.actions"); | |
| const res = actions.reduce( | |
| (acc, action) => { | |
| const editorConfig = editorConfigs[action.config.pluginId]; | |
| const dependencyConfig = pluginDependencyConfig[action.config.pluginId]; | |
| const { configEntity, unEvalEntity } = generateDataTreeAction( | |
| action, | |
| editorConfig, | |
| dependencyConfig, | |
| ); | |
| const end = performance.now(); | |
| acc.dataTree[action.config.name] = unEvalEntity; | |
| acc.configTree[action.config.name] = configEntity; | |
| const out = { | |
| total: end - start, | |
| widgets: endWidgets - startWidgets, | |
| actions: endActions - startActions, | |
| jsActions: endJsActions - startJsActions, | |
| metaWidgets: endMetaWidgets - startMetaWidgets, | |
| }; | |
| return acc; | |
| }, | |
| { dataTree: {} as UnEvalTree, configTree: {} as ConfigTree }, | |
| ); | |
| log.debug("### Create unevalTree timing", out); | |
| endSpan(actionsSpan); | |
| return { unEvalTree: dataTree, configTree }; | |
| return res; | |
| public static actions( | |
| actions: ActionDataState, | |
| editorConfigs: Record<string, unknown[]>, | |
| pluginDependencyConfig: Record<string, DependencyMap>, | |
| ) { | |
| const actionsSpan = startRootSpan("DataTreeFactory.actions"); | |
| const res = actions.reduce( | |
| (acc, action) => { | |
| const editorConfig = editorConfigs[action.config.pluginId] || []; | |
| const dependencyConfig = pluginDependencyConfig[action.config.pluginId] || {}; | |
| const { configEntity, unEvalEntity } = generateDataTreeAction( | |
| action, | |
| editorConfig, | |
| dependencyConfig, | |
| ); | |
| acc.dataTree[action.config.name] = unEvalEntity; | |
| acc.configTree[action.config.name] = configEntity; | |
| return acc; | |
| }, | |
| { dataTree: {} as UnEvalTree, configTree: {} as ConfigTree }, | |
| ); | |
| endSpan(actionsSpan); | |
| return res; |
| let dataTree: UnEvalTree = {}; | ||
| let configTree: ConfigTree = {}; | ||
| const rootSpan = startRootSpan("DataTreeFactory.create"); | ||
|
|
||
| configTree = { | ||
| ...actions.configTree, | ||
| ...jsActions.configTree, | ||
| ...widgets.configTree, | ||
| }; | ||
| dataTree = { | ||
| ...actions.dataTree, | ||
| ...jsActions.dataTree, | ||
| ...widgets.dataTree, | ||
| }; | ||
|
|
||
| dataTree.appsmith = { | ||
| ...appData, | ||
| // combine both persistent and transient state with the transient state | ||
| // taking precedence in case the key is the same | ||
| store: appData.store, | ||
| theme, | ||
| } as AppsmithEntity; | ||
| configTree = { ...configTree, ...metaWidgets.configTree }; | ||
| dataTree = { ...dataTree, ...metaWidgets.dataTree }; | ||
|
|
||
| endSpan(rootSpan); | ||
|
|
||
| return { | ||
| configTree, | ||
| unEvalTree: dataTree, | ||
| }; | ||
| }, |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Optimize data tree merging for better readability
The merging of configTree and dataTree uses multiple spread operators. Consider refactoring this logic into utility functions to enhance readability and maintainability.
|
This PR has increased the number of cyclic dependencies by 4, when compared with the release branch. Refer this document to identify the cyclic dependencies introduced by this PR. |
Description
Tip
Add a TL;DR when the description is longer than 500 words or extremely technical (helps the content, marketing, and DevRel team).
Please also include relevant motivation and context. List any dependencies that are required for this change. Add links to Notion, Figma or any other documents that might be relevant to the PR.
Fixes #
Issue Numberor
Fixes
Issue URLWarning
If no issue exists, please create an issue first, and check with the maintainers if the issue is valid.
Automation
/ok-to-test tags="@tag.All"
🔍 Cypress test results
Caution
🔴 🔴 🔴 Some tests have failed.
Workflow run: https://github.com/appsmithorg/appsmith/actions/runs/11612013649
Commit: a247e1f
Cypress dashboard.
Tags: @tag.All
Spec:
The following are new failures, please fix them before merging the PR:
- cypress/e2e/Regression/ClientSide/Autocomplete/Autocomplete_setters_spec.ts
- cypress/e2e/Regression/ClientSide/Autocomplete/JS_AC1_spec.ts
- cypress/e2e/Regression/ClientSide/Autocomplete/PropertyPaneSuggestion_spec.ts
- cypress/e2e/Regression/ClientSide/BugTests/InputTruncateCheck_Spec.ts
- cypress/e2e/Regression/ClientSide/OtherUIFeatures/ErrorMessages_spec.ts
- cypress/e2e/Regression/ClientSide/Widgets/CurrencyInput/CurrencyInputDynamicCurrencyCode_spec.js
- cypress/e2e/Regression/ClientSide/Widgets/Datepicker/DatePicker2_spec.js
- cypress/e2e/Regression/ClientSide/Widgets/PhoneInput/PhoneInputDynamicValue_spec.js
- cypress/e2e/Regression/ClientSide/Widgets/TableV2/table_data_change_spec.ts
- cypress/e2e/Regression/ServerSide/JsFunctionExecution/PlatformFn_spec.ts
List of identified flaky tests.Thu, 31 Oct 2024 17:56:22 UTC
Communication
Should the DevRel and Marketing teams inform users about this change?
Summary by CodeRabbit
Release Notes
New Features
metaWidgets,widgets, andactions.getActionsFromUnevaluatedTreeandgetWidgetsFromUnevaluatedTree.Improvements
getAllPathsFromPropertyConfigfunction with memoization.Bug Fixes