chore: Plugin Action Editor Context #36187
Conversation
WalkthroughThe pull request introduces a new Plugin Action context and a modular Plugin Action Editor component, consolidating functionalities previously split between the ApiEditor and QueryEditor. It includes the creation of several components such as Changes
Sequence Diagram(s)sequenceDiagram
participant User
participant APIEditor
participant AppPluginActionEditor
participant PluginActionEditor
participant PluginActionContextProvider
User->>APIEditor: Access API Editor
APIEditor->>+AppPluginActionEditor: Check feature flag
AppPluginActionEditor->>+PluginActionContextProvider: Provide context
PluginActionContextProvider->>+PluginActionEditor: Render Plugin Action Editor
PluginActionEditor-->>-AppPluginActionEditor: Return rendered component
AppPluginActionEditor-->>-APIEditor: Return rendered editor
User->>APIEditor: View Plugin Action Editor
Assessment against linked issues
Possibly related issues
Poem
Recent review detailsConfiguration used: .coderabbit.yaml Files selected for processing (2)
Files skipped from review due to trivial changes (1)
Files skipped from review as they are similar to previous changes (1)
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 2, when compared with the release branch. Refer this document to identify the cyclic dependencies introduced by this PR. |
github-actions
bot
added
IDE Pod
|
This PR has increased the number of cyclic dependencies by 2, when compared with the release branch. Refer this document to identify the cyclic dependencies introduced by this PR. |
hetunandu
added
ok-to-test
hetunandu
requested review from
AmanAgarwal041,
ankitakinger,
albinAppsmith and
ashit-rath
![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 (1)
app/client/src/PluginActionEditor/PluginActionEditor.tsx (1)
1-72: Comprehensive and robust implementation ofPluginActionEditorwith good use of Redux and React Router.The
PluginActionEditor.tsxfile effectively uses Redux selectors and React Router to manage and render the state based on the application's current URL and state. The implementation of conditional rendering to handle different states like initialization and entity not found scenarios is well thought out. The use ofPluginActionContextProviderto wrap the children with the necessary data is a good practice, ensuring that all child components have access to the required data.However, consider handling the potential for
nullvalues inactionandpluginmore gracefully. Currently, the component will throw an error and stop rendering if these values are not found, which might not be the best user experience. Perhaps implementing a fallback or a more detailed error message could improve the robustness of this component.
Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Files selected for processing (12)
- app/client/src/PluginActionEditor/PluginActionContext.tsx (1 hunks)
- app/client/src/PluginActionEditor/PluginActionEditor.tsx (1 hunks)
- app/client/src/PluginActionEditor/components/PluginActionForm/PluginActionForm.tsx (1 hunks)
- app/client/src/PluginActionEditor/components/PluginActionForm/index.ts (1 hunks)
- app/client/src/PluginActionEditor/components/PluginActionResponsePane.tsx (1 hunks)
- app/client/src/PluginActionEditor/components/PluginActionToolbar.tsx (1 hunks)
- app/client/src/PluginActionEditor/index.ts (1 hunks)
- app/client/src/ce/pages/Editor/AppPluginActionEditor/AppPluginActionEditor.tsx (1 hunks)
- app/client/src/ee/pages/Editor/AppPluginActionEditor/AppPluginActionEditor.tsx (1 hunks)
- app/client/src/pages/Editor/APIEditor/index.tsx (2 hunks)
- app/client/src/pages/Editor/AppPluginActionEditor/index.ts (1 hunks)
- app/client/src/pages/Editor/QueryEditor/index.tsx (2 hunks)
Files skipped from review due to trivial changes (7)
- app/client/src/PluginActionEditor/components/PluginActionForm/PluginActionForm.tsx
- app/client/src/PluginActionEditor/components/PluginActionForm/index.ts
- app/client/src/PluginActionEditor/components/PluginActionResponsePane.tsx
- app/client/src/PluginActionEditor/components/PluginActionToolbar.tsx
- app/client/src/PluginActionEditor/index.ts
- app/client/src/ee/pages/Editor/AppPluginActionEditor/AppPluginActionEditor.tsx
- app/client/src/pages/Editor/AppPluginActionEditor/index.ts
Additional comments not posted (6)
app/client/src/ce/pages/Editor/AppPluginActionEditor/AppPluginActionEditor.tsx (1)
1-19: Well-structured and clean implementation forAppPluginActionEditor.The file
AppPluginActionEditor.tsxintroduces a new component that uses thePluginActionEditoralong with its child componentsPluginActionToolbar,PluginActionForm, andPluginActionResponsePane. This setup is clean and follows React's best practices for component composition. The use of explicit imports fromPluginActionEditorensures that only necessary components are bundled, which is good for performance.app/client/src/PluginActionEditor/PluginActionContext.tsx (1)
1-62: Excellent setup ofPluginActionContextwith robust typing and error handling.The
PluginActionContext.tsxfile does an excellent job of setting up a new React context tailored for Plugin Actions. The use of TypeScript interfaces to define the shape of the context and props adds a layer of type safety that is crucial for larger applications. The implementation ofusePluginActionContexthook with error handling ensures that the context is consumed correctly within its provider, preventing runtime errors in a larger application setup.The use of
useMemoto memoize the context value is a best practice that helps in optimizing performance by preventing unnecessary re-renders. This is particularly important in a context where the values likeaction,datasource, andpluginmight change frequently and are derived from props.app/client/src/pages/Editor/APIEditor/index.tsx (2)
41-41: Review the import statement forAppPluginActionEditor.The import statement correctly references the
AppPluginActionEditorfrom its location. This is a good practice to ensure that components are modular and can be reused efficiently. However, let's ensure that the path is correctly maintained and that there are no typos or incorrect paths that could lead to runtime errors.
181-187: Examine the feature flag implementation for conditional rendering.The implementation uses a feature flag to conditionally render the
AppPluginActionEditor. This is a sound approach to manage feature releases and ensure that new features do not impact all users immediately. The use ofuseFeatureFlaghook is appropriate here to check the state of the feature flag.However, it's important to ensure that the feature flag is well-documented and that its usage here aligns with how feature flags are managed across the application. Additionally, consider adding a fallback or error handling if the feature flag service fails or the flag is undefined, to improve the robustness of the code.
app/client/src/pages/Editor/QueryEditor/index.tsx (2)
44-44: Review the import statement forAppPluginActionEditor.Just like in the APIEditor, the import statement correctly references the
AppPluginActionEditorfrom its location. It's crucial to maintain consistency in how components are imported across different modules. This import statement appears to be correct, but always double-check for any potential typos or path errors that could lead to runtime issues.
192-197: Examine the feature flag implementation for conditional rendering.The implementation here is similar to that in the APIEditor, using the same feature flag to conditionally render the
AppPluginActionEditor. This consistency is good as it helps maintain a uniform approach across different parts of the application.As with the APIEditor, ensure that the feature flag is well-documented and managed consistently across the application. It would also be wise to add error handling for cases where the feature flag service might fail or the flag is undefined, to prevent any disruptions in the user experience.
Verification successful
Feature Flag Usage Verified Across Application
The feature flag
release_actions_redesign_enabledis consistently used across various parts of the application, including the QueryEditor, APIEditor, and other components. This consistency helps maintain a uniform approach, as highlighted in the review comment.
app/client/src/pages/Editor/QueryEditor/index.tsxapp/client/src/pages/Editor/QueryEditor/EditorJSONtoForm.tsxapp/client/src/pages/Editor/IDE/EditorPane/components/ActionToolbar.tsxapp/client/src/pages/Editor/APIEditor/index.tsxapp/client/src/ce/entities/FeatureFlag.tsapp/client/src/components/formControls/DynamicTextFieldControl.tsxConsider implementing error handling for scenarios where the feature flag service might fail or the flag is undefined, to enhance user experience.
Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify the feature flag usage consistency across the application. # Test: Search for the feature flag usage. Expect: Consistent usage across the application. rg --type typescript "release_actions_redesign_enabled"Length of output: 90
Script:
#!/bin/bash # Description: Verify the feature flag usage consistency across the application. # Test: Search for the feature flag usage. Expect: Consistent usage across the application. rg --type ts "release_actions_redesign_enabled"Length of output: 952
| } | ||
|
|
||
| // No need to export this context to use it. Use the hook defined below instead | ||
| const PluginActionContext = createContext<PluginActionContext | null>(null); |
There was a problem hiding this comment.
Interface and const have same name. I suggest keeping it different.
| return <EntityNotFoundPane />; | ||
| } | ||
| if (!settingsConfig || !editorConfig) { | ||
| throw Error("Plugin config for action not found"); |
There was a problem hiding this comment.
Should we show the same not found component here? Or a better component to handle this error.
| } | ||
|
|
||
| const PluginActionEditor = (props: ChildrenProps) => { | ||
| const { pathname } = useLocation(); |
There was a problem hiding this comment.
With this refactor, should we rely on the path to identify the action? Can we pass the action as a prop to the PluginActionEditor?
Reason for asking this is the path has to conform to a particular standard for this editor to work (path should have :entityId or :queryId). If we make this agnostic then this editor can be rendered anywhere and has no dependency on the url
There was a problem hiding this comment.
Another suggestion: AFAIK each action now has a context object to define which parent it belongs to. Maybe we can use that instead of the URL. (I don't know if this functionality exists for packages however, need to confirm with someone from backend team)
There was a problem hiding this comment.
I will make this change in the next PR to update the PluginActionEditor
| datasource={datasource} | ||
| editorConfig={editorConfig} | ||
| plugin={plugin} | ||
| settingsConfig={settingsConfig} |
There was a problem hiding this comment.
If you check ModuleQueryEditor, the settingsConfig is passed as a prop to the editor.
There was a problem hiding this comment.
What I meant was the Module editors show only specific settings for each pluginType hence they are passed from top bottom to decouple and avoid if-else inside the editors. if the plan is to remove that then what's the plan to keep the editor config agnostic?
Check useModuleFallbackSettingsForm.ts for more info.
There was a problem hiding this comment.
Got it. Yup I have this considered. Will make this close to the PluginActionToolbar where this will be used
| } | ||
|
|
||
| const PluginActionEditor = (props: ChildrenProps) => { | ||
| const { pathname } = useLocation(); |
There was a problem hiding this comment.
Another suggestion: AFAIK each action now has a context object to define which parent it belongs to. Maybe we can use that instead of the URL. (I don't know if this functionality exists for packages however, need to confirm with someone from backend team)
| return <EntityNotFoundPane />; | ||
| } | ||
| if (!settingsConfig || !editorConfig) { | ||
| throw Error("Plugin config for action not found"); |
@ayushpahwa we are planning to add that business logic closer to the actual functionality. So there will be a new component created called |
Description
Fixes #36152
Automation
/ok-to-test tags="@tag.Datasource"
🔍 Cypress test results
Tip
🟢 🟢 🟢 All cypress tests have passed! 🎉 🎉 🎉
Workflow run: https://github.com/appsmithorg/appsmith/actions/runs/10805408539
Commit: 1b8259b
Cypress dashboard.
Tags:
@tag.DatasourceSpec:
Wed, 11 Sep 2024 05:53:02 UTC
Communication
Should the DevRel and Marketing teams inform users about this change?
Summary by CodeRabbit
New Features
PluginActionEditorcomponent for managing plugin actions.PluginActionContextfor improved state management of plugin actions.ApiEditorWrapperandQueryEditorcomponents.Documentation
Bug Fixes
PluginActionEditor.