feat: enable post run actions for plugin queries #39325
Conversation
If the action execute responds with post run action config, show enable the post run action container's view.
WalkthroughThis pull request introduces support for handling post-run actions. The changes update the Response component to conditionally render a new PostActionRunContainer based on the action response. New utility functions are added for validating and extracting post-run action details, which are incorporated into the API types. Corresponding tests for the UI components and utilities are provided, and component mapping definitions (along with re-exports) ensure post-run actions are rendered dynamically via a component map. Additionally, a saga function's accessibility is modified to support broader usage. Changes
Sequence Diagram(s)sequenceDiagram
participant R as Response Component
participant U as postRunActionsUtil (checkForPostRunAction)
participant C as PostActionRunContainer
participant M as PostRunActionComponentMap
participant G as getPostRunActionName
R->>U: call checkForPostRunAction(actionResponse?.postRunAction)
alt Valid post-run action found
R->>C: Render PostActionRunContainer with postRunAction
C->>G: Extract action name from postRunAction
G->>M: Map action name to UI Component
M-->>C: Return matched component
C-->>R: Render the modal component
else No valid post-run action
R-->>R: Skip rendering PostActionRunContainer
end
Suggested labels
Suggested reviewers
Poem
✨ Finishing Touches
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
|
This is to be used in other generator functions in EE repo.
github-actions
bot
added
Frontend
ayushpahwa
requested review from
jsartisan and
hetunandu
and removed request for
jsartisan
![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
🧹 Nitpick comments (7)
app/client/src/api/types.ts (1)
26-30: Consider making action type extensible for future use cases.The type literal "FORM" might limit future extensibility. Consider using a union type or string enum if there's potential for additional action types in the future.
- type: "FORM"; + type: PostActionRunType;Where PostActionRunType could be:
export type PostActionRunType = "FORM" | "MODAL" | "NOTIFICATION"; // Add more as needed // or export enum PostActionRunType { FORM = "FORM", // Add more types as needed }app/client/src/PluginActionEditor/components/PluginActionResponse/components/Response/utils/postRunActionsUtil.test.ts (1)
7-54: Add test cases for config property and invalid type values.While the current test coverage is good, consider adding:
- Tests for actions with config property
- Tests for invalid type values (other than "FORM")
it("should handle action with config", () => { const actionWithConfig: PostActionRunConfig = { type: "FORM", name: "test_action", config: { key: "value" } }; expect(checkForPostRunAction(actionWithConfig)).toBe(true); }); it("should return false for invalid type", () => { const invalidType = { type: "INVALID", name: "test_action" }; expect(checkForPostRunAction(invalidType as PostActionRunConfig)).toBe(false); });app/client/src/sagas/ActionExecution/PluginActionSaga.ts (1)
211-236: LGTM! Consider adding JSDoc.The function export change is appropriate. Consider adding JSDoc documentation to describe the function's purpose and return type for better maintainability.
+/** + * Creates an ActionResponse from an ActionExecutionResponse + * Handles binary data conversion from base64 when necessary + * @param response The execution response to process + * @returns The formatted action response + */ export const createActionExecutionResponse = (🧰 Tools
🪛 Biome (1.9.4)
[error] 216-216: Do not access Object.prototype method 'hasOwnProperty' from target object.
It's recommended using Object.hasOwn() instead of using Object.hasOwnProperty().
See MDN web docs for more details.(lint/suspicious/noPrototypeBuiltins)
[error] 220-220: Do not access Object.prototype method 'hasOwnProperty' from target object.
It's recommended using Object.hasOwn() instead of using Object.hasOwnProperty().
See MDN web docs for more details.(lint/suspicious/noPrototypeBuiltins)
app/client/src/PluginActionEditor/components/PluginActionResponse/components/Response/components/PostActionRunContainer.tsx (1)
12-22: Consider z-index stacking context.The container uses z-index: 100 which may cause stacking context issues. Consider using a z-index system with defined layers.
const Container = styled.div` border: 1px solid var(--ads-v2-color-border); - z-index: 100; + z-index: var(--ads-z-index-post-run-action); position: absolute;app/client/src/PluginActionEditor/components/PluginActionResponse/components/Response/utils/postRunActionsUtil.ts (2)
3-13: Strengthen type checking in checkForPostRunAction.The type checking could be more robust to handle edge cases.
export function checkForPostRunAction(postRunAction?: PostActionRunConfig) { if ( - postRunAction && + postRunAction !== null && + postRunAction !== undefined && typeof postRunAction === "object" && - "type" in postRunAction + "type" in postRunAction && + typeof postRunAction.type === "string" ) { return true; } return false; }
15-27: Simplify getPostRunActionName using optional chaining.The function can be more concise while maintaining the same functionality.
export function getPostRunActionName(postRunAction?: PostActionRunConfig) { - if (!postRunAction) { - return ""; - } - - const { name } = postRunAction; - - if (!name) { - return ""; - } - - return name; + return postRunAction?.name ?? ""; }app/client/src/PluginActionEditor/components/PluginActionResponse/components/Response/Response.test.tsx (1)
271-366: Reduce test data duplication.Test cases 7 and 8 have similar store and props setup. Consider extracting common setup to helper functions.
const createTestStore = (actionResponse: ActionResponse) => mockStore({ ...storeData, entities: { ...storeData.entities, actions: [{ config: { id: "test-action-id", name: "Test Action", }, isLoading: false, data: actionResponse, }], }, }); const createTestProps = (actionResponse: ActionResponse) => ({ ...defaultProps, actionResponse, });
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (12)
app/client/src/PluginActionEditor/components/PluginActionResponse/components/Response/Response.test.tsx(3 hunks)app/client/src/PluginActionEditor/components/PluginActionResponse/components/Response/Response.tsx(3 hunks)app/client/src/PluginActionEditor/components/PluginActionResponse/components/Response/components/PostActionRunContainer.tsx(1 hunks)app/client/src/PluginActionEditor/components/PluginActionResponse/components/Response/utils/postRunActionsUtil.test.ts(1 hunks)app/client/src/PluginActionEditor/components/PluginActionResponse/components/Response/utils/postRunActionsUtil.ts(1 hunks)app/client/src/api/ActionAPI.tsx(2 hunks)app/client/src/api/types.ts(1 hunks)app/client/src/ce/components/PostActionRunComponents/index.ts(1 hunks)app/client/src/ce/components/PostActionRunComponents/types.ts(1 hunks)app/client/src/ee/components/PostActionRunComponents/index.ts(1 hunks)app/client/src/ee/components/PostActionRunComponents/types.ts(1 hunks)app/client/src/sagas/ActionExecution/PluginActionSaga.ts(1 hunks)
✅ Files skipped from review due to trivial changes (2)
- app/client/src/ee/components/PostActionRunComponents/index.ts
- app/client/src/ce/components/PostActionRunComponents/types.ts
⏰ Context from checks skipped due to timeout of 90000ms (4)
- GitHub Check: perform-test / server-build / server-unit-tests
- GitHub Check: perform-test / client-build / client-build
- GitHub Check: perform-test / rts-build / build
- GitHub Check: client-unit-tests / client-unit-tests
🔇 Additional comments (6)
app/client/src/ee/components/PostActionRunComponents/types.ts (1)
3-4: LGTM! Type definition follows best practices.The implementation provides good type safety by extracting keys from PostRunActionNames.
app/client/src/ce/components/PostActionRunComponents/index.ts (1)
3-6:❓ Verification inconclusive
Verify if default components are needed.
The component map is empty. Verify if this is intentional or if default components should be provided.
🏁 Script executed:
#!/bin/bash # Search for any implementations or usages of PostRunActionComponentMap rg -l "PostRunActionComponentMap"Length of output: 323
Action Required: Confirm the Intentionality of the Empty Component Map
The
PostRunActionComponentMapis currently defined as an empty object, yet it is referenced in other parts of the codebase (including usage in tests and in thePostActionRunContainer.tsxcomponent). Please confirm whether leaving the map empty is intentional. If default components should be provided to handle various post-run actions, consider adding them or documenting why the map remains empty.app/client/src/api/ActionAPI.tsx (1)
12-12: LGTM! Clean interface extension.The addition of the
PostActionRunConfigtype import and thepostRunActionoptional property to theActionResponseinterface is well-structured and aligns with the PR objectives.Also applies to: 90-90
app/client/src/PluginActionEditor/components/PluginActionResponse/components/Response/Response.tsx (1)
35-36: LGTM! Clean integration of post-run actions.The PostActionRunContainer integration is well-structured with proper conditional rendering and type safety.
Also applies to: 131-132, 275-279
app/client/src/PluginActionEditor/components/PluginActionResponse/components/Response/Response.test.tsx (2)
31-36: LGTM! Mock setup is well-structured.The mock implementation correctly provides a test component with appropriate test-id.
220-269: LGTM! Test case thoroughly validates post-run action rendering.The test properly verifies both the container and the specific form component.
...or/components/PluginActionResponse/components/Response/components/PostActionRunContainer.tsx
Show resolved
Hide resolved
| @@ -208,7 +208,7 @@ export const getActionTimeout = ( | |||
| return undefined; | |||
| }; | |||
|
|
|||
| const createActionExecutionResponse = ( | |||
| export const createActionExecutionResponse = ( | |||
There was a problem hiding this comment.
this type should be placed somewhere else in a common place and not exported from here
There was a problem hiding this comment.
This is a util function, not a type. Should I move out this util function to a common file?
There was a problem hiding this comment.
Done, added coverage for the new func also.
ayushpahwa
requested review from
hetunandu and
ankitakinger
and removed request for
hetunandu
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
app/client/src/sagas/ActionExecution/PluginActionSagaUtils.ts (1)
48-51: Consider adding error handling for base64 decoding.The
atobfunction can throw if the input is not valid base64. Consider wrapping it in a try-catch block.Apply this diff to add error handling:
// Decoding from base64 to handle the binary files because direct // conversion of binary files to string causes corruption in the final output // this is to only handle the download of binary files - payload.body = atob(payload.body as string); + try { + payload.body = atob(payload.body as string); + } catch (e) { + console.error("Failed to decode base64 binary response:", e); + // Keep the original response on error + }
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
app/client/src/ee/components/PostActionRunComponents/types.ts(1 hunks)app/client/src/sagas/ActionExecution/PluginActionSaga.ts(2 hunks)app/client/src/sagas/ActionExecution/PluginActionSagaUtils.test.ts(2 hunks)app/client/src/sagas/ActionExecution/PluginActionSagaUtils.ts(2 hunks)
✅ Files skipped from review due to trivial changes (1)
- app/client/src/ee/components/PostActionRunComponents/types.ts
🚧 Files skipped from review as they are similar to previous changes (1)
- app/client/src/sagas/ActionExecution/PluginActionSaga.ts
🧰 Additional context used
🪛 Biome (1.9.4)
app/client/src/sagas/ActionExecution/PluginActionSagaUtils.ts
[error] 39-39: Do not access Object.prototype method 'hasOwnProperty' from target object.
It's recommended using Object.hasOwn() instead of using Object.hasOwnProperty().
See MDN web docs for more details.
(lint/suspicious/noPrototypeBuiltins)
[error] 43-43: Do not access Object.prototype method 'hasOwnProperty' from target object.
It's recommended using Object.hasOwn() instead of using Object.hasOwnProperty().
See MDN web docs for more details.
(lint/suspicious/noPrototypeBuiltins)
⏰ Context from checks skipped due to timeout of 90000ms (8)
- GitHub Check: perform-test / rts-build / build
- GitHub Check: perform-test / client-build / client-build
- GitHub Check: perform-test / server-build / server-unit-tests
- GitHub Check: client-unit-tests / client-unit-tests
- GitHub Check: client-lint / client-lint
- GitHub Check: client-check-cyclic-deps / check-cyclic-dependencies
- GitHub Check: client-prettier / prettier-check
- GitHub Check: client-build / client-build
🔇 Additional comments (2)
app/client/src/sagas/ActionExecution/PluginActionSagaUtils.test.ts (1)
107-240: LGTM! Comprehensive test coverage for the new functionality.The test suite thoroughly covers all edge cases for binary response handling, including:
- Regular response processing
- Base64 decoding
- Status code validation
- Header type validation
app/client/src/sagas/ActionExecution/PluginActionSagaUtils.ts (1)
8-10: LGTM! Clear enum definition.The enum
ActionResponseDataTypesis well-defined and follows TypeScript best practices.
| export const createActionExecutionResponse = ( | ||
| response: ActionExecutionResponse, | ||
| ): ActionResponse => { | ||
| const payload = response.data; | ||
|
|
||
| if (payload.statusCode === "200 OK" && payload.hasOwnProperty("headers")) { | ||
| const respHeaders = payload.headers; | ||
|
|
||
| if ( | ||
| respHeaders.hasOwnProperty(RESP_HEADER_DATATYPE) && | ||
| respHeaders[RESP_HEADER_DATATYPE].length > 0 && | ||
| respHeaders[RESP_HEADER_DATATYPE][0] === ActionResponseDataTypes.BINARY && | ||
| getType(payload.body) === Types.STRING | ||
| ) { | ||
| // Decoding from base64 to handle the binary files because direct | ||
| // conversion of binary files to string causes corruption in the final output | ||
| // this is to only handle the download of binary files | ||
| payload.body = atob(payload.body as string); | ||
| } | ||
| } | ||
|
|
||
| return { | ||
| ...payload, | ||
| ...response.clientMeta, | ||
| }; | ||
| }; |
There was a problem hiding this comment.
Replace hasOwnProperty calls with Object.hasOwn().
The current implementation uses the prototype method hasOwnProperty which can be unsafe. Use Object.hasOwn() instead for better security.
Apply this diff to fix the security issue:
- if (payload.statusCode === "200 OK" && payload.hasOwnProperty("headers")) {
+ if (payload.statusCode === "200 OK" && Object.hasOwn(payload, "headers")) {
const respHeaders = payload.headers;
if (
- respHeaders.hasOwnProperty(RESP_HEADER_DATATYPE) &&
+ Object.hasOwn(respHeaders, RESP_HEADER_DATATYPE) &&
respHeaders[RESP_HEADER_DATATYPE].length > 0 &&
respHeaders[RESP_HEADER_DATATYPE][0] === ActionResponseDataTypes.BINARY &&
getType(payload.body) === Types.STRING
) {📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| export const createActionExecutionResponse = ( | |
| response: ActionExecutionResponse, | |
| ): ActionResponse => { | |
| const payload = response.data; | |
| if (payload.statusCode === "200 OK" && payload.hasOwnProperty("headers")) { | |
| const respHeaders = payload.headers; | |
| if ( | |
| respHeaders.hasOwnProperty(RESP_HEADER_DATATYPE) && | |
| respHeaders[RESP_HEADER_DATATYPE].length > 0 && | |
| respHeaders[RESP_HEADER_DATATYPE][0] === ActionResponseDataTypes.BINARY && | |
| getType(payload.body) === Types.STRING | |
| ) { | |
| // Decoding from base64 to handle the binary files because direct | |
| // conversion of binary files to string causes corruption in the final output | |
| // this is to only handle the download of binary files | |
| payload.body = atob(payload.body as string); | |
| } | |
| } | |
| return { | |
| ...payload, | |
| ...response.clientMeta, | |
| }; | |
| }; | |
| export const createActionExecutionResponse = ( | |
| response: ActionExecutionResponse, | |
| ): ActionResponse => { | |
| const payload = response.data; | |
| if (payload.statusCode === "200 OK" && Object.hasOwn(payload, "headers")) { | |
| const respHeaders = payload.headers; | |
| if ( | |
| Object.hasOwn(respHeaders, RESP_HEADER_DATATYPE) && | |
| respHeaders[RESP_HEADER_DATATYPE].length > 0 && | |
| respHeaders[RESP_HEADER_DATATYPE][0] === ActionResponseDataTypes.BINARY && | |
| getType(payload.body) === Types.STRING | |
| ) { | |
| // Decoding from base64 to handle the binary files because direct | |
| // conversion of binary files to string causes corruption in the final output | |
| // this is to only handle the download of binary files | |
| payload.body = atob(payload.body as string); | |
| } | |
| } | |
| return { | |
| ...payload, | |
| ...response.clientMeta, | |
| }; | |
| }; |
🧰 Tools
🪛 Biome (1.9.4)
[error] 39-39: Do not access Object.prototype method 'hasOwnProperty' from target object.
It's recommended using Object.hasOwn() instead of using Object.hasOwnProperty().
See MDN web docs for more details.
(lint/suspicious/noPrototypeBuiltins)
[error] 43-43: Do not access Object.prototype method 'hasOwnProperty' from target object.
It's recommended using Object.hasOwn() instead of using Object.hasOwnProperty().
See MDN web docs for more details.
(lint/suspicious/noPrototypeBuiltins)
|
🔴🔴🔴 Cyclic Dependency Check: 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. You can view the dependency diff in the run log. Look for the check-cyclic-dependencies job in the run. |
There was a problem hiding this comment.
Actionable comments posted: 2
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
app/client/src/sagas/ActionExecution/PluginActionSagaUtils.test.ts(2 hunks)app/client/src/sagas/ActionExecution/PluginActionSagaUtils.ts(2 hunks)
🧰 Additional context used
🪛 Biome (1.9.4)
app/client/src/sagas/ActionExecution/PluginActionSagaUtils.ts
[error] 39-39: Do not access Object.prototype method 'hasOwnProperty' from target object.
It's recommended using Object.hasOwn() instead of using Object.hasOwnProperty().
See MDN web docs for more details.
(lint/suspicious/noPrototypeBuiltins)
[error] 43-43: Do not access Object.prototype method 'hasOwnProperty' from target object.
It's recommended using Object.hasOwn() instead of using Object.hasOwnProperty().
See MDN web docs for more details.
(lint/suspicious/noPrototypeBuiltins)
⏰ Context from checks skipped due to timeout of 90000ms (8)
- GitHub Check: perform-test / rts-build / build
- GitHub Check: perform-test / server-build / server-unit-tests
- GitHub Check: perform-test / client-build / client-build
- GitHub Check: client-lint / client-lint
- GitHub Check: client-check-cyclic-deps / check-cyclic-dependencies
- GitHub Check: client-unit-tests / client-unit-tests
- GitHub Check: client-prettier / prettier-check
- GitHub Check: client-build / client-build
🔇 Additional comments (5)
app/client/src/sagas/ActionExecution/PluginActionSagaUtils.test.ts (2)
1-8: LGTM! Well-organized imports.The imports are properly structured and include all necessary utilities for testing the response handling functionality.
109-141: LGTM! Comprehensive test coverage for regular responses.The test suite thoroughly validates the basic functionality of
createActionExecutionResponsewith well-structured test data and clear assertions.app/client/src/sagas/ActionExecution/PluginActionSagaUtils.ts (3)
3-11: LGTM! Clear type definitions and constants.The enum and constant definitions are clear and well-organized.
34-53: ReplacehasOwnPropertycalls withObject.hasOwn().The current implementation uses the prototype method
hasOwnPropertywhich can be unsafe. UseObject.hasOwn()instead for better security.🧰 Tools
🪛 Biome (1.9.4)
[error] 39-39: Do not access Object.prototype method 'hasOwnProperty' from target object.
It's recommended using Object.hasOwn() instead of using Object.hasOwnProperty().
See MDN web docs for more details.(lint/suspicious/noPrototypeBuiltins)
[error] 43-43: Do not access Object.prototype method 'hasOwnProperty' from target object.
It's recommended using Object.hasOwn() instead of using Object.hasOwnProperty().
See MDN web docs for more details.(lint/suspicious/noPrototypeBuiltins)
55-59: LGTM! Clean response object construction.The response object is constructed efficiently using spread operators.
Description
This PR enables the response pane of DB queries in appsmith to show a form container once the action is executed and the server returns with a
postRunActionconfig. The contents to be shown inside the container are controlled by thepostRunActionconfig. The config has a unique identifier which should be registered in the client. Based on this registry, client can render the required form inside the container. If no form is found, the container is not shown.Fixes #39402
Automation
/test sanity
🔍 Cypress test results
Tip
🟢 🟢 🟢 All cypress tests have passed! 🎉 🎉 🎉
Workflow run: https://github.com/appsmithorg/appsmith/actions/runs/13493868532
Commit: 75b1335
Cypress dashboard.
Tags:
@tag.SanitySpec:
Mon, 24 Feb 2025 09:13:40 UTC
Communication
Should the DevRel and Marketing teams inform users about this change?
Summary by CodeRabbit
New Features
Tests
Responsecomponent to validate the rendering logic based on post-run actions.