fix: Plugin Action Editor navigation issue with git #37670
Conversation
WalkthroughThe Changes
Possibly related PRs
Suggested labels
Suggested reviewers
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
Documentation and Community
|
![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 and nitpick comments (1)
app/client/src/PluginActionEditor/components/PluginActionForm/hooks/useChangeActionCall.ts (1)
Line range hint
1-34: Consider adding TypeScript types for better type safetyAdding explicit types for the action object would help prevent similar issues in the future.
+ interface PluginAction { + id: string; + baseId: string; + pageId: string; + applicationId: string; + packageId: string; + moduleId: string; + workflowId: string; + } - const { action, plugin } = usePluginActionContext(); + const { action, plugin }: { action: PluginAction, plugin: Plugin } = usePluginActionContext();
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
📒 Files selected for processing (1)
app/client/src/PluginActionEditor/components/PluginActionForm/hooks/useChangeActionCall.ts(1 hunks)
🔇 Additional comments (1)
app/client/src/PluginActionEditor/components/PluginActionForm/hooks/useChangeActionCall.ts (1)
23-23: Verify the safety of accessing action.baseId
The change from action.id to action.baseId aligns with the PR objectives. However, we should ensure action.baseId is always defined.
✅ Verification successful
Let me search for the type definition of the action object to verify if baseId is a required field.
Let me try a different search pattern to find the type definition.
Let me try one more search with fixed regex to find the type definition.
Based on the search results, I found that baseId is a required field in the BaseAction interface which is the base interface for all action types. The change from action.id to action.baseId is safe as baseId is a required property. Additionally, the codebase shows consistent usage of baseId for identifying actions across many components and operations.
Change from action.id to action.baseId is safe and aligns with the codebase patterns
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for type definitions and usages of action.baseId
ast-grep --pattern 'interface $interface {
$$$
baseId: $_
$$$
}'
# Search for null checks on baseId in other files
rg 'baseId\s*[?!]?' -A 2
Length of output: 75928
Script:
#!/bin/bash
# Search for type definitions of action object
ast-grep --pattern 'interface $interface {
$$$
baseId: $_
$$$
}'
# Search for type definitions in the specific file's directory
ast-grep --pattern 'type $type = {
$$$
baseId: $_
$$$
}'
Length of output: 137
Script:
#!/bin/bash
# Search for type definitions in the specific file
rg -A 10 "type.*Action.*=|interface.*Action.*{" app/client/src/PluginActionEditor/
# Search for baseId usage in the file
rg -A 5 "baseId" app/client/src/PluginActionEditor/components/PluginActionForm/hooks/useChangeActionCall.ts
Length of output: 579
Script:
#!/bin/bash
# Search for Action type definitions
rg "type\s+.*Action.*=|interface\s+.*Action.*\{" -A 10
# Search for baseId property in the action object's usage context
rg "action\.(baseId|id)" -A 2 app/client/src/PluginActionEditor/
Length of output: 131195
hetunandu
dismissed stale reviews from ankitakinger and albinAppsmith
via
d6786de
There was a problem hiding this comment.
Actionable comments posted: 1
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
📒 Files selected for processing (1)
app/client/src/PluginActionEditor/components/PluginActionForm/hooks/useChangeActionCall.test.tsx(5 hunks)
🔇 Additional comments (2)
app/client/src/PluginActionEditor/components/PluginActionForm/hooks/useChangeActionCall.test.tsx (2)
56-56: LGTM! Test properly validates the fix
The test correctly validates that changeQuery now uses baseId instead of id, which aligns with the PR's objective of fixing the navigation issue.
Also applies to: 76-76, 85-85
36-36: Verify if API plugins should continue using action.id
While the PR aims to fix using baseActionId instead of actionId, this test shows API plugins still using action.id. Please verify if this is intentional.
✅ Verification successful
Using action.id for API plugins is correct
The implementation confirms that API plugins should indeed use action.id while non-API plugins use action.baseId. This is the intended behavior as evidenced by:
- The hook explicitly uses
action.idfor API plugins:dispatch(changeApi(action?.id, false)) - The
changeApiaction only accepts anidparameter, not abaseId - Only non-API plugins (default case) use
baseIdin thechangeQuerycall
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for changeApi usage in the codebase to verify if API plugins should use id or baseId
ast-grep --pattern 'changeApi($id, $_)'
Length of output: 41
Script:
#!/bin/bash
# Let's search for the hook implementation to understand the logic
ast-grep --pattern 'function useChangeActionCall() {
$$$
}'
# Also search for any references to changeApi
rg "changeApi" -A 3
Length of output: 6588
Script:
#!/bin/bash
# Let's look at the hook implementation to understand the complete logic
rg -A 20 "export const useChangeActionCall = \(\) => {" app/client/src/PluginActionEditor/components/PluginActionForm/hooks/useChangeActionCall.ts
Length of output: 300
Script:
#!/bin/bash
# Let's look at the hook implementation with fixed regex
rg -A 20 "export const useChangeActionCall = \(\)" app/client/src/PluginActionEditor/components/PluginActionForm/hooks/useChangeActionCall.ts
# Also check the changeApi action implementation
rg -A 10 "export const changeApi = \(" app/client/src/PluginActionEditor/store/pluginActionEditorActions.ts
Length of output: 1180
| @@ -106,7 +107,7 @@ describe("useChangeActionCall hook", () => { | |||
| }); | |||
|
|
|||
| it("should not dispatch any action if the action Id has not changed", () => { | |||
| const actionMock = { id: "actionId" }; | |||
| const actionMock = { id: "actionId", baseId: "baseActionId" }; | |||
There was a problem hiding this comment.
🛠️ Refactor suggestion
Add test coverage for baseId changes
While the test covers action.id changes, it should also verify behavior when only baseId changes, especially for non-API plugins.
Add this test case:
it("should dispatch changeQuery when baseId changes for non-API plugins", () => {
const pluginMock = { id: "pluginId", type: "OTHER_PLUGIN_TYPE" };
// First mount with initial baseId
const initialAction = {
id: "actionId",
baseId: "baseId1",
pageId: "pageId"
};
(usePluginActionContext as jest.Mock).mockReturnValueOnce({
action: initialAction,
plugin: pluginMock,
});
renderHook(() => useChangeActionCall());
// Change only baseId
const updatedAction = {
...initialAction,
baseId: "baseId2"
};
(usePluginActionContext as jest.Mock).mockReturnValueOnce({
action: updatedAction,
plugin: pluginMock,
});
renderHook(() => useChangeActionCall());
expect(changeQuery).toHaveBeenLastCalledWith({
baseQueryId: "baseId2",
basePageId: "pageId"
});
});
## Description Fixes the passing of wrong id when trying to trigger a changeQuery request. The scenario fixed was passing an actionId instead of a baseActionId. Later on, [a selector](https://github.com/appsmithorg/appsmith/blob/release/app/client/src/sagas/QueryPaneSagas.ts#L111) is not able to find the action as it expects a baseActionId and causes a navigation bug. ## Automation /ok-to-test tags="@tag.Sanity" ### 🔍 Cypress test results <!-- This is an auto-generated comment: Cypress test results --> > [!IMPORTANT] > 🟣 🟣 🟣 Your tests are running. > Tests running at: <https://github.com/appsmithorg/appsmith/actions/runs/12004477441> > Commit: b4e3cb0 > Workflow: `PR Automation test suite` > Tags: `@tag.Sanity` > Spec: `` > <hr>Mon, 25 Nov 2024 06:25:04 UTC <!-- end of auto-generated comment: Cypress test results --> ## Communication Should the DevRel and Marketing teams inform users about this change? - [ ] Yes - [x] No <!-- This is an auto-generated comment: release notes by coderabbit.ai --> ## Summary by CodeRabbit - **New Features** - Enhanced logic for action and plugin state management in the Plugin Action Editor. - Improved handling of action IDs for more efficient query dispatching. - **Bug Fixes** - Simplified initial checks to improve control flow and performance. <!-- end of auto-generated comment: release notes by coderabbit.ai -->
Description
Fixes the passing of wrong id when trying to trigger a changeQuery request. The scenario fixed was passing an actionId instead of a baseActionId.
Later on, a selector is not able to find the action as it expects a baseActionId and causes a navigation bug.
Automation
/ok-to-test tags="@tag.Sanity"
🔍 Cypress test results
Tip
🟢 🟢 🟢 All cypress tests have passed! 🎉 🎉 🎉
Workflow run: https://github.com/appsmithorg/appsmith/actions/runs/12005014920
Commit: d6786de
Cypress dashboard.
Tags:
@tag.SanitySpec:
Mon, 25 Nov 2024 07:52:21 UTC
Communication
Should the DevRel and Marketing teams inform users about this change?
Summary by CodeRabbit
New Features
baseIdproperty for better consistency in dispatch logic.Bug Fixes