Skip to content

feat: creation of datasource and query for ai chat #36785

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.

Sign up for GitHub

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

Closed
KelvinOm wants to merge 3 commits into from
Closed

feat: creation of datasource and query for ai chat #36785

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
5fed2f6
chore: test createDatasourceFromFormSaga
KelvinOm Oct 7, 2024
d13c472
chore: creating a datasource and a quarry after chat widget drop
KelvinOm Oct 9, 2024
3066160
chore: update widget prop with query name
KelvinOm Oct 9, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
32 changes: 31 additions & 1 deletion app/client/src/modules/ui-builder/ui/wds/WDSAIChatWidget/widget/config/defaultConfig.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,5 +1,12 @@
import { ResponsiveBehavior } from "layoutSystems/common/utils/constants";
import type { WidgetDefaultProps } from "WidgetProvider/constants";
import {
BlueprintOperationTypes,
type WidgetDefaultProps,
} from "WidgetProvider/constants";
import { createOrUpdateDataSourceWithAction } from "sagas/DatasourcesSagas";
import { PluginPackageName } from "entities/Action";
import type { ActionData } from "ee/reducers/entityReducers/actionsReducer";
import type { WidgetProps } from "widgets/BaseWidget";
Comment on lines +2 to +9
Copy link
Contributor

@coderabbitai coderabbitai bot Oct 9, 2024

Choose a reason for hiding this comment

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

⚠️ Potential issue

Class, let's discuss the importance of separation of concerns.

I see you've added some new imports to our configuration file. While I appreciate your enthusiasm for adding new functionality, I must point out that importing the createOrUpdateDataSourceWithAction saga function directly into this configuration file is not the best practice. Remember, we want to keep our configuration files clean and declarative.

Consider moving this import to a more appropriate location, such as a saga file or an action handler. This way, we maintain a clear separation between our configuration and our application logic.

Can you think of a better place to put this import? Let's discuss how we can improve this structure to make our code more maintainable and easier to understand.


export const defaultsConfig = {
isVisible: true,
Expand All @@ -9,4 +16,27 @@ export const defaultsConfig = {
responsiveBehavior: ResponsiveBehavior.Fill,
initialAssistantMessage: "",
initialAssistantSuggestions: [],
blueprint: {
operations: [
{
type: BlueprintOperationTypes.ADD_ACTION,
fn: function* (widget: WidgetProps & { children?: WidgetProps[] }) {
Copy link
Contributor

@znamenskii-ilia znamenskii-ilia Oct 10, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

I agree with the rabbit here. Is it possible to not have sagas in configs?

const action: ActionData = yield createOrUpdateDataSourceWithAction(
PluginPackageName.APPSMITH_AI,
{
usecase: { data: "TEXT_CLASSIFY" },
},
);

return [
{
widgetId: widget.widgetId,
propertyName: "query",
propertyValue: action.config.name,
},
];
},
},
],
},
Comment on lines +19 to +41
Copy link
Contributor

@coderabbitai coderabbitai bot Oct 9, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

🛠️ Refactor suggestion

Let's have a lesson on keeping our configuration clean and declarative.

I see you've added a blueprint property with a generator function. While your eagerness to implement new features is commendable, this approach mixes configuration with runtime behavior. As your teacher, I must emphasize the importance of keeping these separate.

Including a generator function with side effects in your configuration file goes against best practices. It makes your code harder to maintain, test, and understand.

Let's improve this together:

  1. Move the generator function to a saga or action handler.
  2. In your configuration, simply reference the action type or saga name.
  3. Handle the actual logic of creating/updating datasources and setting widget properties in the appropriate saga or reducer.

Here's a simple example of how your configuration could look:

blueprint: {
  operations: [
    {
      type: BlueprintOperationTypes.ADD_ACTION,
      actionType: 'CREATE_AI_DATASOURCE_AND_QUERY'
    }
  ]
}

Then, create a separate saga to handle this action type and perform the necessary operations.

Remember, clear separation of concerns leads to more maintainable and understandable code. Keep practicing, and you'll master these concepts in no time!

Copy link
Contributor

@riodeuno riodeuno Oct 10, 2024

Choose a reason for hiding this comment

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

@KelvinOm I agree with @coderabbitai here. I wouldn't use it exactly as @coderabbitai has suggested, but the idea of only passing the new action payload remains. Let me share a detailed change request in a few hours.

Copy link
Contributor

@coderabbitai coderabbitai bot Oct 10, 2024

Choose a reason for hiding this comment

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

@riodeuno, it's wonderful to see that we're on the same page about keeping configurations clean by passing only the necessary action payload. I look forward to your detailed change request to further refine the implementation.

(＾▽＾)

} as unknown as WidgetDefaultProps;
2 changes: 1 addition & 1 deletion app/client/src/sagas/ActionSagas.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -281,7 +281,7 @@ export function* createActionRequestSaga(
actionPayload: ReduxAction<
// TODO: Fix this the next time the file is edited
// eslint-disable-next-line @typescript-eslint/no-explicit-any
Partial<Action> & { eventData: any; pluginId: string }
Partial<Action> & { eventData?: any; pluginId: string }
>,
) {
const payload = { ...actionPayload.payload };
Expand Down
94 changes: 67 additions & 27 deletions app/client/src/sagas/DatasourcesSagas.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -33,7 +33,11 @@ import {
getCurrentBasePageId,
getCurrentPageId,
} from "selectors/editorSelectors";
import type { DatasourceGroupByPluginCategory } from "ee/selectors/entitiesSelector";
import {
type DatasourceGroupByPluginCategory,
getActions,
getDatasourceByPluginId,
} from "ee/selectors/entitiesSelector";
import {
getDatasource,
getDatasourceActionRouteInfo,
Expand Down Expand Up @@ -80,7 +84,6 @@ import type {
import {
AuthenticationStatus,
FilePickerActionStatus,
ToastMessageType,
} from "entities/Datasource";
import {
INTEGRATION_TABS,
Expand All @@ -93,6 +96,8 @@ import {
DATASOURCE_DB_FORM,
DATASOURCE_REST_API_FORM,
} from "ee/constants/forms";
import type { ActionDataState } from "ee/reducers/entityReducers/actionsReducer";
import { createActionRequestSaga } from "./ActionSagas";
import { validateResponse } from "./ErrorSagas";
import AnalyticsUtil from "ee/utils/AnalyticsUtil";
import type { GetFormData } from "selectors/formSelectors";
Expand Down Expand Up @@ -149,12 +154,10 @@ import {
saasEditorDatasourceIdURL,
} from "ee/RouteBuilder";
import {
DATASOURCE_NAME_DEFAULT_PREFIX,
GOOGLE_SHEET_FILE_PICKER_OVERLAY_CLASS,
GOOGLE_SHEET_SPECIFIC_SHEETS_SCOPE,
TEMP_DATASOURCE_ID,
} from "constants/Datasource";
import { getUntitledDatasourceSequence } from "utils/DatasourceSagaUtils";
import { toast } from "@appsmith/ads";
import { fetchPluginFormConfig } from "actions/pluginActions";
import { addClassToDocumentRoot } from "pages/utils";
Expand All @@ -175,7 +178,10 @@ import { getCurrentGitBranch } from "selectors/gitSyncSelectors";
import FocusRetention from "./FocusRetentionSaga";
import { identifyEntityFromPath } from "../navigation/FocusEntity";
import { MAX_DATASOURCE_SUGGESTIONS } from "constants/DatasourceEditorConstants";
import { getFromServerWhenNoPrefetchedResult } from "./helper";
import {
getFromServerWhenNoPrefetchedResult,
getInitialDatasourcePayload,
} from "./helper";
import { executeGoogleApi } from "./loadGoogleApi";
import type { ActionParentEntityTypeInterface } from "ee/entities/Engine/actionHelpers";
import { getCurrentModuleId } from "ee/selectors/modulesSelector";
Expand Down Expand Up @@ -1061,9 +1067,6 @@ function* createTempDatasourceFromFormSaga(
);
const initialValues: unknown = yield call(getConfigInitialValues, formConfig);

const dsList: Datasource[] = yield select(getDatasources);
const sequence = getUntitledDatasourceSequence(dsList);

let datasourceType = actionPayload?.payload?.type;

if (!actionPayload?.payload.type) {
Expand All @@ -1076,25 +1079,11 @@ function* createTempDatasourceFromFormSaga(
}

const defaultEnvId = getDefaultEnvId();
const initialPayload: Datasource = yield getInitialDatasourcePayload(
actionPayload.payload.pluginId,
datasourceType,
);

const initialPayload = {
id: TEMP_DATASOURCE_ID,
name: DATASOURCE_NAME_DEFAULT_PREFIX + sequence,
type: datasourceType,
pluginId: actionPayload.payload.pluginId,
new: false,
datasourceStorages: {
[defaultEnvId]: {
datasourceId: TEMP_DATASOURCE_ID,
environmentId: defaultEnvId,
isValid: false,
datasourceConfiguration: {
properties: [],
},
toastMessage: ToastMessageType.EMPTY_TOAST_MESSAGE,
},
},
};
const payload = merge(initialPayload, actionPayload.payload);

payload.datasourceStorages[defaultEnvId] = merge(
Expand Down Expand Up @@ -1128,7 +1117,58 @@ function* createTempDatasourceFromFormSaga(
);
}

function* createDatasourceFromFormSaga(
export function* createOrUpdateDataSourceWithAction(
pluginPackageName: PluginPackageName,
formData: Record<string, unknown>,
) {
const plugin: Plugin = yield select(
getPluginByPackageName,
pluginPackageName,
);
Comment on lines +1124 to +1127
Copy link
Contributor

@coderabbitai coderabbitai bot Oct 11, 2024

Choose a reason for hiding this comment

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

⚠️ Potential issue

Ensure the plugin exists before proceeding

When retrieving the plugin with getPluginByPackageName, it's important to verify that the plugin is found. If plugin is undefined, it could lead to runtime errors in subsequent calls.

Consider adding a check to handle this scenario, such as:

const plugin: Plugin = yield select(
  getPluginByPackageName,
  pluginPackageName,
);
+ if (!plugin) {
+   throw new Error(`Plugin not found for package name: ${pluginPackageName}`);
+ }
📝 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
const plugin: Plugin = yield select(
getPluginByPackageName,
pluginPackageName,
);
const plugin: Plugin = yield select(
getPluginByPackageName,
pluginPackageName,
);
if (!plugin) {
throw new Error(`Plugin not found for package name: ${pluginPackageName}`);
}

const aiDatasources: Datasource[] = yield select(
getDatasourceByPluginId,
plugin.id,
);
const pageId: string = yield select(getCurrentPageId);
const datasourcePayload: Datasource = yield getInitialDatasourcePayload(
plugin.id,
plugin.type,
);

if (aiDatasources.length === 0) {
yield createDatasourceFromFormSaga({
payload: datasourcePayload,
type: ReduxActionTypes.CREATE_DATASOURCE_FROM_FORM_INIT,
});
}

const updatedAiDatasources: Datasource[] = yield select(
getDatasourceByPluginId,
plugin.id,
);

const actionPayload = {
pageId,
pluginId: updatedAiDatasources[0].pluginId,
datasource: {
id: updatedAiDatasources[0].id,
},
actionConfiguration: {
formData,
},
};

yield createActionRequestSaga({
payload: actionPayload,
type: ReduxActionTypes.CREATE_ACTION_REQUEST,
});

const actions: ActionDataState = yield select(getActions);

return actions[actions.length - 1];
}
Comment on lines +1166 to +1169
Copy link
Contributor

@coderabbitai coderabbitai bot Oct 11, 2024

Choose a reason for hiding this comment

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

⚠️ Potential issue

Ensure the actions array is not empty before accessing

When returning actions[actions.length - 1], it's prudent to check that the actions array has elements. Accessing an empty array can lead to runtime errors.

Consider adding a check to handle this case:

+ if (actions.length === 0) {
+   // Handle the empty array appropriately
+   throw new Error("No actions available.");
+ }
  return actions[actions.length - 1];
📝 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
const actions: ActionDataState = yield select(getActions);
return actions[actions.length - 1];
}
const actions: ActionDataState = yield select(getActions);
if (actions.length === 0) {
// Handle the empty array appropriately
throw new Error("No actions available.");
}
return actions[actions.length - 1];
}


export function* createDatasourceFromFormSaga(
actionPayload: ReduxActionWithCallbacks<Datasource, unknown, unknown>,
) {
try {
Expand Down
23 changes: 20 additions & 3 deletions app/client/src/sagas/WidgetBlueprintSagas.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -60,7 +60,9 @@ export interface UpdatePropertyArgs {
// eslint-disable-next-line @typescript-eslint/no-explicit-any
propertyValue: any;
}
export type BlueprintOperationAddActionFn = () => void;
export type BlueprintOperationAddActionFn = (
widget: WidgetProps & { children?: WidgetProps[] },
) => Generator;
export type BlueprintOperationModifyPropsFn = (
widget: WidgetProps & { children?: WidgetProps[] },
widgets: { [widgetId: string]: FlattenedWidgetProps },
Expand Down Expand Up @@ -110,12 +112,27 @@ export function* executeWidgetBlueprintOperations(
) {
const layoutSystemType: LayoutSystemTypes = yield select(getLayoutSystemType);

operations.forEach((operation: BlueprintOperation) => {
for (const operation of operations) {
const widget: WidgetProps & { children?: string[] | WidgetProps[] } = {
...widgets[widgetId],
};

switch (operation.type) {
case BlueprintOperationTypes.ADD_ACTION:
if (operation.fn) {
const updatePropertyPayloads: UpdatePropertyArgs[] | undefined =
yield (operation.fn as BlueprintOperationAddActionFn)(
widget as WidgetProps & { children?: WidgetProps[] },
);

updatePropertyPayloads &&
updatePropertyPayloads.forEach((params: UpdatePropertyArgs) => {
widgets[params.widgetId][params.propertyName] =
params.propertyValue;
});
}

break;
Comment on lines +115 to +135
Copy link
Contributor

@coderabbitai coderabbitai bot Oct 11, 2024

Choose a reason for hiding this comment

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

🛠️ Refactor suggestion

Well done on updating the executeWidgetBlueprintOperations function, students!

Your changes have greatly improved the function's capability to handle asynchronous operations. Let's review the key points:

  1. The switch to a for...of loop is commendable. It allows us to use yield within the loop, which is crucial for Redux Saga's asynchronous flow.

  2. The new case for BlueprintOperationTypes.ADD_ACTION is a valuable addition. It enables dynamic action addition during operation execution.

However, there's room for a small improvement. Let's make our code even more robust:

Consider using optional chaining and nullish coalescing for safer property access:

case BlueprintOperationTypes.ADD_ACTION:
  if (operation.fn) {
    const updatePropertyPayloads: UpdatePropertyArgs[] | undefined =
      yield (operation.fn as BlueprintOperationAddActionFn)(
        widget as WidgetProps & { children?: WidgetProps[] },
      );

    updatePropertyPayloads?.forEach((params: UpdatePropertyArgs) => {
      widgets[params.widgetId][params.propertyName] = params.propertyValue;
    });
  }
  break;

This change will prevent potential errors if updatePropertyPayloads is undefined.

Keep up the excellent work, and remember: in programming, as in life, it's always better to be safe than sorry!

🧰 Tools
🪛 Biome

[error] 128-132: Change to an optional chain.

Unsafe fix: Change to an optional chain.

(lint/complexity/useOptionalChain)

case BlueprintOperationTypes.MODIFY_PROPS:
if (widget.children && widget.children.length > 0) {
widget.children = (widget.children as string[]).map(
Expand All @@ -139,7 +156,7 @@ export function* executeWidgetBlueprintOperations(
});
break;
}
});
}

const result: { [widgetId: string]: FlattenedWidgetProps } = yield widgets;

Expand Down
38 changes: 38 additions & 0 deletions app/client/src/sagas/helper.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -4,6 +4,7 @@ import type {
FormEvalOutput,
ConditionalOutput,
} from "reducers/evaluationReducers/formEvaluationReducer";
import { select } from "redux-saga/effects";
import AppsmithConsole from "utils/AppsmithConsole";
import LOG_TYPE from "entities/AppsmithConsole/logtype";
import type { Log } from "entities/AppsmithConsole";
Expand All @@ -22,6 +23,14 @@ import { isPlainObject, isString } from "lodash";
import { DATA_BIND_REGEX_GLOBAL } from "constants/BindingsConstants";
import { apiFailureResponseInterceptor } from "api/interceptors";
import { klonaLiteWithTelemetry } from "utils/helpers";
import { getDefaultEnvId } from "ee/api/ApiUtils";
import { getDatasources } from "ee/selectors/entitiesSelector";
import {
DATASOURCE_NAME_DEFAULT_PREFIX,
TEMP_DATASOURCE_ID,
} from "../constants/Datasource";
import { type Datasource, ToastMessageType } from "../entities/Datasource";
import { getUntitledDatasourceSequence } from "../utils/DatasourceSagaUtils";

// function to extract all objects that have dynamic values
export const extractFetchDynamicValueFormConfigs = (
Expand Down Expand Up @@ -249,3 +258,32 @@ export function* getFromServerWhenNoPrefetchedResult(

return yield apiEffect();
}

export function* getInitialDatasourcePayload(
pluginId: string,
pluginType?: string,
) {
const dsList: Datasource[] = yield select(getDatasources);
const sequence = getUntitledDatasourceSequence(dsList);
const defaultEnvId = getDefaultEnvId();

return {
id: TEMP_DATASOURCE_ID,
name: DATASOURCE_NAME_DEFAULT_PREFIX + sequence,
type: pluginType,
pluginId: pluginId,
new: false,
datasourceStorages: {
[defaultEnvId]: {
datasourceId: TEMP_DATASOURCE_ID,
environmentId: defaultEnvId,
isValid: false,
datasourceConfiguration: {
url: "",
properties: [],
},
toastMessage: ToastMessageType.EMPTY_TOAST_MESSAGE,
},
},
};
}
Comment on lines +262 to +289
Copy link
Contributor

@coderabbitai coderabbitai bot Oct 9, 2024

Choose a reason for hiding this comment

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

⚠️ Potential issue

Ensure 'pluginType' is defined to prevent potential undefined property

In the getInitialDatasourcePayload function, you've declared pluginType as an optional parameter. However, when assigning pluginType to the type property in your returned object (line 273), if pluginType is undefined, the type property will also be undefined. This could lead to unexpected behavior in your application. To ensure reliability, it's important that type always has a valid value.

Suggestions:

  1. Provide a default value for pluginType:

    You can assign a default value to pluginType in the function parameters. This way, if pluginType is not provided, it will default to a specified value.

    export function* getInitialDatasourcePayload(
  pluginId: string,
-  pluginType?: string,
+  pluginType: string = "DEFAULT_PLUGIN_TYPE",
) {

  2. Handle the undefined case when assigning to type:

    Alternatively, you can modify the assignment to ensure that type falls back to a default if pluginType is undefined.

      return {
    id: TEMP_DATASOURCE_ID,
    name: DATASOURCE_NAME_DEFAULT_PREFIX + sequence,
-   type: pluginType,
+   type: pluginType ?? "DEFAULT_PLUGIN_TYPE",
    pluginId: pluginId,

    This uses the nullish coalescing operator (??) to assign "DEFAULT_PLUGIN_TYPE" if pluginType is null or undefined.

Committable suggestion was skipped due to low confidence.