fix: skipped unnecessary code for js object updates #37125
Conversation
WalkthroughThe changes made in this pull request focus on the Changes
Assessment against linked issues
Possibly related PRs
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
|
github-actions
bot
added
Integrations Pod General
sneha122
added
ok-to-test
github-actions
bot
added
Bug
github-actions
bot
added
Query & JS Pod
sneha122
requested review from
NilanshBansal
and removed request for
nidhi-nair and
sondermanish
![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 (1)
app/server/appsmith-server/src/main/java/com/appsmith/server/services/ce/LayoutActionServiceCEImpl.java (1)
280-283: Consider using structured logging.
The debug log could be improved by using structured logging with key-value pairs.
-log.debug(
- "Update action based on context type completed, returning actionDTO with action id: {}",
- actionDTO != null ? actionDTO.getId() : null);
+log.debug("Update action based on context type completed, actionId={}",
+ actionDTO != null ? actionDTO.getId() : "null");📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
📒 Files selected for processing (2)
- app/server/appsmith-server/src/main/java/com/appsmith/server/services/ce/LayoutActionServiceCEImpl.java (1 hunks)
- app/server/appsmith-server/src/test/java/com/appsmith/server/services/ActionCollectionServiceTest.java (3 hunks)
🔇 Additional comments (3)
app/server/appsmith-server/src/main/java/com/appsmith/server/services/ce/LayoutActionServiceCEImpl.java (1)
253-257: LGTM! Clear explanation in the comment.
The comment effectively explains why page layout updates are skipped for JS actions, which aligns with the PR objectives of removing redundant operations.
app/server/appsmith-server/src/test/java/com/appsmith/server/services/ActionCollectionServiceTest.java (2)
97-97: LGTM! Good use of @SpyBean for method verification.
The change from @Autowired to @SpyBean enables proper verification of method calls while maintaining real method behavior.
Line range hint 711-776: LGTM! Clear test method name and well-documented verifications.
The test method now:
- Has a more descriptive name that clearly states its purpose
- Properly verifies the number of method calls
- Includes clear comments explaining the expected call counts
...appsmith-server/src/main/java/com/appsmith/server/services/ce/LayoutActionServiceCEImpl.java
Show resolved
Hide resolved
...appsmith-server/src/main/java/com/appsmith/server/services/ce/LayoutActionServiceCEImpl.java
Show resolved
Hide resolved
.../appsmith-server/src/test/java/com/appsmith/server/services/ActionCollectionServiceTest.java
Outdated
Show resolved
Hide resolved
sneha122
added
ok-to-test
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Outside diff range and nitpick comments (3)
app/server/appsmith-server/src/test/java/com/appsmith/server/services/ActionCollectionServiceTest.java (3)
712-713: Consider simplifying the test method name for readability.The method name
testUpdateUnpublishedActionCollection_withValidCollection_callsPageLayoutOnlyOnceAndAssertCyclicDependencyErroris quite long. Shortening it might improve readability while still conveying the purpose of the test.
749-758: Add checks before accessing list elements to prevent exceptions.When calling
dslList.get(0), ensure the list is not empty to avoidIndexOutOfBoundsException. Adding a check will make the test more robust.
773-777: Simplify action retrieval in the stream operation.Instead of streaming and filtering to find
testAction1, consider storing a direct reference when creating the action. This will make the code clearer and more efficient.
📜 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/ActionCollectionServiceTest.java(5 hunks)
🔇 Additional comments (1)
app/server/appsmith-server/src/test/java/com/appsmith/server/services/ActionCollectionServiceTest.java (1)
791-795: Assertions correctly validate the cyclic dependency error.
The test accurately checks for the presence of the cyclic dependency error, ensuring the error handling logic is functioning as intended.
...appsmith-server/src/main/java/com/appsmith/server/services/ce/LayoutActionServiceCEImpl.java
Show resolved
Hide resolved
.../appsmith-server/src/test/java/com/appsmith/server/services/ActionCollectionServiceTest.java
Show resolved
Hide resolved
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/ActionCollectionServiceTest.java (1)
793-803: Consider adding specific error message assertion.While the code verifies the error code, it would be valuable to also assert the specific error message to ensure complete error reporting.
assertEquals( AppsmithError.CYCLICAL_DEPENDENCY_ERROR.getAppErrorCode(), actionCollectionDTO1.getErrorReports().get(0).getCode()); +assertEquals( + "Cyclic dependency found in Table1. Please resolve it.", + actionCollectionDTO1.getErrorReports().get(0).getMessage());
📜 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/ActionCollectionServiceTest.java(6 hunks)
🔇 Additional comments (3)
app/server/appsmith-server/src/test/java/com/appsmith/server/services/ActionCollectionServiceTest.java (3)
713-714: LGTM! Clear and descriptive test method name.
The renamed method accurately describes its purpose of testing page layout calls and cyclic dependency error handling.
750-765: LGTM! Well-structured test data setup.
The table DSL setup with dynamic binding to testCollection1.testAction1.data creates the foundation for testing cyclic dependencies.
773-778: LGTM! Clear cyclic dependency creation.
The code effectively creates a cyclic dependency by making testAction1 return Table1.tableData, while the table depends on the action's data.
## Description This PR is in continuation to [PR](appsmithorg#37062) which we had merged earlier, In previous, we skipped the redundant calls to update page layout when updating each js object action, as we already have a call for [updating the page layout for actionCollection](https://github.com/appsmithorg/appsmith/blob/27bdeb92b6a0b27e6afcbfe80a5cb0705c0812ac/app/server/appsmith-server/src/main/java/com/appsmith/server/services/ce/LayoutCollectionServiceCEImpl.java#L411) Since we have skipped the update page layout for each js action, we no longer need the code part after this, which basically fetches page data from DB and updates the `errorReports` in actionDTO based on layout `layoutOnLoadActionErrors`. This PR skips this unnecessary part too for each js action as we do [set the errorReport for actionCollection in the end](https://github.com/appsmithorg/appsmith/blob/27bdeb92b6a0b27e6afcbfe80a5cb0705c0812ac/app/server/appsmith-server/src/main/java/com/appsmith/server/services/ce/LayoutCollectionServiceCEImpl.java#L430) ### Will this have any impact on error messages shown to user? In order to understand this, checked out the frontend code to see if errorReports from individual js action is getting consumed on updating js object, looks like [it is not](https://github.com/appsmithorg/appsmith/blob/e7e3d5e00290919c1df0767fdefed67458ec3cc9/app/client/src/sagas/JSPaneSagas.ts#L316), so we can safely remove this piece of code. However this points to existing bug in the code (as errorReports is not even getting consumed from actionCollection), which is, when there is cyclic dependency created for js object with a widget, we don't get any toast message. Since this is existing issue which is not caused by any of the above PR implementations, creating separate issue for it and tracking it [here](appsmithorg#37129) Fixes appsmithorg#37114 _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, @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/11698258739> > Commit: 9fbde99 > <a href="https://internal.appsmith.com/app/cypress-dashboard/rundetails-65890b3c81d7400d08fa9ee5?branch=master&workflowId=11698258739&attempt=1" target="_blank">Cypress dashboard</a>. > Tags: `@tag.JS, @tag.JS` > Spec: > <hr>Wed, 06 Nov 2024 06:50:05 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** - Streamlined layout update process for actions, enhancing performance and clarity. - **Bug Fixes** - Improved test reliability by monitoring interactions with the layout service and handling cyclic dependencies. - **Documentation** - Updated comments to clarify the logic behind layout updates for JS actions. - **Tests** - Enhanced test descriptions and assertions for better clarity and validation of method interactions, including cyclic dependency scenarios. <!-- end of auto-generated comment: release notes by coderabbit.ai --> --------- Co-authored-by: “sneha122” <“sneha@appsmith.com”>
Description
This PR is in continuation to PR which we had merged earlier, In previous, we skipped the redundant calls to update page layout when updating each js object action, as we already have a call for updating the page layout for actionCollection
Since we have skipped the update page layout for each js action, we no longer need the code part after this, which basically fetches page data from DB and updates the
errorReportsin actionDTO based on layoutlayoutOnLoadActionErrors. This PR skips this unnecessary part too for each js action as we do set the errorReport for actionCollection in the endWill this have any impact on error messages shown to user?
In order to understand this, checked out the frontend code to see if errorReports from individual js action is getting consumed on updating js object, looks like it is not, so we can safely remove this piece of code.
However this points to existing bug in the code (as errorReports is not even getting consumed from actionCollection), which is, when there is cyclic dependency created for js object with a widget, we don't get any toast message. Since this is existing issue which is not caused by any of the above PR implementations, creating separate issue for it and tracking it here
Fixes #37114
or
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.JS, @tag.JS"
🔍 Cypress test results
Tip
🟢 🟢 🟢 All cypress tests have passed! 🎉 🎉 🎉
Workflow run: https://github.com/appsmithorg/appsmith/actions/runs/11698258739
Commit: 9fbde99
Cypress dashboard.
Tags:
@tag.JS, @tag.JSSpec:
Wed, 06 Nov 2024 06:50:05 UTC
Communication
Should the DevRel and Marketing teams inform users about this change?
Summary by CodeRabbit
New Features
Bug Fixes
Documentation
Tests