fix: update newly created queries body with correct widget bindings #34248
Conversation
WalkthroughThe recent update focuses on enhancing the widget binding mechanism within building blocks. Changes include supplementary imports, modifications to existing functions, and introduction of new functions to handle widget name updates in queries and Redux actions. This ensures widget bindings synchronize correctly when building blocks are pasted. Changes
Sequence Diagram(s)sequenceDiagram
participant Frontend as Frontend
participant Backend as Backend
participant Saga as Redux Saga
Frontend->>Backend: Drag and drop building blocks
Backend-->>Frontend: Return newly added actions
Frontend->>Saga: Dispatch action with new widgets
Saga->>Saga: Call updateWidgetsNameInNewQueries
Saga->>Backend: PUT updated actions
Backend-->>Saga: Acknowledge
Saga->>Saga: Run added actions
Saga-->>Frontend: Update UI with new widget configurations
Assessment against linked issues
Poem
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 as PR comments)
Additionally, you can add CodeRabbit Configuration File (
|
|
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. |
…o feat/update-query-binding-for-building-block-dnd
|
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. |
|
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. |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 5
Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Files selected for processing (8)
- app/client/src/ce/api/ApplicationApi.tsx (1 hunks)
- app/client/src/ce/constants/ReduxActionConstants.tsx (1 hunks)
- app/client/src/ce/reducers/entityReducers/actionsReducer.tsx (1 hunks)
- app/client/src/sagas/ActionSagas.ts (1 hunks)
- app/client/src/sagas/BuildingBlockSagas/BuildingBlockAdditionSagas.ts (7 hunks)
- app/client/src/sagas/BuildingBlockSagas/tests/AddAndMoveBuildingBlockTests.test.ts (1 hunks)
- app/client/src/sagas/BuildingBlockSagas/tests/PasteBuildingBlockWidgetSaga.test.ts (1 hunks)
- app/client/src/sagas/BuildingBlockSagas/tests/fixtures.ts (2 hunks)
Additional comments not posted (12)
app/client/src/sagas/BuildingBlockSagas/tests/fixtures.ts (1)
105-153: ThenewlyCreatedActionsarray is well-structured and appears to follow the correct type specifications forAction[]. However, ensure that thepluginTypeandpluginIdare consistent with the system's plugin configurations.app/client/src/sagas/BuildingBlockSagas/tests/PasteBuildingBlockWidgetSaga.test.ts (2)
1-4: Ensure that the newly imported constants and types are used correctly within the test cases.
30-73: ThenewActionsarray is properly defined and used in the test scenarios. Make sure thepluginTypeandpluginIdare accurate and reflect the actual plugins used in the application.app/client/src/ce/reducers/entityReducers/actionsReducer.tsx (1)
152-157: The new case handler forAPPEND_ACTION_AFTER_BUILDING_BLOCK_DROPcorrectly appends the action data to thedraftMetaState. Ensure that theaction.payload.datastructure aligns with your state management expectations.app/client/src/sagas/BuildingBlockSagas/tests/AddAndMoveBuildingBlockTests.test.ts (3)
38-132: The test cases foraddAndMoveBuildingBlockToCanvasSagaare comprehensive and cover various steps and scenarios. Ensure that all edge cases and error handling paths are tested to maintain robustness.
135-224: The test cases foraddBuildingBlockToCanvasSagaare well-structured. Confirm that the mock data and expected outcomes align with the actual application state and behavior.
226-297: The tests foraddNewlyAddedActionsToReduxandupdateWidgetsNameInNewQueriesare critical for ensuring that new actions are correctly integrated and widget names are updated in queries. Verify that these functionalities work as expected in the live environment.
[APROVED]app/client/src/sagas/BuildingBlockSagas/BuildingBlockAdditionSagas.ts (1)
69-69: The new imports are appropriate and necessary for the added functionalities.Also applies to: 73-73, 76-76, 82-83
app/client/src/sagas/ActionSagas.ts (3)
580-581: The API call toupdateActionAPICallis appropriately used here to handle the action update process.
583-587: Proper response validation and error checking are implemented. Good use ofvalidateResponseto ensure data integrity andcheckAndLogErrorsIfCyclicDependencyto handle potential cyclic dependency issues.
588-589: Returning bothisValidResponseandresponseenhances the function's traceability and testability, facilitating potential further processing or debugging.app/client/src/ce/constants/ReduxActionConstants.tsx (1)
275-276: The addition ofAPPEND_ACTION_AFTER_BUILDING_BLOCK_DROPtoActionTypesappears correctly formatted and follows the existing naming conventions.
app/client/src/sagas/BuildingBlockSagas/BuildingBlockAdditionSagas.ts
Outdated
Show resolved
Hide resolved
app/client/src/sagas/BuildingBlockSagas/BuildingBlockAdditionSagas.ts
Outdated
Show resolved
Hide resolved
jacquesikot
added
Templates
Building blocks
There was a problem hiding this comment.
Actionable comments posted: 0
Outside diff range and nitpick comments (1)
app/client/src/ce/api/ApplicationApi.tsx (1)
Line range hint
283-502: Consider refactoring theApplicationApiclass to use simple functions instead of static methods.The class
ApplicationApionly contains static methods, which can be refactored into a set of standalone functions. This would reduce complexity and improve the modularity of the codebase. Here's a suggested change:- export class ApplicationApi extends Api { - static baseURL = "v1/applications"; - ... - } + export const ApplicationApi = { + baseURL: "v1/applications", + ... + }This approach treats
ApplicationApias an object literal rather than a class, simplifying the structure and potentially enhancing tree-shaking during the build process.
Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Files selected for processing (1)
- app/client/src/ce/api/ApplicationApi.tsx (2 hunks)
Additional context used
Biome
app/client/src/ce/api/ApplicationApi.tsx
[error] 283-502: Avoid classes that contain only static members. (lint/complexity/noStaticOnlyClass)
Prefer using simple functions instead of classes with only static members.
Additional comments not posted (1)
app/client/src/ce/api/ApplicationApi.tsx (1)
279-280: Ensure newly added propertiesnewActionListanddatasourceListare used appropriately in the application.The addition of
newActionListanddatasourceListtoImportBuildingBlockToApplicationResponsealigns with the PR's objectives to handle new actions and datasources after importing building blocks. This should facilitate better management of state and data consistency across the application.
There was a problem hiding this comment.
Actionable comments posted: 1
Outside diff range and nitpick comments (1)
app/client/src/sagas/BuildingBlockSagas/BuildingBlockAdditionSagas.ts (1)
Line range hint
770-770: The catch block that simply rethrows the error is redundant and does not add any value. Consider removing it to simplify the error handling logic.- throw error;
Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Files selected for processing (3)
- app/client/src/sagas/BuildingBlockSagas/BuildingBlockAdditionSagas.ts (7 hunks)
- app/client/src/sagas/BuildingBlockSagas/tests/AddAndMoveBuildingBlockTests.test.ts (1 hunks)
- app/client/src/sagas/BuildingBlockSagas/tests/fixtures.ts (2 hunks)
Files skipped from review as they are similar to previous changes (2)
- app/client/src/sagas/BuildingBlockSagas/tests/AddAndMoveBuildingBlockTests.test.ts
- app/client/src/sagas/BuildingBlockSagas/tests/fixtures.ts
Additional context used
Biome
app/client/src/sagas/BuildingBlockSagas/BuildingBlockAdditionSagas.ts
[error] 770-770: The catch clause that only rethrows the original error is redundant. (lint/complexity/noUselessCatch)
These unnecessary catch clauses can be confusing. It is recommended to remove them.
Additional comments not posted (3)
app/client/src/sagas/BuildingBlockSagas/BuildingBlockAdditionSagas.ts (3)
231-231: The handling of thenewActionListfrom the API response is crucial for ensuring that new actions are properly integrated. Ensure that all actions in this list are correctly processed and added to the Redux store.
904-934: While adding new actions to Redux, ensure that the action is not already present before making the API call. This prevents duplicate actions in the Redux store and unnecessary network calls.
69-69: Ensure proper import usage and check for any unnecessary or unused imports.Also applies to: 73-73, 76-76, 82-82, 83-83
Verification successful
The search indicates that the newly imported modules are indeed used within the file
BuildingBlockAdditionSagas.ts.
getActionis used within the file.No issues found regarding the usage of the imported modules.
Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify that all newly imported modules are used within the file. # Test: Search for the usage of newly imported modules. Expect: At least one usage for each import. rg --type tsx --files-with-matches 'getAction|getCurrentWorkspaceId|selectWidgetInitAction|apiCallToSaveAction|SelectionRequestType'Length of output: 162
Script:
#!/bin/bash # Description: Verify that all newly imported modules are used within the file `BuildingBlockAdditionSagas.ts`. # Test: Search for the usage of newly imported modules. Expect: At least one usage for each import. rg --type ts --files-with-matches 'getAction|getCurrentWorkspaceId|selectWidgetInitAction|apiCallToSaveAction|SelectionRequestType' app/client/src/sagas/BuildingBlockSagas/BuildingBlockAdditionSagas.tsLength of output: 273
app/client/src/sagas/BuildingBlockSagas/BuildingBlockAdditionSagas.ts
Outdated
Show resolved
Hide resolved
…o feat/update-query-binding-for-building-block-dnd
There was a problem hiding this comment.
Actionable comments posted: 0
Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Files selected for processing (1)
- app/client/src/ce/constants/ReduxActionConstants.tsx (1 hunks)
Files skipped from review as they are similar to previous changes (1)
- app/client/src/ce/constants/ReduxActionConstants.tsx
jacquesikot
added
ok-to-test
There was a problem hiding this comment.
Actionable comments posted: 2
Outside diff range and nitpick comments (3)
app/client/src/sagas/BuildingBlockSagas/index.ts (1)
Line range hint
14-50: Refactor suggestion forsaveBuildingBlockWidgetsToStoreto enhance readability and maintainability.The function
saveBuildingBlockWidgetsToStoreis quite complex and handles multiple operations. Consider breaking it down into smaller, more manageable functions. This will improve readability and maintainability. Additionally, error handling within the function could be enhanced to provide more specific messages related to the failure points.app/client/src/sagas/BuildingBlockSagas/BuildingBlockAdditionSagas.ts (2)
Line range hint
72-233: Consider simplifyingaddBuildingBlockActionsToApplicationfor clarity.The function
addBuildingBlockActionsToApplicationis complex and handles multiple operations, including API calls. Consider breaking it down into smaller functions to improve readability and maintainability. Additionally, enhance error handling to provide more specific messages related to the failure points.
Line range hint
414-772: RefactorpasteBuildingBlockWidgetsSagato reduce complexity and improve error handling.The function
pasteBuildingBlockWidgetsSagais highly complex and could benefit from being broken down into smaller, more manageable functions. Additionally, the catch clause that only rethrows the original error is redundant and can be removed to simplify the code.- } catch (error) { - throw error; - } + } catch (error) { + log.error("Error pasting building block widgets", error); + // Handle specific errors or perform cleanup here + }
Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Files selected for processing (3)
- app/client/src/sagas/BuildingBlockSagas/BuildingBlockAdditionSagas.ts (5 hunks)
- app/client/src/sagas/BuildingBlockSagas/index.ts (2 hunks)
- app/client/src/sagas/BuildingBlockSagas/tests/AddAndMoveBuildingBlockTests.test.ts (1 hunks)
Files skipped from review as they are similar to previous changes (1)
- app/client/src/sagas/BuildingBlockSagas/tests/AddAndMoveBuildingBlockTests.test.ts
Additional context used
Biome
app/client/src/sagas/BuildingBlockSagas/BuildingBlockAdditionSagas.ts
[error] 772-772: The catch clause that only rethrows the original error is redundant. (lint/complexity/noUselessCatch)
These unnecessary catch clauses can be confusing. It is recommended to remove them.
…o feat/update-query-binding-for-building-block-dnd
jacquesikot
added
ok-to-test
There was a problem hiding this comment.
Actionable comments posted: 0
Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Files selected for processing (1)
- app/client/src/sagas/BuildingBlockSagas/index.ts (2 hunks)
Files skipped from review as they are similar to previous changes (1)
- app/client/src/sagas/BuildingBlockSagas/index.ts
| }); | ||
| } | ||
|
|
||
| export function* addNewlyAddedActionsToRedux(actions: Action[]) { |
There was a problem hiding this comment.
lets add comments here explaining whats happening.
…o feat/update-query-binding-for-building-block-dnd
jacquesikot
added
ok-to-test
There was a problem hiding this comment.
Actionable comments posted: 0
Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Files selected for processing (2)
- app/client/src/sagas/BuildingBlockSagas/index.ts (2 hunks)
- app/client/src/sagas/BuildingBlockSagas/tests/PasteBuildingBlockWidgetSaga.test.ts (1 hunks)
Files skipped from review as they are similar to previous changes (2)
- app/client/src/sagas/BuildingBlockSagas/index.ts
- app/client/src/sagas/BuildingBlockSagas/tests/PasteBuildingBlockWidgetSaga.test.ts
…ppsmithorg#34248) ## Description When multiple blocks of the same type are dropped unto the canvas right after each other, the query body of the newly created query does not update the binding to the latest version of the widgets. ## Solution We have gotten the newlyUpdatedQueries from the block API, then we update the actionConfiguration.body and the jsonPathKeys to match the updated binding name for the block and implement the action in the local state. Fixes appsmithorg#34237 ## Automation /ok-to-test tags="@tag.Widget" ### 🔍 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/9644500854> > Commit: 562ad23 > <a href="https://internal.appsmith.com/app/cypress-dashboard/rundetails-65890b3c81d7400d08fa9ee5?branch=master&workflowId=9644500854&attempt=1" target="_blank">Cypress dashboard</a>. > Tags: `@tag.Widget` <!-- 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 functionality for saving building block widgets to the store. - **Tests** - Updated and added new test cases for pasting building block widgets. - **Bug Fixes** - Fixed issues with the import and usage of action types and selectors in the building block sagas. <!-- end of auto-generated comment: release notes by coderabbit.ai -->
Nikhil-Nandagopal
added
the
Accelerators Product
Description
When multiple blocks of the same type are dropped unto the canvas right after each other, the query body of the newly created query does not update the binding to the latest version of the widgets.
Solution
We have gotten the newlyUpdatedQueries from the block API, then we update the actionConfiguration.body and the jsonPathKeys to match the updated binding name for the block and implement the action in the local state.
Fixes #34237
Automation
/ok-to-test tags="@tag.Widget"
🔍 Cypress test results
Tip
🟢 🟢 🟢 All cypress tests have passed! 🎉 🎉 🎉
Workflow run: https://github.com/appsmithorg/appsmith/actions/runs/9644500854
Commit: 562ad23
Cypress dashboard.
Tags:
@tag.WidgetCommunication
Should the DevRel and Marketing teams inform users about this change?
Summary by CodeRabbit
New Features
Tests
Bug Fixes