feat: creation of datasource and query for ai chat #36785
Conversation
WalkthroughThe changes in this pull request involve updates to several files related to widget operations and data source management. Key modifications include the addition of new imports, the introduction of new functions, and enhancements to existing sagas for improved handling of data sources and widget operations. Notable updates include the addition of a 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
|
|
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. |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 4
🧹 Outside diff range and nitpick comments (2)
app/client/src/sagas/WidgetBlueprintSagas.ts (1)
119-123: Consider Adding a Default Case to Handle Unexpected Operation TypesWhile you've covered the known operation types, it's a good practice to include a
defaultcase in yourswitchstatement. This helps in catching any unexpected operation types and aids in debugging.Here's a suggestion:
case BlueprintOperationTypes.ADD_ACTION: if (operation.fn) { yield (operation.fn as BlueprintOperationAddActionFn)(); } break; case BlueprintOperationTypes.MODIFY_PROPS: // existing code... break; + default: + console.warn(`Unhandled operation type: ${operation.type}`); + break;app/client/src/sagas/DatasourcesSagas.ts (1)
1149-1160: Dispatch Action with Valid Datasource InformationWhen dispatching
createActionRequest, ensure thatupdatedAiDatasources[0]contains valid and up-to-date information. This prevents potential issues when the action is processed.Additionally, consider adding null checks or validations before accessing
updatedAiDatasources[0]to enhance code reliability.
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
📒 Files selected for processing (4)
- app/client/src/modules/ui-builder/ui/wds/WDSAIChatWidget/widget/config/defaultConfig.ts (1 hunks)
- app/client/src/sagas/DatasourcesSagas.ts (6 hunks)
- app/client/src/sagas/WidgetBlueprintSagas.ts (3 hunks)
- app/client/src/sagas/helper.ts (3 hunks)
🧰 Additional context used
🔇 Additional comments (12)
app/client/src/sagas/WidgetBlueprintSagas.ts (5)
113-115: Excellent Use of 'for...of' Loop for Asynchronous OperationsBy switching from a
forEachloop to afor...ofloop, you've allowed the use ofyieldwithin the iteration. This change is crucial for handling asynchronous generator functions in Redux Saga. Well done on making this adjustment to enhance the saga's capability to manage side effects effectively.
119-123: Proper Implementation of the 'ADD_ACTION' CaseAdding the
ADD_ACTIONcase to yourswitchstatement enables dynamic addition of actions during operation execution. Yielding the operation function ensures that any asynchronous processes are properly integrated into the saga flow. This is a thoughtful enhancement to the operation handling.
148-148: Good Practice Returning the Updated WidgetsReturning the updated
widgetsobject ensures that any modifications made within the operations are correctly propagated. This is essential for maintaining the integrity of the widget state throughout the saga execution.
148-148: Ensure Consistency in Return StatementsDouble-check that all possible execution paths in
executeWidgetBlueprintOperationsreturn the updatedwidgetsobject. This consistency helps prevent unexpected undefined returns, which could cause issues downstream.You can verify this by reviewing the function's return statements and ensuring they all return
resultorwidgetsas appropriate.
63-63:⚠️ Potential issueEnsure All Implementations Return a Generator
Changing the return type of
BlueprintOperationAddActionFnto() => Generatoris an important update. It's essential to verify that all functions implementing this type now correctly return aGenerator. This ensures that asynchronous operations within our sagas are handled properly.To assist you, let's check all implementations:
app/client/src/sagas/DatasourcesSagas.ts (7)
2-2: Utilize Type-SafeobjectKeysfor Better Type InferenceBy importing
objectKeysfrom@appsmith/utils, you're enhancing the type safety when iterating over object keys. This ensures that the compiler can infer the correct types, reducing potential runtime errors.
37-40: Import Relevant Types and Selectors for ClarityIncluding
DatasourceGroupByPluginCategoryandgetDatasourceByPluginIdfrom"ee/selectors/entitiesSelector"helps in maintaining code clarity and modularity. It makes the subsequent code utilizing these imports more readable and maintainable.
179-182: Import Helper Functions for ReusabilityBy importing
getFromServerWhenNoPrefetchedResultandgetInitialDatasourcePayloadfrom"./helper", you're promoting code reusability. This aligns with the DRY (Don't Repeat Yourself) principle.
Line range hint
600-606: Ensure Consistent Type Usage withobjectKeysReplacing
Object.keyswithobjectKeyswhen iterating overdatasourcePayload.datasourceStoragesensures that the keys are correctly typed. This enhances type safety and prevents potential type-related issues during iteration.
1081-1084: Proper Initialization of Datasource PayloadUsing
getInitialDatasourcePayloadto initialize theinitialPayloadfor the datasource is a good approach. It ensures that all necessary default values are set properly before merging withactionPayload.
1137-1142: Verify Successful Creation of DatasourceEnsure that the call to
createDatasourceFromFormSagasuccessfully creates a datasource. It's crucial to handle any errors that might occur during this process to maintain the robustness of your application.To confirm the datasource creation, you might want to:
- Check the response of
createDatasourceFromFormSaga.- Implement error handling in case the saga fails.
- Log relevant information for debugging purposes.
187-187: Import Necessary Action CreatorsImporting
createActionRequestfrom"../actions/pluginActionActions"is essential for dispatching actions related to plugin operations. This aligns with the code's architecture by using centralized action creators.
app/client/src/modules/ui-builder/ui/wds/WDSAIChatWidget/widget/config/defaultConfig.ts
Show resolved
Hide resolved
| blueprint: { | ||
| operations: [ | ||
| { | ||
| type: BlueprintOperationTypes.ADD_ACTION, | ||
| fn: function* () { | ||
| yield createOrUpdateDataSourceWithAction( | ||
| PluginPackageName.APPSMITH_AI, | ||
| { | ||
| usecase: { data: "TEXT_CLASSIFY" }, | ||
| }, | ||
| ); | ||
| }, | ||
| }, | ||
| ], | ||
| }, |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Keep configuration declarative without embedding executable functions
Including a generator function within the blueprint property introduces side-effect logic into your configuration. It's important to keep configurations simple and declarative to enhance maintainability and readability.
I suggest moving the side-effectful generator function to a saga or action handler, and referencing it appropriately from your configuration.
| 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, | ||
| }, | ||
| }, | ||
| }; | ||
| } |
There was a problem hiding this comment.
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:
-
Provide a default value for
pluginType:You can assign a default value to
pluginTypein the function parameters. This way, ifpluginTypeis not provided, it will default to a specified value.export function* getInitialDatasourcePayload( pluginId: string, - pluginType?: string, + pluginType: string = "DEFAULT_PLUGIN_TYPE", ) {
-
Handle the undefined case when assigning to
type:Alternatively, you can modify the assignment to ensure that
typefalls back to a default ifpluginTypeis 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"ifpluginTypeisnullorundefined.
Committable suggestion was skipped due to low confidence.
| export function* createOrUpdateDataSourceWithAction( | ||
| pluginPackageName: PluginPackageName, | ||
| formData: Record<string, unknown>, | ||
| ) { | ||
| const plugin: Plugin = yield select( | ||
| getPluginByPackageName, | ||
| pluginPackageName, | ||
| ); | ||
| const aiDatasources: Datasource[] = yield select( | ||
| getDatasourceByPluginId, | ||
| plugin.id, | ||
| ); | ||
| const pageId: string = yield select(getCurrentPageId); | ||
| const payload: Datasource = yield getInitialDatasourcePayload( | ||
| plugin.id, | ||
| plugin.type, | ||
| ); | ||
|
|
||
| if (aiDatasources.length === 0) { | ||
| yield createDatasourceFromFormSaga({ | ||
| payload, | ||
| type: ReduxActionTypes.CREATE_DATASOURCE_FROM_FORM_INIT, | ||
| }); | ||
| } | ||
|
|
||
| const updatedAiDatasources: Datasource[] = yield select( | ||
| getDatasourceByPluginId, | ||
| plugin.id, | ||
| ); | ||
|
|
||
| yield put( | ||
| createActionRequest({ | ||
| pageId, | ||
| pluginId: updatedAiDatasources[0].pluginId, | ||
| datasource: { | ||
| id: updatedAiDatasources[0].id, | ||
| }, | ||
| actionConfiguration: { | ||
| formData, | ||
| }, | ||
| }), | ||
| ); | ||
| } | ||
|
|
There was a problem hiding this comment.
Handle Empty Datasource Array After Creation
In the createOrUpdateDataSourceWithAction function, after creating a new datasource, you're selecting updatedAiDatasources[0] without verifying if the array is not empty. It's important to check that the datasource exists before accessing it to prevent runtime errors.
Consider adding a check to handle cases where the datasource may not have been created successfully:
const updatedAiDatasources: Datasource[] = yield select(
getDatasourceByPluginId,
plugin.id,
);
+ if (updatedAiDatasources.length === 0) {
+ // Handle the error appropriately, perhaps by throwing an exception or retrying the operation
+ throw new Error("Failed to retrieve the AI datasource after creation.");
+ }
yield put(
createActionRequest({
pageId,
pluginId: updatedAiDatasources[0].pluginId,
datasource: {
id: updatedAiDatasources[0].id,
},
actionConfiguration: {
formData,
},
}),
);📝 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 function* createOrUpdateDataSourceWithAction( | |
| pluginPackageName: PluginPackageName, | |
| formData: Record<string, unknown>, | |
| ) { | |
| const plugin: Plugin = yield select( | |
| getPluginByPackageName, | |
| pluginPackageName, | |
| ); | |
| const aiDatasources: Datasource[] = yield select( | |
| getDatasourceByPluginId, | |
| plugin.id, | |
| ); | |
| const pageId: string = yield select(getCurrentPageId); | |
| const payload: Datasource = yield getInitialDatasourcePayload( | |
| plugin.id, | |
| plugin.type, | |
| ); | |
| if (aiDatasources.length === 0) { | |
| yield createDatasourceFromFormSaga({ | |
| payload, | |
| type: ReduxActionTypes.CREATE_DATASOURCE_FROM_FORM_INIT, | |
| }); | |
| } | |
| const updatedAiDatasources: Datasource[] = yield select( | |
| getDatasourceByPluginId, | |
| plugin.id, | |
| ); | |
| yield put( | |
| createActionRequest({ | |
| pageId, | |
| pluginId: updatedAiDatasources[0].pluginId, | |
| datasource: { | |
| id: updatedAiDatasources[0].id, | |
| }, | |
| actionConfiguration: { | |
| formData, | |
| }, | |
| }), | |
| ); | |
| } | |
| export function* createOrUpdateDataSourceWithAction( | |
| pluginPackageName: PluginPackageName, | |
| formData: Record<string, unknown>, | |
| ) { | |
| const plugin: Plugin = yield select( | |
| getPluginByPackageName, | |
| pluginPackageName, | |
| ); | |
| const aiDatasources: Datasource[] = yield select( | |
| getDatasourceByPluginId, | |
| plugin.id, | |
| ); | |
| const pageId: string = yield select(getCurrentPageId); | |
| const payload: Datasource = yield getInitialDatasourcePayload( | |
| plugin.id, | |
| plugin.type, | |
| ); | |
| if (aiDatasources.length === 0) { | |
| yield createDatasourceFromFormSaga({ | |
| payload, | |
| type: ReduxActionTypes.CREATE_DATASOURCE_FROM_FORM_INIT, | |
| }); | |
| } | |
| const updatedAiDatasources: Datasource[] = yield select( | |
| getDatasourceByPluginId, | |
| plugin.id, | |
| ); | |
| if (updatedAiDatasources.length === 0) { | |
| // Handle the error appropriately, perhaps by throwing an exception or retrying the operation | |
| throw new Error("Failed to retrieve the AI datasource after creation."); | |
| } | |
| yield put( | |
| createActionRequest({ | |
| pageId, | |
| pluginId: updatedAiDatasources[0].pluginId, | |
| datasource: { | |
| id: updatedAiDatasources[0].id, | |
| }, | |
| actionConfiguration: { | |
| formData, | |
| }, | |
| }), | |
| ); | |
| } |
KelvinOm
force-pushed
the
feat/ai-chat-query
branch
from
215b48a to
6722730
Compare
|
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. |
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Outside diff range and nitpick comments (2)
app/client/src/sagas/WidgetBlueprintSagas.ts (1)
115-135: Well done on updating the executeWidgetBlueprintOperations function, students!Your changes show a good understanding of generator functions and type handling. Let's review the improvements:
Changing from
forEachto afor...ofloop is like upgrading from a regular whiteboard marker to a smart pen. It allows us to useyieldwithin the loop, which is crucial for our generator function.Adding the new case for
BlueprintOperationTypes.ADD_ACTIONis like adding a new chapter to our textbook. It aligns perfectly with the changes we made to the BlueprintOperationAddActionFn type.However, there's room for a small improvement in our error handling. Consider using optional chaining to make our code more robust:
yield (operation.fn as BlueprintOperationAddActionFn)?.( widget as WidgetProps & { children?: WidgetProps[] }, );This change is like adding a safety net to our code. It ensures that if
operation.fnis undefined, our code won't throw an error.Would you like me to provide a more detailed explanation or help implement this change?
🧰 Tools
🪛 Biome
[error] 128-132: Change to an optional chain.
Unsafe fix: Change to an optional chain.
(lint/complexity/useOptionalChain)
app/client/src/sagas/DatasourcesSagas.ts (1)
1121-1170: Well done on adding this new function, but let's make it even better!Class, I'm pleased to see this new function
createOrUpdateDataSourceWithAction. It's doing a good job of handling the creation or update of datasources for AI plugins. However, we can improve it a bit:
- The function is quite long. Consider breaking it down into smaller, more focused functions for better readability and maintainability.
- Add some error handling. What happens if the datasource creation fails? Or if the action creation fails?
- Consider adding some comments to explain the purpose of each major step in the function.
Remember, clear and concise code is like a well-organized classroom - it makes everyone's life easier!
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
📒 Files selected for processing (5)
- app/client/src/modules/ui-builder/ui/wds/WDSAIChatWidget/widget/config/defaultConfig.ts (1 hunks)
- app/client/src/sagas/ActionSagas.ts (3 hunks)
- app/client/src/sagas/DatasourcesSagas.ts (7 hunks)
- app/client/src/sagas/WidgetBlueprintSagas.ts (3 hunks)
- app/client/src/sagas/helper.ts (3 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- app/client/src/sagas/helper.ts
🧰 Additional context used
🪛 Biome
app/client/src/sagas/WidgetBlueprintSagas.ts
[error] 128-132: Change to an optional chain.
Unsafe fix: Change to an optional chain.
(lint/complexity/useOptionalChain)
🔇 Additional comments (8)
app/client/src/sagas/WidgetBlueprintSagas.ts (3)
63-65: Excellent update to the BlueprintOperationAddActionFn type, class!I'm pleased to see this improvement in our code. Let's break down why this change is beneficial:
The return type is now
Generatorinstead ofvoid. This allows our function to yield results during execution, making it more powerful for handling asynchronous operations. It's like upgrading from a simple chalkboard to an interactive smartboard!The function now accepts a parameter of type
WidgetPropswith an optionalchildrenproperty. This provides more context to the function, allowing for more specific and tailored operations. It's similar to how having more information about a student helps us create a more personalized learning plan.Keep up the good work! These changes will make our code more flexible and easier to work with in the future.
128-132: Excellent work on handling updatePropertyPayloads, class!Your code shows good attention to detail and safe programming practices. Let's review why this is commendable:
The null check before iteration (
updatePropertyPayloads &&) is like checking if all students are present before starting a group activity. It prevents errors ifupdatePropertyPayloadsis undefined or null.Using
forEachto update the widgets object is an appropriate choice here. It's like efficiently distributing worksheets to each student without needing to collect anything back.The code correctly updates the
widgetsobject with new property values. This is similar to updating each student's record in our class register.Keep up this level of careful coding! It helps prevent errors and makes our codebase more robust.
🧰 Tools
🪛 Biome
[error] 128-132: Change to an optional chain.
Unsafe fix: Change to an optional chain.
(lint/complexity/useOptionalChain)
Line range hint
1-324: Excellent job on enhancing our WidgetBlueprintSagas, class!Let's review the overall changes and their impact on our system:
The modifications to our blueprint system are like upgrading our entire classroom with new technology. We've improved the flexibility and power of our widget operations, allowing for more dynamic and responsive user interfaces.
The changes to function signatures and control flow are akin to refining our teaching methods. They allow for more precise control over widget behavior and better handling of asynchronous operations.
The new case handling for ADD_ACTION is like adding a new subject to our curriculum. It expands the capabilities of our blueprint system, allowing for more diverse widget configurations.
Throughout these changes, you've maintained the overall structure and purpose of the file. This is like renovating a classroom while keeping its fundamental layout intact - it improves functionality without disorienting the users.
These enhancements will significantly improve our ability to create and manage complex widget blueprints. Well done on maintaining architectural integrity while introducing powerful new features!
🧰 Tools
🪛 Biome
[error] 128-132: Change to an optional chain.
Unsafe fix: Change to an optional chain.
(lint/complexity/useOptionalChain)
app/client/src/sagas/DatasourcesSagas.ts (2)
1172-1172: Good job on making this function exportable!Class, pay attention to this small but important change. By adding the
exportkeyword to thecreateDatasourceFromFormSagafunction, we've made it available for use in other parts of our codebase. This is a good practice for modular design.Remember, sharing is caring in programming too!
Line range hint
1-1172: Overall, good work on this file, but there's always room for improvement!Class, let's recap what we've learned from this code review:
- We've added a new function
createOrUpdateDataSourceWithActionwhich handles AI-related datasources. This is a good addition to our functionality.- We've made
createDatasourceFromFormSagaexportable, improving our module structure.These changes are positive steps, but remember:
- Always strive for clear, concise, and well-documented code.
- Consider breaking down large functions into smaller, more manageable pieces.
- Don't forget to add proper error handling to make our code more robust.
Keep up the good work, and remember: in programming, as in life, continuous improvement is the key to success!
app/client/src/sagas/ActionSagas.ts (3)
1-1: Excellent Work Importing 'objectKeys'Well done on importing
objectKeysfrom@appsmith/utilsat line 1. Utilizing specialized utility functions enhances code reliability and readability.
285-285: Ensure Proper Handling of Optional 'eventData'By making
eventDataoptional in thecreateActionRequestSagafunction at line 285, please verify that all usages ofeventDatawithin the function account for it potentially beingundefined. This will help prevent any runtime errors.
992-992: Good Choice Replacing 'Object.keys' with 'objectKeys'At line 992, replacing
Object.keyswithobjectKeysprovides better type safety and avoids issues related to prototype pollution. This is a commendable improvement.
KelvinOm
force-pushed
the
feat/ai-chat-query
branch
from
6722730 to
e3cf293
Compare
|
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. |
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Outside diff range and nitpick comments (3)
app/client/src/sagas/WidgetBlueprintSagas.ts (2)
115-135: Well done on enhancing our blueprint operations, students! Let's refine it further.Your use of a
for...ofloop is commendable, as it allows us to handle asynchronous operations more effectively. The new case forADD_ACTIONis a valuable addition, enabling dynamic action creation.However, let's consider a few improvements to make our code even better:
- Error handling: We should add try-catch blocks to handle potential errors during operation execution.
- Type safety: Let's use type guards to ensure we're calling the correct function type.
- Null checks: We should add null checks before accessing properties or calling functions.
Here's a suggested implementation:
case BlueprintOperationTypes.ADD_ACTION: if (operation.fn && typeof operation.fn === "function") { try { const updatePropertyPayloads: UpdatePropertyArgs[] | undefined = yield* (operation.fn as BlueprintOperationAddActionFn)( widget as WidgetProps & { children?: WidgetProps[] } ); if (updatePropertyPayloads) { for (const params of updatePropertyPayloads) { if (widgets[params.widgetId]) { widgets[params.widgetId][params.propertyName] = params.propertyValue; } } } } catch (error) { console.error("Error in ADD_ACTION operation:", error); // Consider adding appropriate error handling here } } break;Remember, class, robust error handling and type safety are crucial for maintaining a stable application!
Would you like assistance in implementing these improvements?
🧰 Tools
🪛 Biome
[error] 128-132: Change to an optional chain.
Unsafe fix: Change to an optional chain.
(lint/complexity/useOptionalChain)
128-132: Let's consider a small improvement in our code structure, class!The static analysis tool has made an interesting suggestion about using an optional chain. While this is generally a good practice, we need to be cautious due to our use of generator functions.
Here's a potential way to incorporate optional chaining while respecting our generator context:
yield* (function* () { if (updatePropertyPayloads?.length) { for (const params of updatePropertyPayloads) { if (widgets[params.widgetId]) { widgets[params.widgetId][params.propertyName] = params.propertyValue; } } } })();This approach maintains the generator context while introducing optional chaining and a more concise loop structure. Remember, students, it's important to balance modern JavaScript features with the specific requirements of our codebase!
What do you think about this suggestion? Would you like to implement it, or do you prefer the current structure?
🧰 Tools
🪛 Biome
[error] 128-132: Change to an optional chain.
Unsafe fix: Change to an optional chain.
(lint/complexity/useOptionalChain)
app/client/src/sagas/DatasourcesSagas.ts (1)
1120-1169: Well done, class! Let's review this new function.Good job on implementing the
createOrUpdateDataSourceWithActionfunction. It's like a well-organized lesson plan, covering all the necessary steps to create or update a datasource and its associated action. However, just as we always strive for improvement in our lessons, I have a small suggestion:Consider adding error handling to make this function more robust. What if the plugin retrieval fails or the action creation encounters an issue? Remember, in our classroom of code, we always want to be prepared for unexpected situations!
Here's a homework assignment for you:
export function* createOrUpdateDataSourceWithAction( pluginPackageName: PluginPackageName, formData: Record<string, unknown>, ) { + try { const plugin: Plugin = yield select( getPluginByPackageName, pluginPackageName, ); + if (!plugin) { + throw new Error(`Plugin not found for package name: ${pluginPackageName}`); + } // ... rest of the function ... + } catch (error) { + console.error("Error in createOrUpdateDataSourceWithAction:", error); + // Consider how you want to handle or propagate this error + } }This way, we're teaching our code to handle unexpected situations gracefully. Keep up the good work!
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
📒 Files selected for processing (4)
- app/client/src/modules/ui-builder/ui/wds/WDSAIChatWidget/widget/config/defaultConfig.ts (1 hunks)
- app/client/src/sagas/ActionSagas.ts (1 hunks)
- app/client/src/sagas/DatasourcesSagas.ts (5 hunks)
- app/client/src/sagas/WidgetBlueprintSagas.ts (3 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- app/client/src/sagas/ActionSagas.ts
🧰 Additional context used
🪛 Biome
app/client/src/sagas/WidgetBlueprintSagas.ts
[error] 128-132: Change to an optional chain.
Unsafe fix: Change to an optional chain.
(lint/complexity/useOptionalChain)
🔇 Additional comments (1)
app/client/src/sagas/WidgetBlueprintSagas.ts (1)
63-65: Excellent update to theBlueprintOperationAddActionFntype signature, class!This change is a step in the right direction. By updating the function to accept a widget parameter and return a Generator, we're opening up new possibilities for asynchronous operations and dynamic action creation based on widget properties. This is a valuable improvement that will enhance our blueprint system's flexibility.
Remember, class, in programming, adaptability is key!
| 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"; |
There was a problem hiding this comment.
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.
| blueprint: { | ||
| operations: [ | ||
| { | ||
| type: BlueprintOperationTypes.ADD_ACTION, | ||
| fn: function* (widget: WidgetProps & { children?: WidgetProps[] }) { | ||
| const action: ActionData = yield createOrUpdateDataSourceWithAction( | ||
| PluginPackageName.APPSMITH_AI, | ||
| { | ||
| usecase: { data: "TEXT_CLASSIFY" }, | ||
| }, | ||
| ); | ||
|
|
||
| return [ | ||
| { | ||
| widgetId: widget.widgetId, | ||
| propertyName: "query", | ||
| propertyValue: action.config.name, | ||
| }, | ||
| ]; | ||
| }, | ||
| }, | ||
| ], | ||
| }, |
There was a problem hiding this comment.
🛠️ 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:
- Move the generator function to a saga or action handler.
- In your configuration, simply reference the action type or saga name.
- 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!
There was a problem hiding this comment.
@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.
There was a problem hiding this comment.
@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.
(＾▽＾)
| operations: [ | ||
| { | ||
| type: BlueprintOperationTypes.ADD_ACTION, | ||
| fn: function* (widget: WidgetProps & { children?: WidgetProps[] }) { |
There was a problem hiding this comment.
I agree with the rabbit here. Is it possible to not have sagas in configs?
KelvinOm
force-pushed
the
feat/ai-chat-query
branch
from
e3cf293 to
3066160
Compare
|
/build-deploy-preview skip-tests=true |
|
Deploying Your Preview: https://github.com/appsmithorg/appsmith/actions/runs/11290293915. |
|
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. |
There was a problem hiding this comment.
Actionable comments posted: 3
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
📒 Files selected for processing (5)
- app/client/src/modules/ui-builder/ui/wds/WDSAIChatWidget/widget/config/defaultConfig.ts (2 hunks)
- app/client/src/sagas/ActionSagas.ts (1 hunks)
- app/client/src/sagas/DatasourcesSagas.ts (5 hunks)
- app/client/src/sagas/WidgetBlueprintSagas.ts (3 hunks)
- app/client/src/sagas/helper.ts (3 hunks)
🚧 Files skipped from review as they are similar to previous changes (2)
- app/client/src/sagas/ActionSagas.ts
- app/client/src/sagas/helper.ts
🧰 Additional context used
🪛 Biome
app/client/src/sagas/WidgetBlueprintSagas.ts
[error] 128-132: Change to an optional chain.
Unsafe fix: Change to an optional chain.
(lint/complexity/useOptionalChain)
🔇 Additional comments (9)
app/client/src/modules/ui-builder/ui/wds/WDSAIChatWidget/widget/config/defaultConfig.ts (2)
2-9:⚠️ Potential issueClass, let's discuss our import statements.
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
createOrUpdateDataSourceWithActionsaga 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.
19-41:⚠️ Potential issueLet's have a lesson on keeping our configuration clean and declarative.
I see you've added a
blueprintproperty 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:
- Move the generator function to a saga or action handler.
- In your configuration, simply reference the action type or saga name.
- 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!
app/client/src/sagas/WidgetBlueprintSagas.ts (2)
63-65: Excellent update to theBlueprintOperationAddActionFntype signature, class!This change is a step in the right direction. By updating the function to accept a widget parameter and return a Generator, we're enabling more sophisticated and asynchronous operations. This will allow for better handling of complex widget interactions.
Let's break down the benefits:
- The new parameter provides context about the widget being operated on.
- Returning a Generator allows for yielding asynchronous operations, which is crucial in a Redux Saga environment.
Keep up the good work! This change will make our blueprint operations more flexible and powerful.
159-159: Class, let's recap our lesson on improving the WidgetBlueprintSagas!Today, we've seen some excellent improvements to our codebase:
- We've enhanced the
BlueprintOperationAddActionFntype to be more flexible and powerful.- We've updated the
executeWidgetBlueprintOperationsfunction to better handle asynchronous operations.These changes will make our widget blueprint system more robust and easier to work with. They demonstrate a good understanding of Redux Saga and TypeScript's type system.
Remember, continuous improvement is key in software development. Always look for ways to make your code more efficient, readable, and maintainable.
Great job, everyone! Keep up this level of quality in your future work.
app/client/src/sagas/DatasourcesSagas.ts (5)
36-40: Well-structured import statementGood job importing
DatasourceGroupByPluginCategory,getActions, andgetDatasourceByPluginId. This enhances modularity and keeps the code organized.
99-100: Proper addition of necessary importsIncluding
ActionDataStateandcreateActionRequestSagaensures that the sagas have access to the required types and functions.
181-184: Effective use of helper functionsImporting
getFromServerWhenNoPrefetchedResultandgetInitialDatasourcePayloadpromotes code reuse and improves maintainability.
1082-1085: Utilizing helper function for initial payloadExcellent choice to use
getInitialDatasourcePayloadto initialize the payload. This makes the code cleaner and reduces duplication.
1145-1159: Remember to check for empty datasource array before accessingIt's essential to verify that
updatedAiDatasourcesis not empty before accessingupdatedAiDatasources[0]. This will prevent potential runtime errors if the array is empty.
| 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; |
There was a problem hiding this comment.
🛠️ 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:
-
The switch to a
for...ofloop is commendable. It allows us to useyieldwithin the loop, which is crucial for Redux Saga's asynchronous flow. -
The new case for
BlueprintOperationTypes.ADD_ACTIONis 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)
| const actions: ActionDataState = yield select(getActions); | ||
|
|
||
| return actions[actions.length - 1]; | ||
| } |
There was a problem hiding this comment.
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.
| 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]; | |
| } |
| const plugin: Plugin = yield select( | ||
| getPluginByPackageName, | ||
| pluginPackageName, | ||
| ); |
There was a problem hiding this comment.
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.
| 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}`); | |
| } |
|
Deploy-Preview-URL: https://ce-36785.dp.appsmith.com |
|
Closed in favour of https://github.com/appsmithorg/appsmith-ee/pull/5368 |
Description
After dropping chat widget, we create AI datasource If there is no suitable one or use already exist. After we simply create a quary for it.
2024-10-08.13.55.31.mov
Automation
/ok-to-test tags="@tag.All"
🔍 Cypress test results
Caution
🔴 🔴 🔴 Some tests have failed.
Workflow run: https://github.com/appsmithorg/appsmith/actions/runs/11290285036
Commit: 3066160
Cypress dashboard.
Tags: @tag.All
Spec:
The following are new failures, please fix them before merging the PR:
- cypress/e2e/Regression/ClientSide/EmbedSettings/EmbedSettings_spec.js
- cypress/e2e/Regression/ClientSide/FormLogin/EnableFormLogin_spec.js
- cypress/e2e/Regression/ClientSide/OtherUIFeatures/Analytics_spec.js
List of identified flaky tests.Fri, 11 Oct 2024 11:05:07 UTC
Communication
Should the DevRel and Marketing teams inform users about this change?
Summary by CodeRabbit
New Features
blueprintproperty in the configuration.Bug Fixes
Refactor
Documentation