fix: resolve redundant JSObject action saving #36958
Conversation
|
/build-deploy-preview skip-tests=true |
|
Deploying Your Preview: https://github.com/appsmithorg/appsmith/actions/runs/11393743478. |
|
Deploy-Preview-URL: https://ce-36958.dp.appsmith.com |
github-actions
bot
added
Javascript Product
WalkthroughThe changes introduce a new interface Changes
Assessment against linked issues
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: 1
🧹 Outside diff range and nitpick comments (3)
app/server/appsmith-server/src/main/java/com/appsmith/server/services/ce/LayoutCollectionServiceCEImpl.java (3)
Line range hint
138-138: Remove redundant assignment ofunpublishedCollectionThe
actionCollection.setUnpublishedCollection(collectionDTO);call at line 138 is redundant since it's already set at line 121. Consider removing the duplicate assignment to streamline the method.
Line range hint
139-145: Handle exceptions in action updates appropriatelyIn the
moveCollectionmethod, within theactionUpdatesFlux, exceptions duringupdateUnpublishedActionare caught and logged, but the error is suppressed by returningMono.empty(). This might lead to silent failures. Consider propagating the exception or implementing a fallback mechanism to ensure that failures are not overlooked.
Line range hint
386-388: Remove unnecessary mapping operationThe
.map(updatedBranchedActionCollection -> { return updatedBranchedActionCollection; })insendErrorReportsFromPageToCollectiondoesn't modify the data. You can remove this mapping step to simplify the reactive chain.
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
📒 Files selected for processing (1)
- app/server/appsmith-server/src/main/java/com/appsmith/server/services/ce/LayoutCollectionServiceCEImpl.java (1 hunks)
🧰 Additional context used
| .distinct(actionDTO -> actionCollectionDTO.getName() + "." + actionDTO.getName()) | ||
| .flatMap(actionDTO -> { | ||
| actionDTO.setDeletedAt(null); | ||
| setContextId(branchedActionCollection, actionDTO); |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Simplify initialization of baseActionIds
You create a new HashSet called baseActionIds and add all elements from validBaseActionIds. Since validBaseActionIds is already a set, you can use it directly or instantiate baseActionIds with new HashSet<>(validBaseActionIds) to simplify the code.
|
Executing both functions always executes the 2nd function body part, according to this we show an error for the 2nd function. We should avoid overriding the first function definition. |
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Outside diff range and nitpick comments (2)
app/client/src/ce/entities/DataTree/dataTreeJSAction.ts (2)
58-60: Approved: Good addition to prevent duplicate actions.This check effectively prevents duplicate actions from being added to the data tree, aligning with the PR's objective.
Consider using Object.prototype.hasOwnProperty for a slight performance boost:
- if (actionsData[action.name]) continue; + if (Object.prototype.hasOwnProperty.call(actionsData, action.name)) continue;This change ensures we're checking the object's own properties, not inherited ones.
58-60: Overall impact: Positive change with minimal risk.The modification effectively prevents duplicate actions without affecting other parts of the function. It's a targeted fix that addresses the issue at hand.
For improved clarity, consider adding a brief comment explaining why we're skipping duplicate actions:
+ // Skip duplicate actions to prevent redundant entries in the data tree if (actionsData[action.name]) continue;This comment would help future developers understand the rationale behind this check.
|
/build-deploy-preview skip-tests=true |
|
Deploying Your Preview: https://github.com/appsmithorg/appsmith/actions/runs/11424753391. |
|
Deploy-Preview-URL: https://ce-36958.dp.appsmith.com |
|
/build-deploy-preview skip-tests=true |
|
Deploying Your Preview: https://github.com/appsmithorg/appsmith/actions/runs/11436694905. |
|
Deploy-Preview-URL: https://ce-36958.dp.appsmith.com |
rishabhrathod01
requested review from
sneha122
and removed request for
ApekshaBhosale
|
@rishabhrathod01 Should we add unit test cases for the changes? |
Yes, let me add unit test for both the changes. |
rishabhrathod01
requested review from
nidhi-nair and
sondermanish
as code owners
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Outside diff range and nitpick comments (1)
app/server/appsmith-server/src/test/java/com/appsmith/server/services/ActionCollectionServiceImplTest.java (1)
414-463: LGTM. Consider adding an assertion for action properties.The new test method effectively verifies that duplicate actions are handled correctly. Good job on covering this edge case.
Consider adding an assertion to verify that the action's properties (e.g.,
clientSideExecution) are preserved after the update. This would ensure that the de-duplication process doesn't inadvertently modify the action's attributes.
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
📒 Files selected for processing (1)
- app/server/appsmith-server/src/test/java/com/appsmith/server/services/ActionCollectionServiceImplTest.java (1 hunks)
🧰 Additional context used
Failed server tests
|
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Outside diff range and nitpick comments (4)
app/client/src/workers/Evaluation/JSObject/test.ts (2)
150-159: Enhance test case clarity and structure.While the test correctly validates duplicate handling, consider these improvements:
- Make the test description more specific about what constitutes "correct" updates
- Consider breaking down the large test data into smaller, focused test cases for different scenarios (e.g., duplicate variables vs duplicate actions)
155-155: Improve test data quality and readability.The JSObject body contains syntax errors and poorly formatted code. Consider:
- Using properly formatted and syntactically correct JavaScript
- Using template literals for better readability
- Using realistic function bodies that better represent actual use cases
Example of improved body format:
const body = ` export default { myVar1: [], myVar1: [], // Intentional duplicate myVar2: {}, myFun1: () => { // First function return true; }, myFun2: () => { // Intentional duplicate function return false; }, myFun2: () => { // Intentional duplicate function return true; } }`;Also applies to: 167-167
app/client/src/workers/Evaluation/JSObject/index.ts (2)
122-122: Simplify boolean coercionThe double negation (
!!) is redundant here asparsedObjectwill already be coerced to a boolean in this context.- if (!!parsedObject) { + if (parsedObject) {🧰 Tools
🪛 Biome
[error] 122-122: Avoid redundant double-negation.
It is not necessary to use double-negation when a value will already be coerced to a boolean.
Unsafe fix: Remove redundant double-negation(lint/complexity/noExtraBooleanCast)
Line range hint
172-174: Consider adding error loggingThe empty catch block could hide important errors. Consider at least logging the error or adding it to
dataTreeEvalRef.errors.- } catch { - // in case we need to handle error state + } catch (error) { + dataTreeEvalRef.errors.push({ + type: EvalErrorTypes.PARSE_JS_ERROR, + context: { + entity: entity, + propertyPath: `${entityName}.${parsedElement.key}`, + }, + message: error.message, + }); }
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
📒 Files selected for processing (2)
- app/client/src/workers/Evaluation/JSObject/index.ts (4 hunks)
- app/client/src/workers/Evaluation/JSObject/test.ts (2 hunks)
🧰 Additional context used
🪛 Biome
app/client/src/workers/Evaluation/JSObject/index.ts
[error] 122-122: Avoid redundant double-negation.
It is not necessary to use double-negation when a value will already be coerced to a boolean.
Unsafe fix: Remove redundant double-negation(lint/complexity/noExtraBooleanCast)
🔇 Additional comments (4)
app/client/src/workers/Evaluation/JSObject/test.ts (1)
1-11: LGTM: Import statements are well-organized.The new imports correctly support the added test functionality for handling duplicate JSObject actions.
app/client/src/workers/Evaluation/JSObject/index.ts (3)
118-119: LGTM: Effective use of maps to prevent duplicatesUsing maps with action/variable names as keys is an elegant solution to prevent duplicate entries while maintaining O(1) lookup performance.
126-127: LGTM: Effective duplicate preventionThe early return when encountering duplicate actions aligns with the PR's objective of preventing redundant action saving.
197-198: Verify array order consistencyThe conversion from map to array using
Object.values()looks good, but we should verify that the order of actions doesn't affect any downstream consumers.
rishabhrathod01
requested review from
sneha122
and removed request for
nidhi-nair and
sondermanish
|
/build-deploy-preview skip-tests=true |
|
Deploying Your Preview: https://github.com/appsmithorg/appsmith/actions/runs/11485342857. |
|
Deploy-Preview-URL: https://ce-36958.dp.appsmith.com |
|
Deploying Your Preview: https://github.com/appsmithorg/appsmith/actions/runs/11493375745. |
## Description [During analysis of action creation flow metrics](#37151 (comment)), we observed that RefactoringService.isNameAllowed is taking 80-90% of the total JS object action time. This PR optimises this part in a way that for any jsobject action, instead of fetching all actions from DB and comparing it to see if current action name is allowed, we simply do that check in memory where for current action collection, if any action names are being duplicated, we throw the error. We could make this change easily because recently we merged a [PR](#36958) which removes the actions with duplicate name from client payload whenever Js object update API is called, with this change, we can guarantee that for any JS object update call, all actions inside it will always have unique names. This PR makes the similar check on backend where if any action has duplicate name within collection, we throw an error and don't store that action in the DB. We may need to consider following test case in both before and after implementation of this approach. This can be covered during PR testing: What happens if the client sends multiple requests to add a new function in an existing collection. That is, as a result of the debounce logic, if the server receives 2 consecutive requests with a populated collection but without actionId associated to either request. Relevant thread: https://theappsmith.slack.com/archives/C040LHZN03V/p1731571364933089 Fixes #37365 _or_ Fixes `Issue URL` > [!WARNING] > _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.JS" ### 🔍 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/11911295324> > Commit: d5c75ed > <a href="https://internal.appsmith.com/app/cypress-dashboard/rundetails-65890b3c81d7400d08fa9ee5?branch=master&workflowId=11911295324&attempt=1" target="_blank">Cypress dashboard</a>. > Tags: `@tag.JS` > Spec: > <hr>Tue, 19 Nov 2024 11:14:16 UTC <!-- end of auto-generated comment: Cypress test results --> ## Communication Should the DevRel and Marketing teams inform users about this change? - [ ] Yes - [x] No <!-- This is an auto-generated comment: release notes by coderabbit.ai --> ## Summary by CodeRabbit - **New Features** - Enhanced validation to prevent the creation of actions with duplicate names in action collections. - Simplified handling of JavaScript actions, allowing them to bypass certain validation checks. - **Bug Fixes** - Improved error handling during action updates and collection modifications to ensure better logging and management of failures. - **Tests** - Added tests to verify that duplicate action names trigger appropriate error messages, enhancing the robustness of the action collection feature. <!-- end of auto-generated comment: release notes by coderabbit.ai --> --------- Co-authored-by: “sneha122” <“[email protected]”>
## Description In this change, we avoid storing duplicate actions or variables in the `parsedBody` after AST parsing. This prevents duplicate actions from being sent to the server on save. This change also removes `parsedFunction` from the action object of parsedBody as it is not being used. Fixes appsmithorg#36879 ## Test Cases - [x] After parsing JS Object make sure parsedBody doesn't have duplicate actions or variables ## Automation /ok-to-test tags="@tag.JS" ### 🔍 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/11485245933> > Commit: c12ee70 > <a href="https://internal.appsmith.com/app/cypress-dashboard/rundetails-65890b3c81d7400d08fa9ee5?branch=master&workflowId=11485245933&attempt=1" target="_blank">Cypress dashboard</a>. > Tags: `@tag.JS` > Spec: > <hr>Wed, 23 Oct 2024 18:20:11 UTC <!-- end of auto-generated comment: Cypress test results --> ## Communication Should the DevRel and Marketing teams inform users about this change? - [ ] Yes - [x] No <!-- This is an auto-generated comment: release notes by coderabbit.ai --> ## Summary by CodeRabbit - **New Features** - Introduced enhanced methods for creating, updating, and moving action collections, improving validation and consistency. - Added a new interface for better handling of JavaScript actions and variables, enhancing data processing efficiency. - **Bug Fixes** - Improved error reporting from pages to collections, ensuring accurate tracking of action errors. - Added tests to ensure duplicate actions are correctly handled during updates. - **Documentation** - Updated comments and documentation for clarity on new functionalities and changes. <!-- end of auto-generated comment: release notes by coderabbit.ai -->
…37391) ## Description [During analysis of action creation flow metrics](appsmithorg#37151 (comment)), we observed that RefactoringService.isNameAllowed is taking 80-90% of the total JS object action time. This PR optimises this part in a way that for any jsobject action, instead of fetching all actions from DB and comparing it to see if current action name is allowed, we simply do that check in memory where for current action collection, if any action names are being duplicated, we throw the error. We could make this change easily because recently we merged a [PR](appsmithorg#36958) which removes the actions with duplicate name from client payload whenever Js object update API is called, with this change, we can guarantee that for any JS object update call, all actions inside it will always have unique names. This PR makes the similar check on backend where if any action has duplicate name within collection, we throw an error and don't store that action in the DB. We may need to consider following test case in both before and after implementation of this approach. This can be covered during PR testing: What happens if the client sends multiple requests to add a new function in an existing collection. That is, as a result of the debounce logic, if the server receives 2 consecutive requests with a populated collection but without actionId associated to either request. Relevant thread: https://theappsmith.slack.com/archives/C040LHZN03V/p1731571364933089 Fixes appsmithorg#37365 _or_ Fixes `Issue URL` > [!WARNING] > _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.JS" ### 🔍 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/11911295324> > Commit: d5c75ed > <a href="https://internal.appsmith.com/app/cypress-dashboard/rundetails-65890b3c81d7400d08fa9ee5?branch=master&workflowId=11911295324&attempt=1" target="_blank">Cypress dashboard</a>. > Tags: `@tag.JS` > Spec: > <hr>Tue, 19 Nov 2024 11:14:16 UTC <!-- end of auto-generated comment: Cypress test results --> ## Communication Should the DevRel and Marketing teams inform users about this change? - [ ] Yes - [x] No <!-- This is an auto-generated comment: release notes by coderabbit.ai --> ## Summary by CodeRabbit - **New Features** - Enhanced validation to prevent the creation of actions with duplicate names in action collections. - Simplified handling of JavaScript actions, allowing them to bypass certain validation checks. - **Bug Fixes** - Improved error handling during action updates and collection modifications to ensure better logging and management of failures. - **Tests** - Added tests to verify that duplicate action names trigger appropriate error messages, enhancing the robustness of the action collection feature. <!-- end of auto-generated comment: release notes by coderabbit.ai --> --------- Co-authored-by: “sneha122” <“[email protected]”>
Description
In this change, we avoid storing duplicate actions or variables in the
parsedBodyafter AST parsing. This prevents duplicate actions from being sent to the server on save.This change also removes
parsedFunctionfrom the action object of parsedBody as it is not being used.Fixes #36879
Test Cases
Automation
/ok-to-test tags="@tag.JS"
🔍 Cypress test results
Tip
🟢 🟢 🟢 All cypress tests have passed! 🎉 🎉 🎉
Workflow run: https://github.com/appsmithorg/appsmith/actions/runs/11485245933
Commit: c12ee70
Cypress dashboard.
Tags:
@tag.JSSpec:
Wed, 23 Oct 2024 18:20:11 UTC
Communication
Should the DevRel and Marketing teams inform users about this change?
Summary by CodeRabbit
New Features
Bug Fixes
Documentation