Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

chore: [Plugin Action Editor] Combine Plugin Editor UI state #36651

Open
wants to merge 11 commits into
base: release
Choose a base branch
from
Original file line number Diff line number Diff line change
@@ -1,25 +1,22 @@
import { useCallback } from "react";
import { useDispatch, useSelector } from "react-redux";
import { getApiPaneConfigSelectedTabIndex } from "selectors/apiPaneSelectors";
import { API_EDITOR_TABS } from "constants/ApiEditorConstants/CommonApiConstants";
import { setApiPaneConfigSelectedTabIndex } from "actions/apiPaneActions";
import {
getPluginActionConfigSelectedTab,
setPluginActionEditorSelectedTab,
} from "PluginActionEditor/store";

export function useSelectedFormTab(): [string, (id: string) => void] {
export function useSelectedFormTab(): [
string | undefined,
(id: string) => void,
] {
const dispatch = useDispatch();
// the redux form has been configured with indexes, but the new ads components need strings to work.
// these functions convert them back and forth as needed.
const selectedIndex = useSelector(getApiPaneConfigSelectedTabIndex);
const selectedValue = Object.values(API_EDITOR_TABS)[selectedIndex];
const setSelectedIndex = useCallback(
const selectedValue = useSelector(getPluginActionConfigSelectedTab);
const setSelectedTab = useCallback(
(value: string) => {
const index = Object.values(API_EDITOR_TABS).indexOf(
value as API_EDITOR_TABS,
);

dispatch(setApiPaneConfigSelectedTabIndex(index));
dispatch(setPluginActionEditorSelectedTab(value));
},
[dispatch],
);

return [selectedValue, setSelectedIndex];
return [selectedValue, setSelectedTab];
}
Original file line number Diff line number Diff line change
Expand Up @@ -2,20 +2,16 @@ import { renderHook } from "@testing-library/react-hooks/dom";
import { useDispatch } from "react-redux";
import { PluginType } from "entities/Action";
import { usePluginActionContext } from "PluginActionEditor";
import { changeApi } from "actions/apiPaneActions";
import { changeQuery } from "actions/queryPaneActions";
import { changeApi, changeQuery } from "../../../store";
ankitakinger marked this conversation as resolved.
Show resolved Hide resolved
import usePrevious from "utils/hooks/usePrevious";
import { useChangeActionCall } from "./useChangeActionCall";

jest.mock("react-redux", () => ({
useDispatch: jest.fn(),
}));

jest.mock("actions/apiPaneActions", () => ({
jest.mock("../../../store", () => ({
changeApi: jest.fn(),
}));

jest.mock("actions/queryPaneActions", () => ({
changeQuery: jest.fn(),
}));

Expand Down
Original file line number Diff line number Diff line change
@@ -1,9 +1,8 @@
import { useEffect } from "react";
import { useDispatch } from "react-redux";
import { changeApi } from "actions/apiPaneActions";
import { changeQuery } from "actions/queryPaneActions";
import { PluginType } from "entities/Action";
import { usePluginActionContext } from "PluginActionEditor";
import { changeApi, changeQuery } from "PluginActionEditor/store";
import usePrevious from "utils/hooks/usePrevious";

export const useChangeActionCall = () => {
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -3,9 +3,9 @@ import { IDEBottomView, ViewHideBehaviour } from "IDE";
import { ActionExecutionResizerHeight } from "pages/Editor/APIEditor/constants";
import EntityBottomTabs from "components/editorComponents/EntityBottomTabs";
import { useDispatch, useSelector } from "react-redux";
import { getApiPaneDebuggerState } from "selectors/apiPaneSelectors";
import { setApiPaneDebuggerState } from "actions/apiPaneActions";
import { DEBUGGER_TAB_KEYS } from "components/editorComponents/Debugger/helpers";
import { setPluginActionEditorDebuggerState } from "../../store";
import { getPluginActionDebuggerState } from "../../store";
ankitakinger marked this conversation as resolved.
Show resolved Hide resolved
import { DEBUGGER_TAB_KEYS } from "components/editorComponents/Debugger/constants";
import AnalyticsUtil from "ee/utils/AnalyticsUtil";
import { usePluginActionResponseTabs } from "./hooks";

Expand All @@ -16,11 +16,11 @@ function PluginActionResponse() {

// TODO combine API and Query Debugger state
const { open, responseTabHeight, selectedTab } = useSelector(
getApiPaneDebuggerState,
getPluginActionDebuggerState,
);

const toggleHide = useCallback(
() => dispatch(setApiPaneDebuggerState({ open: !open })),
() => dispatch(setPluginActionEditorDebuggerState({ open: !open })),
[dispatch, open],
);

Expand All @@ -32,14 +32,18 @@ function PluginActionResponse() {
});
}

dispatch(setApiPaneDebuggerState({ open: true, selectedTab: tabKey }));
dispatch(
setPluginActionEditorDebuggerState({ open: true, selectedTab: tabKey }),
);
},
[dispatch],
);

const updateResponsePaneHeight = useCallback(
(height: number) => {
dispatch(setApiPaneDebuggerState({ responseTabHeight: height }));
dispatch(
setPluginActionEditorDebuggerState({ responseTabHeight: height }),
);
},
[dispatch],
);
Expand Down
1 change: 1 addition & 0 deletions app/client/src/PluginActionEditor/constants.ts
Original file line number Diff line number Diff line change
@@ -0,0 +1 @@
export const POST_BODY_FORM_DATA_KEY = "displayFormat";
2 changes: 2 additions & 0 deletions app/client/src/PluginActionEditor/index.ts
Original file line number Diff line number Diff line change
Expand Up @@ -11,3 +11,5 @@ export type {
PluginActionNameEditorProps,
} from "./components/PluginActionNameEditor";
export { default as PluginActionNameEditor } from "./components/PluginActionNameEditor";

export type { PluginActionEditorState } from "./store/pluginEditorReducer";
4 changes: 4 additions & 0 deletions app/client/src/PluginActionEditor/store/index.ts
Original file line number Diff line number Diff line change
@@ -0,0 +1,4 @@
export { default as pluginActionReducer } from "./pluginEditorReducer";

export * from "./pluginActionEditorActions";
export * from "./pluginActionEditorSelectors";
Original file line number Diff line number Diff line change
@@ -0,0 +1,57 @@
import type { PluginEditorDebuggerState } from "./pluginEditorReducer";
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

store
actions
reducer
selector

^ is this folder pattern we are establishing to use/refactor consistently everywhere?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

+1. There are so many other flows which we are not handling in Action redesign (like admin settings), won't the folder structure for those be different?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I want to make sure the store is near the feature and this makes sense to me. This would be a gradual change as we start making more modular features. @ankitakinger can you share why this does not work ? Please suggest any alternatives you can think of

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm not saying it won't work. My point is about consistency, since we are not going to touch other flows.

import {
type ReduxAction,
ReduxActionTypes,
} from "ee/constants/ReduxActionConstants";
import type { Action } from "entities/Action";

export const setPluginActionEditorDebuggerState = (
payload: Partial<PluginEditorDebuggerState>,
) => ({
type: ReduxActionTypes.SET_PLUGIN_ACTION_EDITOR_DEBUGGER_STATE,
payload,
});

export const setPluginActionEditorSelectedTab = (payload: string) => ({
type: ReduxActionTypes.SET_PLUGIN_ACTION_EDITOR_FORM_SELECTED_TAB,
payload: {
selectedTab: payload,
},
});

export const updatePostBodyContentType = (
title: string,
apiId: string,
): ReduxAction<{ title: string; apiId: string }> => ({
type: ReduxActionTypes.UPDATE_API_ACTION_BODY_CONTENT_TYPE,
payload: { title, apiId },
});

export const changeApi = (
id: string,
isSaas: boolean,
newApi?: boolean,
): ReduxAction<{ id: string; isSaas: boolean; newApi?: boolean }> => {
return {
type: ReduxActionTypes.API_PANE_CHANGE_API,
payload: { id, isSaas, newApi },
};
};

export interface ChangeQueryPayload {
baseQueryId: string;
packageId?: string;
applicationId?: string;
basePageId?: string;
moduleId?: string;
workflowId?: string;
newQuery?: boolean;
action?: Action;
}

export const changeQuery = (payload: ChangeQueryPayload) => {
return {
type: ReduxActionTypes.QUERY_PANE_CHANGE,
payload,
};
};
Original file line number Diff line number Diff line change
@@ -0,0 +1,61 @@
import type { AppState } from "ee/reducers";
import { createSelector } from "reselect";

import { POST_BODY_FORM_DATA_KEY } from "../constants";

export const getActionEditorSavingMap = (state: AppState) =>
state.ui.pluginActionEditor.isSaving;
Comment on lines +6 to +7
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🛠️ Refactor suggestion

Consider Adding Explicit Return Types to Exported Functions for Clarity

It's beneficial to explicitly specify return types for your exported functions. This practice enhances code readability and type safety, making it clearer for others (and your future self) to understand what each function returns. It can also help catch unintended type errors during development.

For example, you can define the return type of getActionEditorSavingMap as Record<string, boolean>:

-export const getActionEditorSavingMap = (state: AppState) =>
+export const getActionEditorSavingMap = (state: AppState): Record<string, boolean> =>
  state.ui.pluginActionEditor.isSaving;
📝 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.

Suggested change
export const getActionEditorSavingMap = (state: AppState) =>
state.ui.pluginActionEditor.isSaving;
export const getActionEditorSavingMap = (state: AppState): Record<string, boolean> =>
state.ui.pluginActionEditor.isSaving;


export const isActionSaving = (id: string) =>
createSelector([getActionEditorSavingMap], (savingMap) => {
return id in savingMap && savingMap[id];
});

const getActionDirtyState = (state: AppState) =>
state.ui.pluginActionEditor.isDirty;

export const isActionDirty = (id: string) =>
createSelector([getActionDirtyState], (actionDirtyMap) => {
return id in actionDirtyMap && actionDirtyMap[id];
});

const getActionRunningState = (state: AppState) =>
state.ui.pluginActionEditor.isRunning;

export const getActionIsRunning = (id: string) =>
createSelector(
[getActionRunningState],
(isRunningMap) => id in isRunningMap && isRunningMap[id],
);
Comment on lines +25 to +29
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🛠️ Refactor suggestion

Let's Rename getActionIsRunning to isActionRunning for Consistency

Consistency in naming conventions greatly aids in code comprehension and maintenance. Since you've used the isAction... prefix for other boolean selectors like isActionSaving and isActionDirty, renaming getActionIsRunning to isActionRunning will align with this pattern. This makes it immediately apparent that the function returns a boolean value indicating a state.

Here's how you might adjust the code:

-export const getActionIsRunning = (id: string) =>
+export const isActionRunning = (id: string) =>
  createSelector(
    [getActionRunningState],
    (isRunningMap) => id in isRunningMap && isRunningMap[id],
  );
📝 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.

Suggested change
export const getActionIsRunning = (id: string) =>
createSelector(
[getActionRunningState],
(isRunningMap) => id in isRunningMap && isRunningMap[id],
);
export const isActionRunning = (id: string) =>
createSelector(
[getActionRunningState],
(isRunningMap) => id in isRunningMap && isRunningMap[id],
);


const getActionDeletingState = (state: AppState) =>
state.ui.pluginActionEditor.isDeleting;

export const getActionIsDeleting = (id: string) =>
createSelector(
[getActionDeletingState],
(deletingMap) => id in deletingMap && deletingMap[id],
);
Comment on lines +34 to +38
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🛠️ Refactor suggestion

Let's Rename getActionIsDeleting to isActionDeleting for Clarity

Maintaining a consistent naming scheme helps others quickly grasp the purpose of your functions. By renaming getActionIsDeleting to isActionDeleting, you align it with your other selectors like isActionSaving and isActionDirty. This uniformity reinforces the understanding that the function checks a boolean state.

You can modify the code as follows:

-export const getActionIsDeleting = (id: string) =>
+export const isActionDeleting = (id: string) =>
  createSelector(
    [getActionDeletingState],
    (deletingMap) => id in deletingMap && deletingMap[id],
  );
📝 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.

Suggested change
export const getActionIsDeleting = (id: string) =>
createSelector(
[getActionDeletingState],
(deletingMap) => id in deletingMap && deletingMap[id],
);
export const isActionDeleting = (id: string) =>
createSelector(
[getActionDeletingState],
(deletingMap) => id in deletingMap && deletingMap[id],
);


type GetFormData = (
state: AppState,
id: string,
) => { label: string; value: string } | undefined;

export const getPostBodyFormat: GetFormData = (state, id) => {
const formData = state.ui.pluginActionEditor.formData;

if (id in formData) {
return formData[id][POST_BODY_FORM_DATA_KEY];
}

return undefined;
};
export const getPluginActionConfigSelectedTab = (state: AppState) =>
state.ui.pluginActionEditor.selectedConfigTab;

export const getPluginActionDebuggerState = (state: AppState) =>
state.ui.pluginActionEditor.debugger;

export const isPluginActionCreating = (state: AppState) =>
state.ui.pluginActionEditor.isCreating;
Loading
Loading