chore: Create query selector #37018
chore: Create query selector #37018
Conversation
github-actions
bot
added
the
skip-changelog
WalkthroughThe changes in this pull request primarily involve modifications to the Changes
Possibly related PRs
Suggested labels
Poem
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 2, 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 (3)
app/client/src/components/propertyControls/OneClickBindingControl.tsx (1)
75-77: Maintain consistent prop formatting.While the functionality is correct, consider maintaining consistent prop formatting. The
allowedDatasourceTypesprop uses multiple lines whilegetQueryBindingValueis on a single line.- allowedDatasourceTypes={ - this.props.controlConfig?.allowedDatasourceTypes - } + allowedDatasourceTypes={this.props.controlConfig?.allowedDatasourceTypes}Also applies to: 87-87
app/client/src/components/editorComponents/WidgetQueryGeneratorForm/CommonControls/DatasourceDropdown/useSource/useOtherOptions.tsx (1)
38-40: LGTM! Consider enhancing the comment.The early return pattern is clean and efficient.
Consider making the comment more specific:
-// we don't want to show the options for connect to query variant +// Skip additional options when in query-only mode to restrict functionalityapp/client/src/components/editorComponents/WidgetQueryGeneratorForm/index.tsx (1)
116-117: Consider adding JSDoc documentation for the new props.While the types are well-defined, adding JSDoc documentation would improve maintainability by explaining the purpose and expected values of these props.
+ /** Allowed types of datasources that can be selected */ allowedDatasourceTypes?: WidgetQueryGeneratorFormContextType["allowedDatasourceTypes"]; + /** Function to extract binding value from query data */ getQueryBindingValue?: WidgetQueryGeneratorFormContextType["getQueryBindingValue"];
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
📒 Files selected for processing (7)
- app/client/src/actions/datasourceActions.ts (1 hunks)
- app/client/src/components/editorComponents/WidgetQueryGeneratorForm/CommonControls/DatasourceDropdown/types.ts (1 hunks)
- app/client/src/components/editorComponents/WidgetQueryGeneratorForm/CommonControls/DatasourceDropdown/useSource/useConnectToOptions.tsx (6 hunks)
- app/client/src/components/editorComponents/WidgetQueryGeneratorForm/CommonControls/DatasourceDropdown/useSource/useDatasourceOptions.tsx (2 hunks)
- app/client/src/components/editorComponents/WidgetQueryGeneratorForm/CommonControls/DatasourceDropdown/useSource/useOtherOptions.tsx (1 hunks)
- app/client/src/components/editorComponents/WidgetQueryGeneratorForm/index.tsx (6 hunks)
- app/client/src/components/propertyControls/OneClickBindingControl.tsx (4 hunks)
🧰 Additional context used
🔇 Additional comments (11)
app/client/src/components/editorComponents/WidgetQueryGeneratorForm/CommonControls/DatasourceDropdown/types.ts (1)
4-4: LGTM! Verify switch statements.The new enum variant follows consistent naming patterns and aligns with the PR objective.
Let's verify that all switch statements handling this enum are updated:
✅ Verification successful
The previous command might have failed because we need to first understand how the enum is used. Let's search for its definition and usages to verify the impact of the new variant.
The new enum variant is properly handled in all usages
The search results show that the new
CONNECT_TO_QUERYvariant is correctly handled in the codebase:
- In
useOtherOptions.tsx: Special case handling to return empty options- In
useDatasourceOptions.tsx: Early return with empty arrays- No switch statements found that would require updates
- All components using the enum are importing it correctly
The implementation is consistent with the intended behavior of restricting options for the chat widget.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Search for switch statements using DROPDOWN_VARIANT ast-grep --pattern 'switch ($expr) { $$$ }' | rg -A 5 'DROPDOWN_VARIANT'Length of output: 72
Script:
#!/bin/bash # Let's try a different approach to find usages of the enum # First, find the enum name rg -l "enum.*DROPDOWN_VARIANT" # Then search for all usages of DROPDOWN_VARIANT rg "DROPDOWN_VARIANT" -A 5 # Also search specifically for the new enum value rg "CONNECT_TO_QUERY"Length of output: 15061
app/client/src/components/propertyControls/OneClickBindingControl.tsx (2)
1-3: LGTM! Clean type import.The type import is well-structured and follows TypeScript best practices.
116-117: LGTM! Well-structured type definitions.The type definitions are properly inherited from WidgetQueryGeneratorFormContextType and correctly marked as optional.
app/client/src/components/editorComponents/WidgetQueryGeneratorForm/index.tsx (1)
20-21: LGTM! Clean type definitions and interface changes.The exported interface with new optional properties provides good type safety for query-specific functionality.
Also applies to: 23-23, 57-58
app/client/src/components/editorComponents/WidgetQueryGeneratorForm/CommonControls/DatasourceDropdown/useSource/useConnectToOptions.tsx (4)
7-7: LGTM: Required imports added for new functionality.The additions of
getPluginsandisDynamicValueimports support the new filtering and dynamic value detection features.Also applies to: 29-29
213-218: LGTM: Dynamic value handling added.The addition of dynamic value detection improves the binding logic by properly handling dynamic expressions.
164-164: Verify context type definitions.The new context variables are being used correctly, but let's ensure they're properly typed in the context definition.
#!/bin/bash # Search for WidgetQueryGeneratorFormContext type definition ast-grep --pattern 'interface WidgetQueryGeneratorFormContextType { $$$ allowedDatasourceTypes?: $_ $$$ }'Also applies to: 166-166
192-204: Verify plugin filtering logic completeness.The datasource type filtering looks correct but might need additional error handling.
Let's verify the plugin types being used:
✅ Verification successful
Let me gather more information about the plugin filtering logic to verify its completeness.
This will help us understand the Plugin interface structure and how plugins are handled in the codebase.
Let me also check for any existing error handling patterns:
Let me try again with corrected file type flags:
Let me try one final search without file type restrictions:
Plugin filtering logic is complete and safe
The current implementation is correct and includes proper error handling:
- The
allowedDatasourceTypescheck is optional (wrapped in an if statement)- The plugin filtering has a null-safe implementation (wrapped in if(plugin))
- The filtering maintains the original queries if no matching plugin is found
- The code follows the established pattern seen across the codebase for plugin filtering
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Search for plugin type definitions and usage rg -A 2 "allowedDatasourceTypes" --type typescriptLength of output: 4258
Script:
#!/bin/bash # Search for plugin type definitions and related error handling ast-grep --pattern 'interface Plugin { $$$ name: $_ id: $_ $$$ }' # Check how plugins are used in the context rg -B 2 -A 2 "plugins\.find\(" --type typescriptLength of output: 161
Script:
#!/bin/bash # Look for error handling patterns around plugin filtering rg -B 2 -A 2 "filteredQueries = filteredQueries\.filter" --type typescriptLength of output: 111
Script:
#!/bin/bash # Search for plugin type definitions and related error handling ast-grep --pattern 'interface Plugin { $$$ name: $_ id: $_ $$$ }' # Check how plugins are used in the context rg -B 2 -A 2 'plugins\.find\(' --type ts --type tsx # Look for error handling patterns around plugin filtering rg -B 2 -A 2 'filteredQueries = filteredQueries\.filter' --type ts --type tsx # Check for any null checks or error handling around plugin usage rg -A 2 'if \(!plugin\)' --type ts --type tsxLength of output: 341
Script:
#!/bin/bash # Search for plugin usage patterns rg -B 2 -A 2 'plugins\.find\(' # Look for error handling patterns rg -A 2 'if \(!plugin\)' # Check the entire file content for context fd -g "*useConnectToOptions.tsx" -x cat {}Length of output: 25089
app/client/src/components/editorComponents/WidgetQueryGeneratorForm/CommonControls/DatasourceDropdown/useSource/useDatasourceOptions.tsx (3)
37-37: LGTM!Clean import addition that aligns with the component's needs.
61-65: Verify impact on dependent components.The early return for CONNECT_TO_QUERY variant effectively disables datasource selection. While this aligns with the PR objectives, we should verify that dependent components handle empty options gracefully.
45-46: Verify context provider implementation.The context usage looks good, but let's verify the provider implementation.
✅ Verification successful
Let me gather more information about the context implementation to verify if
datasourceDropdownVariantis properly included.
Let me try a different approach to find the context type and implementation.
Let me check one final thing to verify if
datasourceDropdownVariantis properly defined in the context type.
Let me try one last search with a different pattern to find the context value definition.
Context implementation is properly defined
The
datasourceDropdownVariantproperty is correctly defined in theWidgetQueryGeneratorFormContextTypeinterface (line 54) as typeDROPDOWN_VARIANT, making the context usage in the reviewed code valid and type-safe.🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Search for the WidgetQueryGeneratorFormContext provider implementation ast-grep --pattern 'const WidgetQueryGeneratorFormContext = React.createContext<$_>($_)'Length of output: 553
Script:
#!/bin/bash # Check the context type definition and default value rg -A 10 "type WidgetQueryGeneratorFormContextType" app/client/src/components/editorComponents/WidgetQueryGeneratorForm/index.tsx # Also check where the context value is provided ast-grep --pattern 'value={$_}' app/client/src/components/editorComponents/WidgetQueryGeneratorForm/index.tsxLength of output: 243
Script:
#!/bin/bash # Search for type definition and context value with more context rg -B 2 -A 15 "WidgetQueryGeneratorFormContext" app/client/src/components/editorComponents/WidgetQueryGeneratorForm/index.tsx # Also check for the interface or type definition rg "interface.*WidgetQueryGeneratorFormContextType" app/client/src/components/editorComponents/WidgetQueryGeneratorForm/index.tsxLength of output: 2467
Script:
#!/bin/bash # Get the complete context type definition with more lines rg -A 20 "interface WidgetQueryGeneratorFormContextType" app/client/src/components/editorComponents/WidgetQueryGeneratorForm/index.tsx # Also check where contextValue is defined rg -B 5 "const contextValue = {" app/client/src/components/editorComponents/WidgetQueryGeneratorForm/index.tsxLength of output: 936
Script:
#!/bin/bash # Search for contextValue assignment with proper escaping rg -B 5 "contextValue =" app/client/src/components/editorComponents/WidgetQueryGeneratorForm/index.tsx # Also get the complete interface definition ast-grep --pattern 'interface WidgetQueryGeneratorFormContextType { $$$ }'Length of output: 4445
...nts/WidgetQueryGeneratorForm/CommonControls/DatasourceDropdown/useSource/useOtherOptions.tsx
Show resolved
Hide resolved
app/client/src/components/editorComponents/WidgetQueryGeneratorForm/index.tsx
Show resolved
Hide resolved
...WidgetQueryGeneratorForm/CommonControls/DatasourceDropdown/useSource/useConnectToOptions.tsx
Outdated
Show resolved
Hide resolved
| export const createTempDatasourceFromForm = (payload: { | ||
| pluginId: string; | ||
| type: PluginType; | ||
| }) => { |
There was a problem hiding this comment.
💡 Codebase verification
Type change needs to be reverted - saga expects broader type
The saga implementation expects CreateDatasourceConfig | Datasource type but the action creator now accepts a narrower type with just pluginId and type. This mismatch will cause runtime issues as the saga merges additional properties from the payload.
Key issues:
- The saga uses
actionPayload.payloadproperties beyond justpluginIdandtype - The payload is merged with
initialPayloadat line 1098, expecting additional configuration - All callers are passing only the restricted payload which may lead to incomplete datasource creation
🔗 Analysis chain
Type-safe payload structure improvement.
The new payload structure with explicit pluginId and type properties improves type safety and better reflects the function's purpose in creating temporary datasources for queries.
Let's verify the usage of this function to ensure all callers have been updated:
Let me gather more information about the saga implementation to understand how the new payload type is handled.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for createTempDatasourceFromForm usage
rg "createTempDatasourceFromForm" -A 2
Length of output: 6009
Script:
#!/bin/bash
# Check the saga implementation to see how it handles the new payload type
ast-grep --pattern 'function* createTempDatasourceFromFormSaga($$$) {
$$$
}'
Length of output: 5849
| @@ -77,17 +79,19 @@ export function sortQueries( | |||
| export function getBindingValue( | |||
| widget: WidgetProps, | |||
| query: ActionData | ModuleInstanceData, | |||
| getQueryBindingValue: (query: ActionData | ModuleInstanceData) => string = ( | |||
There was a problem hiding this comment.
For chat widget, we don't want ${query.config.name}.data, we just want ${query.config.name}. So updated this function so that binding can be controlled by widget.
|
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: 1
🧹 Outside diff range and nitpick comments (1)
app/client/src/components/editorComponents/WidgetQueryGeneratorForm/CommonControls/DatasourceDropdown/useSource/types.ts (1)
4-6: Consider adding JSDoc for better documentation.The type definition is clean and follows best practices. Consider adding JSDoc to document the purpose and expected return format.
+/** + * Function type for generating binding value string from query data + * @param query - The query data from either an action or module instance + * @returns A string representation of the binding value + */ export type GetQueryBindingValue = ( query: ActionData | ModuleInstanceData, ) => string;
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
📒 Files selected for processing (3)
- app/client/src/components/editorComponents/WidgetQueryGeneratorForm/CommonControls/DatasourceDropdown/useSource/types.ts (1 hunks)
- app/client/src/components/editorComponents/WidgetQueryGeneratorForm/CommonControls/DatasourceDropdown/useSource/useConnectToOptions.tsx (6 hunks)
- app/client/src/components/editorComponents/WidgetQueryGeneratorForm/index.tsx (6 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- app/client/src/components/editorComponents/WidgetQueryGeneratorForm/index.tsx
🧰 Additional context used
🔇 Additional comments (4)
app/client/src/components/editorComponents/WidgetQueryGeneratorForm/CommonControls/DatasourceDropdown/useSource/types.ts (1)
1-2: LGTM! Clean imports using type imports.app/client/src/components/editorComponents/WidgetQueryGeneratorForm/CommonControls/DatasourceDropdown/useSource/useConnectToOptions.tsx (3)
7-7: LGTM: Clean import additionsThe new imports for plugins and dynamic value utilities are well-organized and necessary for the enhanced functionality.
Also applies to: 29-30
83-84: Previous review comment about null checks is still applicableAlso applies to: 88-88, 94-94
215-218: LGTM: Proper dynamic value handlingThe addition of dynamic value checking enhances the binding logic while maintaining existing functionality.
| // filter query based on allowedDatasourceType | ||
| if (allowedDatasourceTypes) { | ||
| const plugin = plugins.find((plugin) => | ||
| allowedDatasourceTypes.includes(plugin.name), | ||
| ); | ||
|
|
||
| if (plugin) { | ||
| filteredQueries = filteredQueries.filter( | ||
| (query) => query.config.pluginId === plugin.id, | ||
| ); | ||
| } | ||
| } | ||
|
|
There was a problem hiding this comment.
Enhance allowedDatasourceTypes filtering logic
The current implementation might not handle multiple allowed datasource types correctly as it only checks for the first matching plugin.
Consider this more robust implementation:
- const plugin = plugins.find((plugin) =>
- allowedDatasourceTypes.includes(plugin.name),
- );
-
- if (plugin) {
- filteredQueries = filteredQueries.filter(
- (query) => query.config.pluginId === plugin.id,
- );
- }
+ const allowedPluginIds = plugins
+ .filter((plugin) => allowedDatasourceTypes.includes(plugin.name))
+ .map((plugin) => plugin.id);
+
+ if (allowedPluginIds.length > 0) {
+ filteredQueries = filteredQueries.filter((query) =>
+ allowedPluginIds.includes(query.config.pluginId),
+ );
+ }📝 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.
| // filter query based on allowedDatasourceType | |
| if (allowedDatasourceTypes) { | |
| const plugin = plugins.find((plugin) => | |
| allowedDatasourceTypes.includes(plugin.name), | |
| ); | |
| if (plugin) { | |
| filteredQueries = filteredQueries.filter( | |
| (query) => query.config.pluginId === plugin.id, | |
| ); | |
| } | |
| } | |
| // filter query based on allowedDatasourceType | |
| if (allowedDatasourceTypes) { | |
| const allowedPluginIds = plugins | |
| .filter((plugin) => allowedDatasourceTypes.includes(plugin.name)) | |
| .map((plugin) => plugin.id); | |
| if (allowedPluginIds.length > 0) { | |
| filteredQueries = filteredQueries.filter((query) => | |
| allowedPluginIds.includes(query.config.pluginId), | |
| ); | |
| } | |
| } |
|
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. |
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Outside diff range and nitpick comments (3)
app/client/src/components/editorComponents/WidgetQueryGeneratorForm/index.tsx (1)
57-58: Consider adding JSDoc comments for the new interface properties.The new properties would benefit from documentation explaining their purpose and expected values.
+ /** Array of allowed datasource types to filter available options */ allowedDatasourceTypes?: string[]; + /** Function to generate binding value from query data */ getQueryBindingValue?: (query: ActionData | ModuleInstanceData) => string;app/client/src/components/editorComponents/WidgetQueryGeneratorForm/CommonControls/DatasourceDropdown/useSource/useConnectToOptions.tsx (2)
215-218: Add type safety for the value parameter.Consider adding type annotation for better type safety:
- onSelect: function (value: string, valueOption?: DropdownOptionType) { + onSelect: function (value: string | undefined, valueOption?: DropdownOptionType) { - const isDynamic = isDynamicValue(value); + const isDynamic = value ? isDynamicValue(value) : false;
164-164: Add type validation for allowedDatasourceTypes.Consider adding runtime type validation for the allowedDatasourceTypes context value to prevent potential runtime errors.
if (allowedDatasourceTypes && !Array.isArray(allowedDatasourceTypes)) { console.warn('allowedDatasourceTypes should be an array'); return filteredQueries; }Also applies to: 166-166
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
📒 Files selected for processing (3)
- app/client/packages/dsl/src/index.ts (1 hunks)
- app/client/src/components/editorComponents/WidgetQueryGeneratorForm/CommonControls/DatasourceDropdown/useSource/useConnectToOptions.tsx (6 hunks)
- app/client/src/components/editorComponents/WidgetQueryGeneratorForm/index.tsx (6 hunks)
🧰 Additional context used
🔇 Additional comments (6)
app/client/packages/dsl/src/index.ts (1)
14-14: LGTM! Verify function export.The new export follows the established pattern and is appropriately grouped with other migrate-related exports.
✅ Verification successful
The previous ast-grep pattern didn't yield results, so I'm trying a different approach:
- First, locating the exact utils file path
- Then searching for isDynamicValue with context to verify its existence and export status
This will help us confirm whether the function exists and is properly exported.
Export verified and correctly implemented
The
isDynamicValuefunction exists inapp/client/packages/dsl/src/migrate/utils.tsand is properly exported as a const arrow function. It's actively used across multiple migration files for checking dynamic bindings, confirming its utility and proper implementation.🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Verify the isDynamicValue function exists in the utils file ast-grep --pattern 'export function isDynamicValue($_) { $$$ }'Length of output: 65
Script:
#!/bin/bash # Let's try a different approach to locate the isDynamicValue function # First, let's find the utils file in the migrate directory fd utils.ts -p app/client/packages/dsl/src/migrate # Then let's use ripgrep to search for isDynamicValue with context rg "isDynamicValue" -A 3 app/client/packages/dsl/src/migrateLength of output: 8198
app/client/src/components/editorComponents/WidgetQueryGeneratorForm/index.tsx (4)
20-21: LGTM: Clean type imports and interface export.The new imports and interface export changes look good. Making the interface public is appropriate since it's needed by external components.
Also applies to: 23-23
116-117: LGTM: Props interface properly extends context types.Good practice using the context type definitions for the props to maintain type consistency.
257-258: Previous optimization comment still applies.The existing review comment about optimizing useMemo dependencies is still valid. The new properties should also be considered for optimization.
Also applies to: 278-280
129-129: Verify the impact of optional props.The new optional props might affect the component's behavior when undefined. Consider adding default values or handling undefined cases explicitly.
Also applies to: 134-134
app/client/src/components/editorComponents/WidgetQueryGeneratorForm/CommonControls/DatasourceDropdown/useSource/useConnectToOptions.tsx (1)
29-29: LGTM: Clean function signature update with proper defaults.The addition of the
getQueryBindingValueparameter provides good flexibility while maintaining backward compatibility.Also applies to: 82-84
| @@ -32,9 +32,10 @@ export const createDatasourceFromForm = ( | |||
| }; | |||
| }; | |||
|
|
|||
| export const createTempDatasourceFromForm = ( | |||
There was a problem hiding this comment.
Types are a little weird in data source code. The code just uses plugin id and type in the function, yet we pass Datasource | CreateDatasourceConfig here. I think this is done just to satisfy the typescript.
|
@jsartisan What happens if the Appsmith AI Datasource doesn't exist? Or if the user want's to use their own datasource? |
You will see an empty dropdown in that case. That state is not properly handled in UI as of now. Btw, as soon as you drop the chat widget, we create a new Appsmith AI query ( @KelvinOm do we also create datasource if it does not exists? )
This case won't happen. the AIChat widget explicitly require AppsmithAI datasoruce cause that datasources is handling lot of stuff under the hood. So, it can't be binded with other datasources like MySQL, postgres. The way the widget tells that it only needs AppsmithAI is by specifying in the propertyPaneConfig like this: {
propertyName: "chatQuery",
helpText: "The query to be used for the chat",
label: "Chat Query",
controlType: "ONE_CLICK_BINDING_CONTROL",
controlConfig: {
showEditFieldsModal: false,
datasourceDropdownVariant: DROPDOWN_VARIANT.CONNECT_TO_QUERY
excludePrimaryColumnFromQueryGeneration: true,
isConnectableToWidget: false,
allowedDatasourceTypes: [PluginName.APPSMITH_AI], // <-------------------
getQueryBindingValue: (query: ActionData | ModuleInstanceData) => {
return query.config.name;
},
},
} |
@jsartisan Yes, if datasource is not exists, then we create it, if there is one, we add query to it. |
jsartisan
force-pushed
the
chore/create-query-selector-control
branch
from
58b3bae
to
d6e322c
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
📒 Files selected for processing (8)
- app/client/packages/dsl/src/index.ts (1 hunks)
- app/client/src/actions/datasourceActions.ts (1 hunks)
- app/client/src/components/editorComponents/WidgetQueryGeneratorForm/CommonControls/DatasourceDropdown/types.ts (1 hunks)
- app/client/src/components/editorComponents/WidgetQueryGeneratorForm/CommonControls/DatasourceDropdown/useSource/useConnectToOptions.tsx (6 hunks)
- app/client/src/components/editorComponents/WidgetQueryGeneratorForm/CommonControls/DatasourceDropdown/useSource/useDatasourceOptions.tsx (2 hunks)
- app/client/src/components/editorComponents/WidgetQueryGeneratorForm/CommonControls/DatasourceDropdown/useSource/useOtherOptions.tsx (1 hunks)
- app/client/src/components/editorComponents/WidgetQueryGeneratorForm/index.tsx (6 hunks)
- app/client/src/components/propertyControls/OneClickBindingControl.tsx (4 hunks)
🚧 Files skipped from review as they are similar to previous changes (7)
- app/client/packages/dsl/src/index.ts
- app/client/src/actions/datasourceActions.ts
- app/client/src/components/editorComponents/WidgetQueryGeneratorForm/CommonControls/DatasourceDropdown/types.ts
- app/client/src/components/editorComponents/WidgetQueryGeneratorForm/CommonControls/DatasourceDropdown/useSource/useConnectToOptions.tsx
- app/client/src/components/editorComponents/WidgetQueryGeneratorForm/CommonControls/DatasourceDropdown/useSource/useDatasourceOptions.tsx
- app/client/src/components/editorComponents/WidgetQueryGeneratorForm/index.tsx
- app/client/src/components/propertyControls/OneClickBindingControl.tsx
🔇 Additional comments (2)
app/client/src/components/editorComponents/WidgetQueryGeneratorForm/CommonControls/DatasourceDropdown/useSource/useOtherOptions.tsx (2)
38-40: LGTM! Early return pattern improves readability.The early return for CONNECT_TO_QUERY variant aligns with the PR's objective to restrict functionality for the chat widget.
57-68: Previous review comment about consolidating analytics events is still applicable.
| const options = []; | ||
|
|
||
| const entryPoint = DatasourceCreateEntryPoints.ONE_CLICK_BINDING; | ||
| options.push({ | ||
| icon: <Icon name="plus" size="md" />, | ||
| id: "Connect new datasource", | ||
| label: "Connect new datasource", | ||
| value: "Connect new datasource", | ||
| onSelect: () => { | ||
| history.push( | ||
| integrationEditorURL({ | ||
| basePageId, | ||
| selectedTab: INTEGRATION_TABS.NEW, | ||
| }), | ||
| ); | ||
|
|
||
| AnalyticsUtil.logEvent("NAVIGATE_TO_CREATE_NEW_DATASOURCE_PAGE", { | ||
| entryPoint, | ||
| }); | ||
| }, | ||
| AnalyticsUtil.logEvent("BIND_OTHER_ACTIONS", { | ||
| widgetName: widget.widgetName, | ||
| widgetType: widget.type, | ||
| propertyName: propertyName, | ||
| selectedAction: "Connect new datasource", | ||
| }); | ||
|
|
||
| const entryPoint = DatasourceCreateEntryPoints.ONE_CLICK_BINDING; | ||
|
|
||
| AnalyticsUtil.logEvent("NAVIGATE_TO_CREATE_NEW_DATASOURCE_PAGE", { | ||
| entryPoint, | ||
| }); | ||
| }, | ||
| ]; | ||
| }); |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Consider extracting the connect datasource option to a separate function.
The options array initialization and the connect datasource option configuration could be extracted to improve readability and maintainability.
+ const getConnectDatasourceOption = (basePageId: string) => ({
+ icon: <Icon name="plus" size="md" />,
+ id: "Connect new datasource",
+ label: "Connect new datasource",
+ value: "Connect new datasource",
+ onSelect: () => {
+ history.push(
+ integrationEditorURL({
+ basePageId,
+ selectedTab: INTEGRATION_TABS.NEW,
+ }),
+ );
+ // Analytics events here
+ },
+ });
const otherOptions = useMemo(() => {
if (datasourceDropdownVariant === DROPDOWN_VARIANT.CONNECT_TO_QUERY)
return [];
const options = [];
- options.push({
- icon: <Icon name="plus" size="md" />,
- // ... rest of the configuration
- });
+ options.push(getConnectDatasourceOption(basePageId));Committable suggestion was skipped due to low confidence.
/ok-to-test tags="@tag.All"
This PR updates the WidgetQueryGeneratorForm to only allow queries so that it looks like a query selector. This is needed for chat widget where the widget can only bind Appsmith AI queries.
PS: The above image is an example where TableWidget had allowedDatasourcestypes as "Mysql" ( just for testing )
Summary by CodeRabbit
Summary by CodeRabbit
New Features
Improvements
Bug Fixes
Tip
🟢 🟢 🟢 All cypress tests have passed! 🎉 🎉 🎉
Workflow run: https://github.com/appsmithorg/appsmith/actions/runs/11493055126
Commit: d6e322c
Cypress dashboard.
Tags:
@tag.AllSpec:
Thu, 24 Oct 2024 06:53:37 UTC