chore: improved performance of getUnevaluatedDataTree #37189
Conversation
WalkthroughThe Changes
Possibly related PRs
Suggested labels
Suggested reviewers
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
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Outside diff range and nitpick comments (1)
app/client/src/reducers/evaluationReducers/loadingEntitiesReducer.ts (1)
17-17: Consider enhancing the comment with specific complexity detailsThe comment could be more specific about the actual time complexity of Set comparison (O(n) where n is the number of elements).
-// its just a set with string properties time complexity of equal is not too bad +// Set comparison has O(n) time complexity where n is the number of elements
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
📒 Files selected for processing (1)
app/client/src/reducers/evaluationReducers/loadingEntitiesReducer.ts(2 hunks)
🔇 Additional comments (2)
app/client/src/reducers/evaluationReducers/loadingEntitiesReducer.ts (2)
4-4: LGTM: Appropriate use of lodash's isEqual
The import is well-placed and isEqual is the right choice for comparing Sets.
15-22: LGTM: Effective performance optimization
Good implementation of state comparison to prevent unnecessary updates in Redux.
vsvamsi1
added
ok-to-test
|
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. |
vsvamsi1
added
ok-to-test
|
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. |
|
/ci-test-limit |
|
Tests running at: https://github.com/appsmithorg/appsmith/actions/runs/11649274283. |
|
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. |
|
/ci-test-limit |
|
Tests running at: https://github.com/appsmithorg/appsmith/actions/runs/11653571485. |
|
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. |
|
/ci-test-limit |
|
Tests running at: https://github.com/appsmithorg/appsmith/actions/runs/11653831153. |
|
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. |
|
/ci-test-limit |
|
Tests running at: https://github.com/appsmithorg/appsmith/actions/runs/11654189893. |
|
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. |
|
Workflow run: https://github.com/appsmithorg/appsmith/actions/runs/11654189893. |
vsvamsi1
added
ok-to-test
vsvamsi1
changed the title
vsvamsi1
added
ok-to-test
github-actions
bot
added
the
skip-changelog
vsvamsi1
added
ok-to-test
|
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. |
There was a problem hiding this comment.
Actionable comments posted: 4
🧹 Outside diff range and nitpick comments (4)
app/client/cypress/e2e/Regression/ClientSide/Widgets/Datepicker/DatePicker2_spec.js (2)
Line range hint
52-133: Track re-enabling of commented test cases.Several critical validation tests are commented out. While there's a note about ADS changes causing failures, we should track when these can be re-enabled.
Would you like me to create a GitHub issue to track the re-enabling of these test cases:
- Min/max date validation
- Default date validation
- Required field validation
Line range hint
1-190: Align test implementation with Cypress best practices.Several patterns in the code (including commented sections) don't follow the provided coding guidelines:
- Using
cy.wait(1000)which should be avoided- Using CSS selectors like
.bp3-inputinstead of data-* attributes- Using sleep commands
Consider these improvements:
- Replace waits with proper Cypress commands:
// Instead of cy.wait cy.get(selector).should('be.visible');
- Use data attributes for selectors:
-cy.get(formWidgetsPage.datepickerWidget + " .bp3-input") +cy.get('[data-cy=datepicker-input]')app/client/src/entities/DataTree/dataTreeFactory.ts (2)
59-59: Consider removing unused parametermoduleInstanceEntitiesThe
moduleInstanceEntitiesparameter is consistently passed asnull. If it's not used within thewidgetsmethod, consider removing it from the method signature to simplify the code.
114-131: Check for emptyjsActionsbefore processingConsider adding a check to see if
jsActionsis empty before iterating over it. This can prevent unnecessary function calls and improve performance slightly.
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
📒 Files selected for processing (8)
app/client/cypress/e2e/Regression/ClientSide/Widgets/Datepicker/DatePicker2_spec.js(1 hunks)app/client/cypress/limited-tests.txt(1 hunks)app/client/cypress/support/commands.js(3 hunks)app/client/src/ce/entities/DataTree/types.ts(0 hunks)app/client/src/entities/DataTree/dataTreeFactory.ts(2 hunks)app/client/src/entities/DataTree/dataTreeWidget.test.ts(0 hunks)app/client/src/entities/DataTree/dataTreeWidget.ts(3 hunks)app/client/src/selectors/dataTreeSelectors.ts(3 hunks)
💤 Files with no reviewable changes (2)
- app/client/src/ce/entities/DataTree/types.ts
- app/client/src/entities/DataTree/dataTreeWidget.test.ts
🧰 Additional context used
📓 Path-based instructions (3)
app/client/cypress/e2e/Regression/ClientSide/Widgets/Datepicker/DatePicker2_spec.js (1)
Pattern app/client/cypress/**/**.*: Review the following e2e test code written using the Cypress test library. Ensure that:
- Follow best practices for Cypress code and e2e automation.
- Avoid using cy.wait in code.
- Avoid using cy.pause in code.
- Avoid using agHelper.sleep().
- Use locator variables for locators and do not use plain strings.
- Use data-* attributes for selectors.
- Avoid Xpaths, Attributes and CSS path.
- Avoid selectors like .btn.submit or button[type=submit].
- Perform logins via API with LoginFromAPI.
- Perform logout via API with LogOutviaAPI.
- Perform signup via API with SignupFromAPI.
- Avoid using it.only.
- Avoid using after and aftereach in test cases.
- Use multiple assertions for expect statements.
- Avoid using strings for assertions.
- Do not use duplicate filenames even with different paths.
- Avoid using agHelper.Sleep, this.Sleep in any file in code.
app/client/cypress/limited-tests.txt (1)
Pattern app/client/cypress/**/**.*: Review the following e2e test code written using the Cypress test library. Ensure that:
- Follow best practices for Cypress code and e2e automation.
- Avoid using cy.wait in code.
- Avoid using cy.pause in code.
- Avoid using agHelper.sleep().
- Use locator variables for locators and do not use plain strings.
- Use data-* attributes for selectors.
- Avoid Xpaths, Attributes and CSS path.
- Avoid selectors like .btn.submit or button[type=submit].
- Perform logins via API with LoginFromAPI.
- Perform logout via API with LogOutviaAPI.
- Perform signup via API with SignupFromAPI.
- Avoid using it.only.
- Avoid using after and aftereach in test cases.
- Use multiple assertions for expect statements.
- Avoid using strings for assertions.
- Do not use duplicate filenames even with different paths.
- Avoid using agHelper.Sleep, this.Sleep in any file in code.
app/client/cypress/support/commands.js (1)
Pattern app/client/cypress/**/**.*: Review the following e2e test code written using the Cypress test library. Ensure that:
- Follow best practices for Cypress code and e2e automation.
- Avoid using cy.wait in code.
- Avoid using cy.pause in code.
- Avoid using agHelper.sleep().
- Use locator variables for locators and do not use plain strings.
- Use data-* attributes for selectors.
- Avoid Xpaths, Attributes and CSS path.
- Avoid selectors like .btn.submit or button[type=submit].
- Perform logins via API with LoginFromAPI.
- Perform logout via API with LogOutviaAPI.
- Perform signup via API with SignupFromAPI.
- Avoid using it.only.
- Avoid using after and aftereach in test cases.
- Use multiple assertions for expect statements.
- Avoid using strings for assertions.
- Do not use duplicate filenames even with different paths.
- Avoid using agHelper.Sleep, this.Sleep in any file in code.
🔇 Additional comments (12)
app/client/src/entities/DataTree/dataTreeWidget.ts (2)
2-2: LGTM! Import changes align with the optimization goal.
The addition of omit and WIDGET_PROPS_TO_SKIP_FROM_EVAL supports the performance improvement objective by enabling property filtering.
Also applies to: 24-24
180-190: Effective performance optimization through property filtering.
The changes improve performance by:
- Clear parameter naming with
widgetWithEval - Filtering out unnecessary properties using
WIDGET_PROPS_TO_SKIP_FROM_EVAL
This optimization aligns with the PR's goal of reducing processing time.
Let's verify the impact of property filtering:
✅ Verification successful
Property filtering optimization is correctly implemented
The optimization filters out non-essential widget properties (children, parentId, renderMode, detachFromLayout, noContainerOffset) that don't require evaluation, reducing processing overhead. The constant is properly defined in WidgetConstants.tsx and is consistently used across the codebase.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Check what properties are being filtered out
rg -A 1 "WIDGET_PROPS_TO_SKIP_FROM_EVAL.*=.*{"
Length of output: 184
Script:
#!/bin/bash
# Search for the constant definition with proper regex escaping
rg "WIDGET_PROPS_TO_SKIP_FROM_EVAL\s*=\s*\{" -A 5
# Also search for any imports or references to this constant
rg "WIDGET_PROPS_TO_SKIP_FROM_EVAL"
Length of output: 1032
app/client/cypress/support/commands.js (1)
5-5: LGTM: dayjs plugin integration
The advancedFormat plugin is correctly imported and extended to enable advanced date formatting capabilities.
Also applies to: 22-22
app/client/src/entities/DataTree/dataTreeFactory.ts (1)
38-41: Verify access to widgetsMeta with metaWidgetId
In the metaWidgets method, you're accessing widgetsMeta using widget.metaWidgetId || widget.widgetId. Ensure that widget.metaWidgetId correctly references the intended metadata and that there are no cases where this could lead to undefined or incorrect meta data retrieval.
app/client/src/selectors/dataTreeSelectors.ts (8)
14-23: Imports are correctly structured
The added import statements appropriately bring in the necessary types and entities for the updated selectors.
64-71: Efficient combination of action entities
The getCurrentActionsEntities selector effectively merges actions, module actions, and workflow actions into a single array.
72-77: Efficient combination of JS action entities
The getCurrentJSActionsEntities selector correctly merges JS actions, module JS actions, and workflow JS actions into one array.
94-100: Proper creation of action entities for the data tree
The getActionsFromUnevaluatedDataTree selector is well-defined and utilizes DataTreeFactory.actions as intended.
102-105: Proper creation of JS action entities for the data tree
The getJSActionsFromUnevaluatedDataTree selector correctly leverages DataTreeFactory.jsActions to process JS actions.
107-124: Comprehensive construction of widget entities
The getWidgetsFromUnevaluatedDataTree selector gathers all relevant widget data, ensuring that module data, widgets, metadata, and layout information are accurately combined.
125-131: Accurate integration of meta widgets
The getMetaWidgetsFromUnevaluatedDataTree selector correctly processes meta widgets, maintaining consistency in the data tree.
133-163: Ensure no key collisions when merging meta widgets
Merging metaWidgets.dataTree and metaWidgets.configTree into dataTree and configTree could potentially lead to key collisions, overwriting existing entries. Please verify that this merge will not introduce unintended side effects.
app/client/cypress/limited-tests.txt
Outdated
| @@ -1,6 +1,6 @@ | |||
| # To run only limited tests - give the spec names in below format: | |||
| cypress/e2e/Regression/ClientSide/Templates/Fork_Template_spec.js | |||
|
|
|||
| cypress/e2e/Regression/ClientSide/Widgets/Datepicker/DatePicker2_spec.js | |||
There was a problem hiding this comment.
💡 Codebase verification
Review needed: Test implementation has potential issues
The DatePicker2 test file requires attention:
- Contains commented
cy.wait()calls that should be removed - Has an
afterEachblock that needs review - Several test cases are commented out without explanation
Please:
- Remove all
cy.wait()references (even commented ones) - Review and fix or remove commented test cases
- Verify if the
afterEachblock is necessary
🔗 Analysis chain
LGTM! Verify the new test implementation.
The addition of the DatePicker2 test specification follows the correct format.
Let's verify the implementation details of the new test file:
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Verify the DatePicker2 test implementation follows best practices
# Test: Check for discouraged practices
echo "Checking for discouraged practices in DatePicker2_spec.js..."
rg -i "cy\.wait|cy\.pause|agHelper\.sleep|it\.only|after\(|afterEach\(" "cypress/e2e/Regression/ClientSide/Widgets/Datepicker/DatePicker2_spec.js"
# Test: Check selector usage
echo "Checking selector patterns..."
rg -i "\.btn\.submit|button\[type=submit\]|\[class=|xpath=|css=" "cypress/e2e/Regression/ClientSide/Widgets/Datepicker/DatePicker2_spec.js"
# Test: Verify login/logout implementation
echo "Checking login/logout implementation..."
rg -i "cy\.login|cy\.logout" "cypress/e2e/Regression/ClientSide/Widgets/Datepicker/DatePicker2_spec.js"
Length of output: 1018
Script:
#!/bin/bash
# Let's first find the correct path of the DatePicker2 test file
fd -e js -g "*DatePicker2*spec.js"
# Once we find it, we'll verify its content
Length of output: 120
Script:
#!/bin/bash
# Now that we have the correct path, let's check for best practices
echo "Checking for discouraged practices..."
rg -i "cy\.wait|cy\.pause|agHelper\.sleep|it\.only|after\(|afterEach\(" "app/client/cypress/e2e/Regression/ClientSide/Widgets/Datepicker/DatePicker2_spec.js"
echo "Checking selector patterns..."
rg -i "\.btn\.submit|button\[type=submit\]|\[class=|xpath=|css=" "app/client/cypress/e2e/Regression/ClientSide/Widgets/Datepicker/DatePicker2_spec.js"
echo "Checking login/logout implementation..."
rg -i "cy\.login|cy\.logout" "app/client/cypress/e2e/Regression/ClientSide/Widgets/Datepicker/DatePicker2_spec.js"
# Let's also check the basic test structure
echo "Checking test structure..."
rg "describe\(|it\(" "app/client/cypress/e2e/Regression/ClientSide/Widgets/Datepicker/DatePicker2_spec.js"
Length of output: 1656
|
|
||
| cy.setDate(1,"ddd MMM DD YYYY"); |
There was a problem hiding this comment.
Standardize date format usage across assertions.
The date formats are inconsistent between setDate and the assertion. This could lead to flaky tests.
Apply this change to maintain consistency:
-cy.setDate(1,"ddd MMM DD YYYY");
-const nextDay = dayjs().add(1, "days").format("DD/MM/YYYY");
+const DATE_FORMAT = "DD/MM/YYYY";
+cy.setDate(1, DATE_FORMAT);
+const nextDay = dayjs().add(1, "days").format(DATE_FORMAT);Committable suggestion skipped: line range outside the PR's diff.
| @@ -525,7 +527,7 @@ Cypress.Commands.add("getDate", (date, dateFormate) => { | |||
| }); | |||
|
|
|||
| Cypress.Commands.add("setDate", (date) => { | |||
| const expDate = dayjs().add(date, "days").format("dddd, MMMM DD"); | |||
| const expDate = dayjs().add(date, "days").format("dddd, MMMM Do, YYYY"); | |||
There was a problem hiding this comment.
💡 Codebase verification
Test coverage needed for new date format dddd, MMMM Do, YYYY
The existing tests use different date formats (ddd MMM DD YYYY and DD/MM/YYYY), but none verify the new format with ordinal suffix. Add a test case to verify the new format:
- Add test in
app/client/cypress/e2e/Regression/ClientSide/Binding/DatePicker_Text_spec.js - Verify the formatted date includes day name, month name, ordinal day, and year (e.g., "Monday, January 1st, 2024")
🔗 Analysis chain
Consider adding a test for the new date format
The date format has been updated to include ordinal suffix and year (dddd, MMMM Do, YYYY). While the change is correct, we should ensure test coverage for this format change.
Let's verify the test coverage:
Consider adding a comment explaining the date format pattern for better maintainability:
- const expDate = dayjs().add(date, "days").format("dddd, MMMM Do, YYYY");
+ // Format: "Monday, January 1st, 2024"
+ const expDate = dayjs().add(date, "days").format("dddd, MMMM Do, YYYY");🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for tests using setDate command
rg -l "cy.setDate" "app/client/cypress/e2e"
Length of output: 297
Script:
#!/bin/bash
# Let's check the actual test content to verify date format coverage
rg -A 10 "cy.setDate" "app/client/cypress/e2e/Regression/ClientSide/Binding/DatePicker_Text_spec.js"
rg -A 10 "cy.setDate" "app/client/cypress/e2e/Regression/ClientSide/Widgets/Datepicker/DatePicker2_spec.js"
rg -A 10 "cy.setDate" "app/client/cypress/e2e/Regression/ClientSide/Widgets/Datepicker/DatePicker_With_Switch_spec.js"
# Also search for any existing date format tests
rg "MMMM Do, YYYY" "app/client/cypress/e2e"
Length of output: 2763
| const editorConfig = editorConfigs[action.config.pluginId]; | ||
| const dependencyConfig = pluginDependencyConfig[action.config.pluginId]; |
There was a problem hiding this comment.
Handle missing plugin configurations gracefully
When accessing editorConfigs[action.config.pluginId] and pluginDependencyConfig[action.config.pluginId], ensure that these configurations exist for all plugins. If a configuration is missing, this could lead to undefined values and potential runtime errors. Consider adding fallback logic or error handling.
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Outside diff range and nitpick comments (3)
app/client/src/sagas/EvaluationsSaga.ts (3)
353-360: Update span name to match 'evaluateAndExecuteDynamicTrigger' functionThe span initialized with
startRootSpan("DataTreeFactory.create")may not accurately reflect the function name. Consider renaming it to"evaluateAndExecuteDynamicTrigger"for better traceability in telemetry.
504-509: Rename span to 'validateProperty' for clarityCurrently, the span is named
"DataTreeFactory.create". Updating it to"validateProperty"will make telemetry data more accurate and understandable.
672-679: Adjust span name to 'evalAndLintingHandler'The span in
evalAndLintingHandleris labeled as"DataTreeFactory.create". For consistency and clarity in tracing, consider changing it to"evalAndLintingHandler".
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
📒 Files selected for processing (4)
app/client/cypress/e2e/Regression/ClientSide/Widgets/Datepicker/DatePicker2_spec.js(1 hunks)app/client/cypress/limited-tests.txt(0 hunks)app/client/src/sagas/EvalWorkerActionSagas.ts(2 hunks)app/client/src/sagas/EvaluationsSaga.ts(3 hunks)
💤 Files with no reviewable changes (1)
- app/client/cypress/limited-tests.txt
🚧 Files skipped from review as they are similar to previous changes (1)
- app/client/cypress/e2e/Regression/ClientSide/Widgets/Datepicker/DatePicker2_spec.js
🔇 Additional comments (2)
app/client/src/sagas/EvalWorkerActionSagas.ts (2)
26-26: LGTM: Telemetry import added correctly
The addition of telemetry imports aligns with the PR's performance monitoring objectives.
170-176: Verify the source of performance improvements
While these changes add telemetry to measure performance, they don't directly contribute to the claimed 80% performance improvement (800ms to 160ms). The actual optimizations should be visible in the implementation of getUnevaluatedDataTree.
|
/build-deploy-preview skip-tests=true |
|
Deploying Your Preview: https://github.com/appsmithorg/appsmith/actions/runs/11658432878. |
|
Deploy-Preview-URL: https://ce-37189.dp.appsmith.com |
) ## Description Refactored getUnevaluatedDataTree selector to be more resilient to state changes thereby reselect cache gets triggered less often. Identified an action which was firing the selectors unnecessarily, fixed that as well. For our customer during page load it used to take about 800ms cumulatively, it has now dropped to about 160ms by about 80%. This is also an impactful selector which cumulatively takes about 50,000 seconds for all our client systems per week, hence we focussed our optimisation here ## Automation /ok-to-test tags="@tag.All" ### 🔍 Cypress test results <!-- This is an auto-generated comment: Cypress test results --> > [!TIP] > 🟢 🟢 🟢 All cypress tests have passed! 🎉 🎉 🎉 > Workflow run: <https://github.com/appsmithorg/appsmith/actions/runs/11658078700> > Commit: 557172d > <a href="https://internal.appsmith.com/app/cypress-dashboard/rundetails-65890b3c81d7400d08fa9ee5?branch=master&workflowId=11658078700&attempt=2" target="_blank">Cypress dashboard</a>. > Tags: `@tag.All` > Spec: > <hr>Mon, 04 Nov 2024 06:00:06 UTC <!-- end of auto-generated comment: Cypress test results --> ## Communication Should the DevRel and Marketing teams inform users about this change? - [ ] Yes - [ ] No <!-- This is an auto-generated comment: release notes by coderabbit.ai --> ## Summary by CodeRabbit ## Summary by CodeRabbit - **Bug Fixes** - Improved state management by preventing unnecessary updates to loading entities, enhancing app performance. - **Refactor** - Updated the `loadingEntitiesReducer` to include an equality check for loading entities before state updates. - Enhanced date handling capabilities in the DatePicker widget tests and commands. - Restructured the `DataTreeFactory` class for improved modularity and clarity in data tree creation. - Simplified selector functions for better readability and maintainability in data tree selectors. <!-- end of auto-generated comment: release notes by coderabbit.ai -->
Description
Refactored getUnevaluatedDataTree selector to be more resilient to state changes thereby reselect cache gets triggered less often. Identified an action which was firing the selectors unnecessarily, fixed that as well. For our customer during page load it used to take about 800ms cumulatively, it has now dropped to about 160ms by about 80%. This is also an impactful selector which cumulatively takes about 50,000 seconds for all our client systems per week, hence we focussed our optimisation here
Automation
/ok-to-test tags="@tag.All"
🔍 Cypress test results
Tip
🟢 🟢 🟢 All cypress tests have passed! 🎉 🎉 🎉
Workflow run: https://github.com/appsmithorg/appsmith/actions/runs/11658078700
Commit: 557172d
Cypress dashboard.
Tags:
@tag.AllSpec:
Mon, 04 Nov 2024 06:00:06 UTC
Communication
Should the DevRel and Marketing teams inform users about this change?
Summary by CodeRabbit
Summary by CodeRabbit
Bug Fixes
Refactor
loadingEntitiesReducerto include an equality check for loading entities before state updates.DataTreeFactoryclass for improved modularity and clarity in data tree creation.