fix: Updating the logic for datasource selector to fox the client build failure on prod #38107
Conversation
WalkthroughThe changes introduced in this pull request enhance the Changes
Possibly related PRs
Suggested labels
Suggested reviewers
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
Documentation and Community
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Outside diff range and nitpick comments (3)
app/client/src/PluginActionEditor/components/PluginActionResponse/components/DatasourceTab/DatasourceSelector/index.tsx (1)
22-28: Consider adding null check for plugin.uiComponentWhile the null check for
pluginis good, accessinguiComponentshould also be null-checked to prevent potential runtime errors.- return props.plugin ? ( - API_FORM_COMPONENTS.includes(props.plugin.uiComponent) ? ( + return props.plugin?.uiComponent ? ( + API_FORM_COMPONENTS.includes(props.plugin.uiComponent) ? (app/client/src/PluginActionEditor/components/PluginActionResponse/components/DatasourceTab/DatasourceInfo.tsx (1)
13-13: Consider enhancing analytics with plugin informationThe plugin prop is now available but not utilized in the analytics event. Consider adding the plugin name to the analytics event for better tracking.
AnalyticsUtil.logEvent("EDIT_DATASOURCE_CLICK", { datasourceId: datasourceId, - pluginName: "", + pluginName: plugin?.name || "", entryPoint: entryPoint, });Also applies to: 19-19, 25-25, 56-56
app/client/src/PluginActionEditor/components/PluginActionResponse/components/DatasourceTab/Datasource.tsx (1)
49-50: Clean implementation of plugin integrationThe changes follow a good architectural pattern by:
- Fetching plugin data at the container level
- Passing it down consistently to child components
- Maintaining separation of concerns
This approach should effectively resolve the client build failure while maintaining code quality.
Also applies to: 149-149, 177-177
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
📒 Files selected for processing (4)
app/client/src/PluginActionEditor/components/PluginActionResponse/components/DatasourceTab/Datasource.tsx(4 hunks)app/client/src/PluginActionEditor/components/PluginActionResponse/components/DatasourceTab/DatasourceInfo.tsx(2 hunks)app/client/src/PluginActionEditor/components/PluginActionResponse/components/DatasourceTab/DatasourceSelector/index.tsx(2 hunks)app/client/src/PluginActionEditor/components/PluginActionResponse/components/DatasourceTab/DatasourceTables.tsx(3 hunks)
🔇 Additional comments (6)
app/client/src/PluginActionEditor/components/PluginActionResponse/components/DatasourceTab/DatasourceSelector/index.tsx (1)
2-2: LGTM: Type imports and interface updates are correct
The Plugin type import and interface update properly maintain type safety.
Also applies to: 18-18
app/client/src/PluginActionEditor/components/PluginActionResponse/components/DatasourceTab/DatasourceTables.tsx (2)
12-12: LGTM: Clean implementation of plugin prop flow
The changes correctly implement the plugin prop flow through the component hierarchy while maintaining type safety.
Also applies to: 19-19, 29-29, 62-62
Line range hint 1-1: Verify plugin prop usage across the codebase
Let's ensure all usages of these components have been updated to provide the plugin prop.
app/client/src/PluginActionEditor/components/PluginActionResponse/components/DatasourceTab/Datasource.tsx (3)
28-28: LGTM: Import follows existing pattern
The new import aligns with other entity selector imports from the same module.
149-149: Verify plugin prop types in child components
The plugin prop is consistently passed to child components, but we should verify the prop types are properly defined.
Let's check the prop types in the child components:
Also applies to: 177-177
✅ Verification successful
Let me search for the component definitions and prop types with a different approach using ripgrep. This should help us verify the plugin prop types in the child components.
The previous searches revealed the Plugin type is used throughout the codebase, but we need to specifically check the component files to verify the prop types in the child components that are receiving the plugin prop.
I can see that both DatasourceInfo and DatasourceTables components have proper type definitions for the plugin prop as Plugin | undefined, but we should verify the DatasourceSelector component as well since it's receiving the plugin prop from DatasourceInfo.
Plugin prop types are properly defined in all components
All components receiving the plugin prop have proper type definitions:
- DatasourceTables:
plugin?: Plugin - DatasourceInfo:
plugin?: Plugin - DatasourceSelector:
plugin?: Plugin
The Plugin type is correctly imported from 'api/PluginApi' in all components.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for DatasourceInfo and DatasourceTables component definitions
ast-grep --pattern 'interface Props {
$$$
plugin: $$$
$$$
}'
Length of output: 68
Script:
#!/bin/bash
# Search for DatasourceInfo component definition and its props
rg -A 5 "DatasourceInfo.*Props"
# Search for DatasourceTables component definition and its props
rg -A 5 "DatasourceTables.*Props"
# Search for plugin prop type definitions in these components
rg -B 2 -A 2 "plugin:.*Plugin"
Length of output: 23722
Script:
#!/bin/bash
# Let's check the specific component files for DatasourceInfo and DatasourceTables
fd -e tsx "DatasourceInfo|DatasourceTables" -x cat {}
Length of output: 4619
Script:
#!/bin/bash
# Let's check the DatasourceSelector component since it also receives the plugin prop
fd -e tsx "DatasourceSelector" -x cat {}
Length of output: 8403
49-50: Consider adding error handling for undefined plugin
While the empty string fallback prevents runtime errors, it might mask potential issues. Consider adding explicit error handling or validation.
Let's verify the plugin selector usage pattern across the codebase:
Consider this safer implementation:
- const plugin = useSelector((state) => getPlugin(state, pluginId || ""));
+ const plugin = useSelector((state) => {
+ if (!pluginId) return undefined;
+ return getPlugin(state, pluginId);
+ });
Description
Updating the logic for datasource selector to fox the client build failure on prod
Fixes #
Issue Numberor
Fixes
Issue URLWarning
If no issue exists, please create an issue first, and check with the maintainers if the issue is valid.
Automation
/ok-to-test tags="@tag.Datasource"
🔍 Cypress test results
Warning
Workflow run: https://github.com/appsmithorg/appsmith/actions/runs/12277105493
Commit: 507ed6f
Cypress dashboard.
Tags: @tag.Datasource
Spec:
It seems like no tests ran 😔. We are not able to recognize it, please check workflow here.
Wed, 11 Dec 2024 13:32:23 UTC
Communication
Should the DevRel and Marketing teams inform users about this change?
Summary by CodeRabbit
New Features
Datasourcecomponent to support plugin data.DatasourceInfo,DatasourceTables, andDatasourceSelectorcomponents now accept apluginprop, improving their functionality and flexibility.Bug Fixes
Documentation
pluginprop.