chore: updated actions fetch logic for consolidated view api #36096
Conversation
WalkthroughThe changes involve a comprehensive update to the action retrieval system, transitioning from an application-centric approach to a page-centric model. This includes modifications to method signatures across various classes, emphasizing the use of Changes
Sequence Diagram(s)sequenceDiagram
participant User
participant PageService
participant ActionService
participant API
User->>PageService: Request Page Load
PageService->>ActionService: Get Actions for Page
ActionService->>API: Fetch Actions by Page ID
API-->>ActionService: Return Actions
ActionService-->>PageService: Provide Actions
PageService-->>User: Display Page with Actions
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 1, when compared with the release branch. Refer this document to identify the cyclic dependencies introduced by this PR. |
Failed server tests
|
sneha122
requested review from
nidhi-nair,
sondermanish and
ayushpahwa
as code owners
| if (!isBlank(basePageId)) { | ||
| // When branchName is null, we don't need to fetch page from DB to derive pageId | ||
| // We can simply reuse the pageId that is passed by client to query actions | ||
| Mono<String> branchedPageIdMono = !StringUtils.hasText(branchName) |
There was a problem hiding this comment.
In this part, I am checking if the branchName is empty (app is not connected to git), I am using basePageId from the request directly to fetch the actions of a page, but if branchName is not empty (app is connected to git), I am fetching the page data based on basePageId and branchName. @nidhi-nair Can you please check this part and let me know if this will break anything else?
sneha122
requested review from
subrata71 and
AmanAgarwal041
and removed request for
nidhi-nair,
ayushpahwa 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: 0
Outside diff range, codebase verification and nitpick comments (4)
app/server/appsmith-server/src/main/java/com/appsmith/server/newactions/base/NewActionServiceCE.java (1)
84-84: Attention Required: Update Method Calls to UsepageIdThe method
getActionsForViewModehas been updated to acceptpageIdinstead ofapplicationId. However, there are still instances in the codebase where this method is called withapplicationId. These need to be updated to ensure the method functions correctly with the new parameter.Please review and update the following instances:
ActionControllerCE.java: The method is called withbranchedApplicationId. This should be updated to usepageId.Ensure that all occurrences of this method call are updated to pass the correct parameter to maintain functionality and prevent potential errors.
Analysis chain
Excellent modification to the
getActionsForViewModemethod signature!Changing the parameter from
applicationIdtopageIdis consistent with the overall direction of the PR, which emphasizes page-level action retrieval. This change enhances the clarity and purpose of the method.However, it's crucial to ensure that all the callers of this method are updated to pass the
pageIdinstead ofapplicationId. This will maintain the integrity and correctness of the functionality.Please verify that all the callers of the
getActionsForViewModemethod are updated to pass thepageId. You can use the following command to search for the method usage:Ensure that the
pageIdis being passed correctly in all the occurrences.Scripts executed
The following scripts were executed for the analysis:
Script:
rg --type java -A 5 $'getActionsForViewMode'Length of output: 7871
app/client/src/actions/pageActions.tsx (1)
297-306: Excellent work implementing the new action creator!The
fetchPublishedPageResourcesActionfollows the Redux action pattern correctly. It dispatches an action with the appropriate type and payload. The comments provide clear context about the current purpose and potential future extensibility of this action.As a suggestion for future extensibility, consider the following:
When extending this action to fetch other page-level resources, you can modify the payload to include an optional
resourceTypeproperty. This way, the same action can be reused to fetch different types of resources based on theresourceTypevalue. For example:export interface FetchPublishedPageResourcesPayload { pageId: string; resourceType?: 'actions' | 'widgets' | 'datasources'; } export const fetchPublishedPageResourcesAction = ( pageId: string, resourceType?: 'actions' | 'widgets' | 'datasources', ): ReduxAction<FetchPublishedPageResourcesPayload> => ({ type: ReduxActionTypes.FETCH_PUBLISHED_PAGE_RESOURCES_INIT, payload: { pageId, resourceType, }, });This approach allows for flexibility and reusability of the action for fetching different resource types in the future.
app/server/appsmith-server/src/main/java/com/appsmith/server/repositories/ce/CustomNewActionRepositoryCEImpl.java (1)
216-218: Suggest renaming the method to maintain consistency.Consider renaming the method to
getCriterionForFindPublishedActionsByPageIdAndExcludedPluginTypeto maintain consistency with the updated functionality and the changes made to the public methodfindPublishedActionsByPageIdAndExcludedPluginType.Apply this diff to rename the method:
-protected BridgeQuery<NewAction> getCriterionForFindPublishedActionsByApplicationIdAndExcludedPluginType( +protected BridgeQuery<NewAction> getCriterionForFindPublishedActionsByPageIdAndExcludedPluginType(Approve the changes to the query construction logic.
The query construction logic has been updated correctly to use
pageIdinstead ofapplicationId. The changes align with the shift in focus from application IDs to page IDs for fetching published actions.app/client/src/ce/sagas/PageSagas.tsx (1)
385-416: Great work on implementing thefetchPublishedPageResourcesSaga! The overall structure and logic look good. 👍A few suggestions to consider:
- The error handling could be more specific by checking the error type and providing a more detailed error message. This will help with debugging and troubleshooting.
- Consider splitting the function into smaller, reusable functions to improve readability and maintainability. For example, you could extract the API call and response validation into separate functions.
Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Files selected for processing (11)
- app/client/src/actions/pageActions.tsx (2 hunks)
- app/client/src/ce/constants/ReduxActionConstants.tsx (2 hunks)
- app/client/src/ce/sagas/PageSagas.tsx (5 hunks)
- app/client/src/ee/sagas/PageSagas.tsx (2 hunks)
- app/client/src/pages/AppViewer/index.tsx (2 hunks)
- app/client/src/reducers/uiReducers/appViewReducer.tsx (1 hunks)
- app/server/appsmith-server/src/main/java/com/appsmith/server/newactions/base/NewActionServiceCE.java (1 hunks)
- app/server/appsmith-server/src/main/java/com/appsmith/server/newactions/base/NewActionServiceCEImpl.java (2 hunks)
- app/server/appsmith-server/src/main/java/com/appsmith/server/repositories/ce/CustomNewActionRepositoryCE.java (1 hunks)
- app/server/appsmith-server/src/main/java/com/appsmith/server/repositories/ce/CustomNewActionRepositoryCEImpl.java (1 hunks)
- app/server/appsmith-server/src/main/java/com/appsmith/server/services/ce/ConsolidatedAPIServiceCEImpl.java (3 hunks)
Additional context used
Learnings (1)
app/server/appsmith-server/src/main/java/com/appsmith/server/services/ce/ConsolidatedAPIServiceCEImpl.java (2)
Learnt from: sumitsum PR: appsmithorg/appsmith#29875 File: app/server/appsmith-server/src/main/java/com/appsmith/server/services/ConsolidatedAPIServiceImpl.java:0-0 Timestamp: 2023-12-27T21:27:19.887Z Learning: Test cases for the `getConsolidatedInfoForPageLoad` method have already been added, and the refactoring of the method into smaller methods is planned for later and is being tracked in a separate GitHub issue.Learnt from: sumitsum PR: appsmithorg/appsmith#29875 File: app/server/appsmith-server/src/main/java/com/appsmith/server/services/ConsolidatedAPIServiceImpl.java:0-0 Timestamp: 2023-12-29T03:33:22.401Z Learning: The user has acknowledged the suggestion to refactor the `getConsolidatedInfoForPageLoad` method and has indicated that this will be addressed later.
Additional comments not posted (18)
app/client/src/reducers/uiReducers/appViewReducer.tsx (1)
34-38: Great work on enhancing the error handling capabilities of the reducer! 👍The new case for the
FETCH_PUBLISHED_PAGE_RESOURCES_ERRORaction type is implemented correctly. It setsisFetchingPagetofalsewhen an error occurs during the fetching of published page resources, ensuring that the application can appropriately respond to failures in the fetching process.The code follows the existing pattern and style of the reducer, maintaining consistency and readability.
app/client/src/ee/sagas/PageSagas.tsx (1)
76-79: Great work on enhancing the published page resources management! 👍The new
takeLatesteffect and the correspondingfetchPublishedPageResourcesSagasaga function are well-integrated into the existing code structure and follow the Redux saga best practices. This change expands the application's capability to efficiently manage published page resources.As mentioned in the summary, this addition reflects an increase in the complexity and responsiveness of the state management logic. It allows the saga to handle the new
ReduxActionTypes.FETCH_PUBLISHED_PAGE_RESOURCES_INITaction type and ensures that only the most recent action is processed, preventing race conditions.The shift in focus from application-level to page-level resource management is a positive change that improves the overall structure and clarity of the codebase. Keep up the good work in enhancing the functionality and performance of the application! 🚀
app/server/appsmith-server/src/main/java/com/appsmith/server/repositories/ce/CustomNewActionRepositoryCE.java (2)
40-41: Great work updating the method to fetch actions based on page ID! 👍The change from
applicationIdtopageIdas a method parameter aligns perfectly with the PR objective of improving performance by fetching actions specific to a page. This will significantly reduce the amount of data fetched and improve the response time.
43-43: Excellent update to the method signature! 🌟Using
Optionalfor theaclPermissionandsortparameters is a great way to enhance the method's flexibility. It allows the caller to optionally provide these values, making the method more adaptable to different scenarios. This change improves the overall design and usability of the method.app/server/appsmith-server/src/main/java/com/appsmith/server/newactions/base/NewActionServiceCE.java (1)
81-82: Great work on refining the method signature to align with the page-specific action retrieval approach!The changes to the method signature are a step in the right direction. By focusing on retrieving published actions based on the
pageIdand allowing the exclusion of specific plugin types, you have laid the foundation for more targeted and efficient action fetching.This modification aligns well with the PR objective of improving performance by fetching only the actions relevant to the current page. It also provides flexibility in excluding certain plugin types, which could be beneficial in certain scenarios.
app/client/src/pages/AppViewer/index.tsx (2)
31-34: Great job introducing the new action to fetch page-specific resources! 👍The new
fetchPublishedPageResourcesActionimport aligns perfectly with the PR objective of improving performance by fetching only the relevant resources for the current page. This is a step in the right direction to optimize the application's resource management.
175-176: Excellent sequencing of the new action dispatch! 🙌Dispatching the
fetchPublishedPageResourcesActionright aftersetupPublishedPageensures that the page-specific resources are fetched as soon as the page is set up. This sequential flow optimizes the rendering process and enhances the overall performance.A few additional insights:
- The
basePageIdis correctly passed as an argument to fetch the resources specific to the current page.- The comment "Used for fetching page resources" provides clear context for the new dispatch.
Keep up the great work in improving the application's resource management! 😊
app/client/src/actions/pageActions.tsx (1)
75-77: Great job defining the new interface!The
FetchPublishedPageResourcesPayloadinterface is well-structured with a clear purpose. Using TypeScript interfaces to define the shape of the payload is a good practice for maintaining type safety and improving code readability.app/server/appsmith-server/src/main/java/com/appsmith/server/repositories/ce/CustomNewActionRepositoryCEImpl.java (1)
206-207: Approve the changes to the method signature and name.The changes align with the shift in focus from application IDs to page IDs for fetching published actions. The method now accepts
pageIdinstead ofapplicationIdand the name has been updated to reflect this change.Verify the impact of the changes on the codebase.
Ensure that all calls to this method have been updated to pass the
pageIdinstead ofapplicationId. Also, verify that the logic for fetching published actions based onpageIdis correct and does not introduce any unintended side effects.Run the following script to verify the method usage:
Also applies to: 209-210
Verification successful
Verification Successful: Method Usage Updated Correctly
The method
findPublishedActionsByPageIdAndExcludedPluginTypehas been updated to usepageIdinstead ofapplicationId, and this change has been correctly propagated throughout the codebase. The usage inNewActionServiceCEImpl.javaconfirms that the method is being called with the correct arguments. No further occurrences of the method were found, indicating that all necessary updates have been made.
- Files Verified:
CustomNewActionRepositoryCEImpl.javaCustomNewActionRepositoryCE.javaNewActionServiceCEImpl.javaScripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify all method calls to `findPublishedActionsByPageIdAndExcludedPluginType` pass the correct arguments. # Test: Search for the method usage. Expect: Only occurrences with `pageId` argument. rg --type java -A 5 $'findPublishedActionsByPageIdAndExcludedPluginType'Length of output: 3018
app/server/appsmith-server/src/main/java/com/appsmith/server/services/ce/ConsolidatedAPIServiceCEImpl.java (4)
40-40: Import statement looks good!The new import statement for
org.springframework.util.StringUtilsis necessary and correctly added.
201-201: Good use of caching!The new variable
branchedPageMonoCachedis correctly declared and used for caching the result of fetching a page. This will help improve performance by reducing redundant database calls.
287-303: Efficient action retrieval logic!The updated logic for fetching actions based on the presence of
basePageIdandbranchNameis well-implemented. It efficiently utilizes the cached page data when available and handles the case whenbranchNameis null by directly usingbasePageIdto query actions. This approach avoids unnecessary database calls and improves performance.
Line range hint
214-220: Minor code changes look good!The remaining minor code changes are safe and do not introduce any issues.
app/client/src/ce/sagas/PageSagas.tsx (1)
385-385: Please address the previous comment by sagar-qa007.The comment asks if you should consider updating an existing test case or adding a new one for unit testing. This is still a valid concern that should be addressed.
app/client/src/ce/constants/ReduxActionConstants.tsx (2)
983-983: Great job adding the new action type constant! 👍The
FETCH_PUBLISHED_PAGE_RESOURCES_INITconstant is added to theAppViewActionTypesobject correctly. It follows the established naming convention and is placed in the appropriate location.
992-992: Nice work adding the error action type! 🙌The
FETCH_PUBLISHED_PAGE_RESOURCES_ERRORconstant is added to theAppViewActionErrorTypesobject as expected. It adheres to the naming convention and is positioned correctly within the object.app/server/appsmith-server/src/main/java/com/appsmith/server/newactions/base/NewActionServiceCEImpl.java (2)
733-736: Looks good!The updated method signature accurately reflects its purpose of fetching published actions by page ID while excluding certain plugin types. The additional
excludedPluginTypesparameter provides a useful filtering option.
778-787: Good job with the parameter validation and error handling!The null check for the
pageIdparameter ensures that an error is returned for invalid values, which is a good practice. Initializing theexcludedPluginTypeslist with "JS" plugin type aligns with the method's purpose of excluding certain plugin types. The method call looks fine, passing the necessary parameters.
|
@AmanAgarwal041 This would require a little bit of testing:
In both cases apps should work without any issues |
|
/build-deploy-preview skip-tests=true |
|
Deploying Your Preview: https://github.com/appsmithorg/appsmith/actions/runs/10697747261. |
|
Deploy-Preview-URL: https://ce-36096.dp.appsmith.com |
...smith-server/src/main/java/com/appsmith/server/services/ce/ConsolidatedAPIServiceCEImpl.java
Outdated
Show resolved
Hide resolved
...ppsmith-server/src/main/java/com/appsmith/server/newactions/base/NewActionServiceCEImpl.java
Outdated
Show resolved
Hide resolved
...ppsmith-server/src/main/java/com/appsmith/server/newactions/base/NewActionServiceCEImpl.java
Outdated
Show resolved
Hide resolved
|
This PR has increased the number of cyclic dependencies by 1, 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: 0
Outside diff range, codebase verification and nitpick comments (1)
app/server/appsmith-server/src/test/java/com/appsmith/server/services/ce/ActionServiceCE_Test.java (1)
Line range hint
699-755: Refactor the test method for better readability and maintainability.Consider breaking down the
checkActionInViewModetest method into smaller, focused test methods for each scenario. This will improve readability and maintainability of the test class.Additionally, consider adding more specific assertions to check for additional properties of the returned
ActionViewDTOobjects, such asjsonPathKeys,timeoutInMillisecond, etc., to ensure comprehensive testing.
Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Files selected for processing (2)
- app/server/appsmith-server/src/test/java/com/appsmith/server/services/ConsolidatedAPIServiceImplTest.java (1 hunks)
- app/server/appsmith-server/src/test/java/com/appsmith/server/services/ce/ActionServiceCE_Test.java (2 hunks)
Additional context used
Learnings (1)
app/server/appsmith-server/src/test/java/com/appsmith/server/services/ConsolidatedAPIServiceImplTest.java (1)
Learnt from: sumitsum PR: appsmithorg/appsmith#29875 File: app/server/appsmith-server/src/main/java/com/appsmith/server/services/ConsolidatedAPIServiceImpl.java:0-0 Timestamp: 2023-12-27T21:27:19.887Z Learning: Test cases for the `getConsolidatedInfoForPageLoad` method have already been added, and the refactoring of the method into smaller methods is planned for later and is being tracked in a separate GitHub issue.
Additional comments not posted (2)
app/server/appsmith-server/src/test/java/com/appsmith/server/services/ConsolidatedAPIServiceImplTest.java (1)
225-225: Good work on enhancing the test setup! This change makes the mock more realistic.Setting the ID of the
mockNewPageobject ensures that the mock instance has a defined ID, which is important for a couple of reasons:
The
mockNewPageis used to mock the behavior of thespyNewPageService. Having a more complete mock object makes the test more robust.If the functionality being tested depends on the page ID in any way, having a defined ID in the mock will make the test validate the behavior more accurately.
So this small change enhances the quality and reliability of the test. Well done!
app/server/appsmith-server/src/test/java/com/appsmith/server/services/ce/ActionServiceCE_Test.java (1)
Line range hint
755-772: LGTM!The
getActionInViewModetest method is well-written and focused. It covers the important aspects of testing thegetActionsForViewModefunctionality for a specific action.
|
This PR has increased the number of cyclic dependencies by 1, 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: 0
Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Files selected for processing (1)
- app/server/appsmith-server/src/main/java/com/appsmith/server/newactions/base/NewActionServiceCEImpl.java (2 hunks)
Additional comments not posted (2)
app/server/appsmith-server/src/main/java/com/appsmith/server/newactions/base/NewActionServiceCEImpl.java (2)
778-787: LGTM!The changes align with the updated
findAllPublishedActionsByPageIdAndExcludedPluginTypesmethod signature. Fetching published actions directly bypageIdshould improve the performance of this method.
733-736: Looks good! Verify the method usage across the codebase.The changes align with the PR objective to fetch actions specific to a page. Using the
pageIddirectly to fetch published actions for the current page should improve performance.Run the following script to verify the method usage:
Verification successful
Method usage verified successfully!
The method
findAllPublishedActionsByPageIdAndExcludedPluginTypesis correctly implemented and used with the new signature in theNewActionServiceCEImplclass. There are no other usages of this method in the codebase, indicating that the changes are isolated and should not affect other parts of the code. Everything looks good!Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify all method calls to `findAllPublishedActionsByPageIdAndExcludedPluginTypes` match the new signature. # Test: Search for the method usage. Expect: Only occurrences of the new signature. rg --type java -A 5 $'findAllPublishedActionsByPageIdAndExcludedPluginTypes'Length of output: 3016
|
Tested :
|
|
/build-deploy-preview skip-tests=true |
|
Deploying Your Preview: https://github.com/appsmithorg/appsmith/actions/runs/10720480695. |
|
Deploy-Preview-URL: https://ce-36096.dp.appsmith.com |
...smith-server/src/main/java/com/appsmith/server/services/ce/ConsolidatedAPIServiceCEImpl.java
Show resolved
Hide resolved
|
This PR has increased the number of cyclic dependencies by 1, 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: 2
Outside diff range, codebase verification and nitpick comments (1)
app/server/appsmith-server/src/main/java/com/appsmith/server/newactions/base/NewActionServiceCEImpl.java (1)
784-799: Ensure robust error handling for null or emptypageId.The method
getActionsForViewModeByPageIdcorrectly checks ifpageIdis null or empty and returns an error. However, the error message usesFieldName.PAGE_ID, which might be confusing since it's a generic field name. Consider using a more descriptive error message that directly addresses the context of the error.Improve the error message to enhance clarity:
- return Flux.error(new AppsmithException(AppsmithError.INVALID_PARAMETER, FieldName.PAGE_ID)); + return Flux.error(new AppsmithException(AppsmithError.INVALID_PARAMETER, "Page ID cannot be null or empty when fetching actions by page ID."));
Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Files selected for processing (6)
- app/server/appsmith-server/src/main/java/com/appsmith/server/newactions/base/NewActionServiceCE.java (1 hunks)
- app/server/appsmith-server/src/main/java/com/appsmith/server/newactions/base/NewActionServiceCEImpl.java (2 hunks)
- app/server/appsmith-server/src/main/java/com/appsmith/server/repositories/ce/CustomNewActionRepositoryCE.java (1 hunks)
- app/server/appsmith-server/src/main/java/com/appsmith/server/repositories/ce/CustomNewActionRepositoryCEImpl.java (1 hunks)
- app/server/appsmith-server/src/main/java/com/appsmith/server/services/ce/ConsolidatedAPIServiceCEImpl.java (3 hunks)
- app/server/appsmith-server/src/test/java/com/appsmith/server/services/ConsolidatedAPIServiceImplTest.java (2 hunks)
Files skipped from review as they are similar to previous changes (1)
- app/server/appsmith-server/src/main/java/com/appsmith/server/services/ce/ConsolidatedAPIServiceCEImpl.java
Additional context used
Learnings (1)
app/server/appsmith-server/src/test/java/com/appsmith/server/services/ConsolidatedAPIServiceImplTest.java (1)
Learnt from: sumitsum PR: appsmithorg/appsmith#29875 File: app/server/appsmith-server/src/main/java/com/appsmith/server/services/ConsolidatedAPIServiceImpl.java:0-0 Timestamp: 2023-12-27T21:27:19.887Z Learning: Test cases for the `getConsolidatedInfoForPageLoad` method have already been added, and the refactoring of the method into smaller methods is planned for later and is being tracked in a separate GitHub issue.
Additional comments not posted (10)
app/server/appsmith-server/src/main/java/com/appsmith/server/repositories/ce/CustomNewActionRepositoryCE.java (2)
40-42: Clarify the functionality of the new method signature.The method name
findPublishedActionsByPageIdAndExcludedPluginTypesuggests a change in functionality from the previousfindPublishedActionsByApplicationIdAndPluginType. Please confirm if the method now excludes certain plugin types from the results, as the name implies, and ensure this behavior is documented and tested.
43-43: Approve the updated method signature.The updated method signature for
findByApplicationIdnow acceptsOptional<AclPermission>andOptional<Sort>, enhancing flexibility and aligning with best practices. Please ensure that the usage of these optional parameters is well-documented in the method's Javadoc to guide developers on when and how to use them.app/server/appsmith-server/src/main/java/com/appsmith/server/newactions/base/NewActionServiceCE.java (2)
83-84: New Method Added:getActionsForViewModeByPageIdThe addition of
getActionsForViewModeByPageIdis a significant change that aligns with the PR's objective to optimize action fetching by focusing on individual pages rather than the entire application. This method should improve performance significantly, especially for applications with a large number of actions.However, it's crucial to ensure that this new method integrates seamlessly with existing functionalities. I recommend conducting thorough testing to verify that the new method does not introduce any regressions or compatibility issues, especially in scenarios where multiple pages are involved or where actions are heavily customized.
Verification successful
Integration and Testing of
getActionsForViewModeByPageIdThe method
getActionsForViewModeByPageIdis well-integrated into the existing codebase, as evidenced by its usage inConsolidatedAPIServiceCEImpland the presence of a unit test inConsolidatedAPIServiceImplTest. This suggests that the method is functioning as intended and does not introduce regressions.To ensure comprehensive testing, it would be prudent to verify the existence of integration tests that cover real-world scenarios involving this method. If such tests are not present, consider adding them to ensure robust verification of the method's behavior in a production-like environment.
- Integration Point:
ConsolidatedAPIServiceCEImpl- Test Coverage:
ConsolidatedAPIServiceImplTestScripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify integration of `getActionsForViewModeByPageId` with existing functionalities. # Test: Search for method usage in the codebase. Expect: Proper integration without breaking existing functionalities. rg --type java -A 5 $'getActionsForViewModeByPageId'Length of output: 3879
83-84: Method Removed:findAllByApplicationIdAndPluginTypeThe removal of
findAllByApplicationIdAndPluginTypemarks a significant shift in how actions are managed within the application, moving from an application-centric to a page-centric approach. This change should streamline the management of actions and potentially improve performance by reducing unnecessary data fetching.It is important to verify that the removal of this method does not negatively impact any existing functionalities that might have depended on it. I suggest reviewing all parts of the application that previously interacted with this method to ensure they have been appropriately updated or refactored to work with the new page-centric methods.
Verification successful
Method Successfully Removed:
findAllByApplicationIdAndPluginTypeThe method
findAllByApplicationIdAndPluginTypehas been successfully removed from the codebase. There are no remaining references or definitions, indicating a clean transition to the new page-centric approach for managing actions. This ensures that the application is not relying on outdated methods, aligning with the intended architectural changes.Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify the impact of removing `findAllByApplicationIdAndPluginType` on existing functionalities. # Test: Search for any remaining references to the removed method in the codebase. Expect: No unresolved references. rg --type java -A 5 $'findAllByApplicationIdAndPluginType'Length of output: 57
Script:
#!/bin/bash # Description: Confirm the removal of `findAllByApplicationIdAndPluginType` by searching for its definition in the codebase. # Test: Search for the definition of the removed method. Expect: No definitions found. rg --type java -A 5 'findAllByApplicationIdAndPluginType'Length of output: 57
app/server/appsmith-server/src/main/java/com/appsmith/server/repositories/ce/CustomNewActionRepositoryCEImpl.java (3)
215-223: Good practice: Including non-null check for page ID.The method
getCriterionForFindPublishedActionsByAppIdAndExcludedPluginTypeeffectively constructs query criteria that align with the new focus on page-specific actions. Including a non-null check for the page ID is a good practice, ensuring more precise and reliable query results.
227-233: Well-aligned with PR objectives: Focus on page-specific actions.The method
findPublishedActionsByPageIdAndExcludedPluginTypeis well-aligned with the PR's objectives, focusing on page-specific actions. This implementation enhances performance and is consistent with the new approach.
236-241: Effective construction of query criteria based on page ID.The method
getCriterionForFindPublishedActionsByPageIdAndExcludedPluginTypeeffectively constructs query criteria that focus on page-specific actions, aligning with the PR's objectives. The inclusion of conditions to exclude certain plugin types is well-implemented.app/server/appsmith-server/src/test/java/com/appsmith/server/services/ConsolidatedAPIServiceImplTest.java (2)
225-225: Proper setup of test objects is crucial.The addition of
mockNewPage.setId("mockPageId")ensures that themockNewPageobject has a defined ID, which is essential for accurately simulating the environment in which theConsolidatedAPIServiceoperates. This change is necessary for the integrity of the tests that depend on this setup.
261-261: Method signature update aligns with new functionality.The renaming of the method from
getActionsForViewModetogetActionsForViewModeByPageIdat line 261 is a crucial update that reflects the new logic of fetching actions based on thepageId. This change ensures that the tests are aligned with the modifications in the main codebase, thereby maintaining the relevance and accuracy of the tests.app/server/appsmith-server/src/main/java/com/appsmith/server/newactions/base/NewActionServiceCEImpl.java (1)
769-782: Verify the integration of new methods with existing functionalities.The methods
getActionsForViewModeandgetActionsForViewModeByPageIdhave been introduced or modified to fetch actions based onapplicationIdandpageIdrespectively. It's crucial to ensure that these changes integrate seamlessly with existing functionalities, especially given the shift from application-centric to page-centric action retrieval.Run the following script to verify the integration:
Also applies to: 784-799
Verification successful
Integration of New Methods Verified Successfully
The methods
getActionsForViewModeandgetActionsForViewModeByPageIdare well-integrated into the existing codebase. They are utilized in various service and controller classes and are covered by tests, ensuring that they function correctly without introducing breaking changes. This thorough testing and usage across the codebase confirm their seamless integration.
- Test Coverage: The methods are tested in
ConsolidatedAPIServiceImplTest.javaandActionServiceCE_Test.java.- Service and Controller Usage: They are used in
ConsolidatedAPIServiceCEImpl.javaandActionControllerCE.java.This integration ensures that the shift from application-centric to page-centric action retrieval is handled smoothly. Keep up the good work in maintaining robust test coverage for new functionalities!
Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify integration of new methods with existing functionalities. # Test: Search for method usages in the codebase. Expect: Proper integration without breaking changes. rg --type java -A 5 $'getActionsForViewMode|getActionsForViewModeByPageId'Length of output: 9099
...erver/src/main/java/com/appsmith/server/repositories/ce/CustomNewActionRepositoryCEImpl.java
Show resolved
Hide resolved
...ppsmith-server/src/main/java/com/appsmith/server/newactions/base/NewActionServiceCEImpl.java
Show resolved
Hide resolved
|
This PR has increased the number of cyclic dependencies by 1, 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: 0
Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Files selected for processing (1)
- app/client/src/ce/sagas/PageSagas.tsx (5 hunks)
Files skipped from review as they are similar to previous changes (1)
- app/client/src/ce/sagas/PageSagas.tsx
Thank you @subrata71 I will do that I have draft EE PR created, ill make sure all tests pass there as well. |
|
/build-deploy-preview skip-tests=true |
|
Deploying Your Preview: https://github.com/appsmithorg/appsmith/actions/runs/10733126547. |
@AmanAgarwal041 This issue has been fixed, could you please check it on latest DP? |
|
Deploy-Preview-URL: https://ce-36096.dp.appsmith.com |
The issue is resolved now! |
All cypress and Junit test cases have passed on draft EE PR: https://github.com/appsmithorg/appsmith-ee/pull/5041 |
|
|
||
| // Sending applicationId as empty as we have publishedActions present, | ||
| // it won't call the actions view api with applicationId | ||
| yield put(fetchActionsForView({ applicationId: "", publishedActions })); |
There was a problem hiding this comment.
@sneha122 if we send the applicationId here, in that case also because publishedActions is present it won't be fetching the actions. correct?
There was a problem hiding this comment.
Correct, as long as we have data in publishedActions, it won't fetch actions again
…horg#36096) ## Description This PR introduces a couple of improvements to the actions part of the consolidated view API. - With current implementation, we call consolidated view API only once during page load and fetch all of the resources at once. This can be a problem for a heavy app which has more than 500 actions, in this case fetching all published actions takes a sizeable amount of time. This PR introduces an improvement where we don't fetch all actions of an application at once, instead we fetch all actions of the current page and whenever user switches to different page, we call the consolidated view API again to fetch actions of the switched page. This way we can reduce the time taken by consolidated view API's action part performant by 5-10x. - With this new implementation, we use the basePageId passed to the consolidated view API and use that to fetch actions belonging to a page, there are two possibilities here: - If the app is not connected, basePageId can be used directly to fetch actions of a page - If app is git connected, first we need to fetch the pageId based on basePageId and branchName, then use that pageId to fetch actions belonging to the current page and branch name. Fixes #`Issue Number` _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.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/10722112357> > Commit: a725de6 > <a href="https://internal.appsmith.com/app/cypress-dashboard/rundetails-65890b3c81d7400d08fa9ee5?branch=master&workflowId=10722112357&attempt=1" target="_blank">Cypress dashboard</a>. > Tags: `@tag.All` > Spec: > <hr>Thu, 05 Sep 2024 15:19:53 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 - **New Features** - Introduced functionality to fetch actions specifically for pages, enhancing action management. - Improved action retrieval logic to focus on page-specific actions, streamlining user experience. - Added a new saga for fetching published page resources, enhancing data management capabilities. - **Bug Fixes** - Enhanced error handling for action fetching to ensure better user feedback during failures. <!-- end of auto-generated comment: release notes by coderabbit.ai --> --------- Co-authored-by: “sneha122” <“[email protected]”>
Description
This PR introduces a couple of improvements to the actions part of the consolidated view API.
With current implementation, we call consolidated view API only once during page load and fetch all of the resources at once. This can be a problem for a heavy app which has more than 500 actions, in this case fetching all published actions takes a sizeable amount of time. This PR introduces an improvement where we don't fetch all actions of an application at once, instead we fetch all actions of the current page and whenever user switches to different page, we call the consolidated view API again to fetch actions of the switched page. This way we can reduce the time taken by consolidated view API's action part performant by 5-10x.
With this new implementation, we use the basePageId passed to the consolidated view API and use that to fetch actions belonging to a page, there are two possibilities here:
- If the app is not connected, basePageId can be used directly to fetch actions of a page
- If app is git connected, first we need to fetch the pageId based on basePageId and branchName, then use that pageId to fetch actions belonging to the current page and branch name.
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
Tip
🟢 🟢 🟢 All cypress tests have passed! 🎉 🎉 🎉
Workflow run: https://github.com/appsmithorg/appsmith/actions/runs/10722112357
Commit: a725de6
Cypress dashboard.
Tags:
@tag.AllSpec:
Thu, 05 Sep 2024 15:19:53 UTC
Communication
Should the DevRel and Marketing teams inform users about this change?
Summary by CodeRabbit
Summary by CodeRabbit
New Features
Bug Fixes