feat: Schema tab UI update #37420
feat: Schema tab UI update #37420
Conversation
…o action-redesign/schema-tab
…o action-redesign/schema-tab
…o action-redesign/schema-tab
albinAppsmith
requested review from
ApekshaBhosale and
sagar-qa007
as code owners
WalkthroughThe pull request introduces several changes to the schema management in the application. Key modifications include the update of data-testid attributes for better testability, the introduction of new React components for displaying and interacting with datasource schemas, and the removal of an outdated component. Additionally, constants related to schema messages have been added or modified for improved user feedback, and various styling adjustments have been made to enhance the UI. Changes
Assessment against linked issues
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
|
github-actions
bot
added
IDE Navigation
|
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: 7
🧹 Outside diff range and nitpick comments (15)
app/client/src/PluginActionEditor/components/PluginActionResponse/components/Schema/CurrentDataSource.tsx (3)
13-17: Consider renaming interface to follow TypeScript conventionsThe interface name
iPropsshould be renamed toCurrentDataSourcePropsto follow TypeScript naming conventions.-interface iProps { +interface CurrentDataSourceProps { type: "link" | "trigger"; datasourceId: string; datasourceName: string; }
38-38: Improve image accessibilityThe alt text "entityIcon" is generic. Consider using a more descriptive text like
alt="Datasource plugin icon".-<img alt="entityIcon" src={getAssetUrl(datasourceIcon)} /> +<img alt="Datasource plugin icon" src={getAssetUrl(datasourceIcon)} />
31-42: Add data-testid attributes for testingTo align with the testing requirements mentioned in the PR summary, add data-testid attributes to the component.
const content = ( <Flex + data-testid="datasource-content" alignItems="center" gap="spaces-2" justifyContent={type === "link" ? "center" : "start"} > - <EntityIcon height="16px" width="16px"> + <EntityIcon data-testid="datasource-icon" height="16px" width="16px"> <img alt="Datasource plugin icon" src={getAssetUrl(datasourceIcon)} /> </EntityIcon> {datasourceName} </Flex> );app/client/src/PluginActionEditor/components/PluginActionResponse/components/Schema/TableColumns.tsx (3)
32-37: Consider using useMemo for columnsAndKeys array.The array combination operation could be memoized to prevent unnecessary recalculations on re-renders.
- const columnsAndKeys: Array<DatasourceColumns | DatasourceKeys> = []; - - if (selectedTableItems) { - columnsAndKeys.push(...selectedTableItems.keys); - columnsAndKeys.push(...selectedTableItems.columns); - } + const columnsAndKeys = React.useMemo(() => { + if (!selectedTableItems) return []; + return [...selectedTableItems.keys, ...selectedTableItems.columns]; + }, [selectedTableItems]);
39-50: Add ARIA attributes for better accessibility.The scrollable container should have appropriate ARIA attributes for accessibility.
<Flex borderLeft="1px solid var(--ads-v2-color-border)" flex="1" flexDirection="column" height={`100%`} justifyContent={ isLoading || columns.length === 0 ? "center" : "flex-start" } overflowY="scroll" padding="spaces-3" + role="region" + aria-label="Table columns" >
56-64: Improve key generation for list items.Using array index in keys combined with field name could lead to issues if the same field appears multiple times.
- key={`${field.name}${index}`} + key={`${selectedTable}-${field.name}-${field.type}`}app/client/src/pages/Editor/DatasourceInfo/DatasourceField.tsx (1)
39-39: Consider adding flex-wrap for narrow viewportsThe gap property is a good choice for spacing. Consider adding
flex-wrap: wrapto handle narrow viewport scenarios better.const Content = styled.div` margin: 0px 4px; flex-direction: row; min-width: 0; display: flex; gap: var(--ads-v2-spaces-2); + flex-wrap: wrap; `;app/client/src/pages/Editor/DatasourceInfo/DatasourceStructureNotFound.tsx (1)
Line range hint
45-71: Consider performance optimization and code simplificationThe editDatasource function could benefit from some improvements:
- The eslint warning about new function creation could be addressed by memoizing the function
- The optional chaining syntax could be simplified
Consider applying these changes:
+ const editDatasource = React.useCallback(() => { let entryPoint = DatasourceEditEntryPoints.QUERY_EDITOR_DATASOURCE_SCHEMA; if (props.context === DatasourceStructureContext.DATASOURCE_VIEW_MODE) { entryPoint = DatasourceEditEntryPoints.DATASOURCE_VIEW_MODE_EDIT; } AnalyticsUtil.logEvent("EDIT_DATASOURCE_CLICK", { datasourceId: datasourceId, pluginName: pluginName, entryPoint: entryPoint, }); if (props.context === DatasourceStructureContext.DATASOURCE_VIEW_MODE) { - props?.customEditDatasourceFn && props?.customEditDatasourceFn(); + props.customEditDatasourceFn?.(); return; } const url = datasourcesEditorIdURL({ basePageId, datasourceId: datasourceId, params: { ...omit(getQueryParams(), "viewMode"), viewMode: false }, generateEditorPath: true, }); history.push(url); - }; + }, [props.context, props.customEditDatasourceFn, datasourceId, pluginName, basePageId]);app/client/src/PluginActionEditor/components/PluginActionResponse/components/Schema/SchemaTables.tsx (2)
22-31: Consider extracting magic numbers and improving maintainability.The styled component contains hard-coded values and specific grid layout. Consider:
- Extracting the height value to a theme constant
- Using CSS custom properties for grid-template-columns
const StyledFlex = styled(Flex)` & .t--entity-item { - height: 28px; + height: ${({ theme }) => theme.sizes.entityItem.height}; - grid-template-columns: 0 auto 1fr auto auto auto auto auto; + grid-template-columns: var(--entity-item-grid-template, 0 auto 1fr auto auto auto auto auto); .entity-icon > .ads-v2-icon { display: none; } } `;
43-50: Consider debouncing the refresh action.The refresh action might benefit from debouncing to prevent rapid successive calls.
+import { debounce } from "lodash"; -const refreshStructure = useCallback(() => { +const refreshStructure = useCallback( + debounce(() => { dispatch( refreshDatasourceStructure( datasourceId, DatasourceStructureContext.QUERY_EDITOR, ), ); - }, [dispatch, datasourceId]); + }, 300), + [dispatch, datasourceId] +);app/client/src/PluginActionEditor/components/PluginActionResponse/components/Schema/StatusDisplay.tsx (1)
22-33: Add JSDoc comments to improve type documentationConsider adding JSDoc comments to describe the purpose of each state and the component props.
+/** + * Possible states for schema data loading and display + */ type IStates = | "SCHEMA_LOADING" | "LOADING" | "NOSCHEMA" | "NODATA" | "FAILED" | "NOCOLUMNS"; +/** + * Props for the StatusDisplay component + * @property {IStates} state - Current display state + * @property {Function} [editDatasource] - Optional callback for editing datasource + */ interface IProps { state: IStates; editDatasource?: () => void; }app/client/src/PluginActionEditor/components/PluginActionResponse/components/Schema/DatasourceSelector.tsx (2)
45-48: Consider making props optional with default values.The Props interface could be more flexible by making some properties optional, especially if this component might be reused in different contexts.
interface Props { - datasourceId: string; - datasourceName: string; + datasourceId?: string; + datasourceName?: string; + defaultDatasourceName?: string; }
64-70: Consider memoizing selectors for better performance.Multiple selector calls could impact performance. Consider combining related selectors or using
createSelectorfrom Redux toolkit for memoization.const combinedSelector = createSelector( [(state: AppState) => state, (state: AppState, pluginId: string) => pluginId], (state, pluginId) => ({ plugin: getPlugin(state, pluginId), dataSources: getDatasourceByPluginId(state, pluginId) }) );app/client/src/ce/constants/messages.ts (1)
2270-2274: Consider consolidating duplicate empty state messagesThe empty state messages for schema and table are nearly identical. Consider using a single set of messages to maintain consistency and reduce duplication.
-export const EMPTY_SCHEMA_TITLE_TEXT = () => "Empty schema"; -export const EMPTY_SCHEMA_MESSAGE_TEXT = () => - "There are no schema records to show"; +export const EMPTY_DATA_TITLE_TEXT = (type: string) => `Empty ${type}`; +export const EMPTY_DATA_MESSAGE_TEXT = (type: string) => + `There are no ${type} records to show`;app/client/src/PluginActionEditor/components/PluginActionResponse/components/Schema/index.tsx (1)
99-116: MemoizeeditDatasourceto prevent unnecessary re-rendersConsider using
useCallbackto memoizeeditDatasource, which will prevent unnecessary re-renders of child components receiving this function as a prop.Apply this diff:
- // eslint-disable-next-line react-perf/jsx-no-new-function-as-prop const editDatasource = () => { ... }; + const editDatasource = useCallback(() => { ... }, [props.datasourceId, currentPageId]);Also, update the import statement to include
useCallback:Outside selected line ranges:
- import React, { useEffect, useState } from "react"; + import React, { useCallback, useEffect, useState } from "react";
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
📒 Files selected for processing (13)
app/client/cypress/support/Pages/DataSources.ts(1 hunks)app/client/src/PluginActionEditor/components/PluginActionResponse/components/Schema.tsx(0 hunks)app/client/src/PluginActionEditor/components/PluginActionResponse/components/Schema/CurrentDataSource.tsx(1 hunks)app/client/src/PluginActionEditor/components/PluginActionResponse/components/Schema/DatasourceSelector.tsx(1 hunks)app/client/src/PluginActionEditor/components/PluginActionResponse/components/Schema/SchemaTables.tsx(1 hunks)app/client/src/PluginActionEditor/components/PluginActionResponse/components/Schema/StatusDisplay.tsx(1 hunks)app/client/src/PluginActionEditor/components/PluginActionResponse/components/Schema/TableColumns.tsx(1 hunks)app/client/src/PluginActionEditor/components/PluginActionResponse/components/Schema/index.tsx(1 hunks)app/client/src/ce/constants/messages.ts(2 hunks)app/client/src/pages/Editor/DatasourceInfo/DatasourceField.tsx(4 hunks)app/client/src/pages/Editor/DatasourceInfo/DatasourceStructureNotFound.tsx(1 hunks)app/client/src/pages/Editor/DatasourceInfo/DatasourceViewModeSchema.tsx(1 hunks)app/client/src/pages/Editor/DatasourceInfo/GoogleSheetSchema.tsx(1 hunks)
💤 Files with no reviewable changes (1)
- app/client/src/PluginActionEditor/components/PluginActionResponse/components/Schema.tsx
✅ Files skipped from review due to trivial changes (3)
- app/client/cypress/support/Pages/DataSources.ts
- app/client/src/pages/Editor/DatasourceInfo/DatasourceViewModeSchema.tsx
- app/client/src/pages/Editor/DatasourceInfo/GoogleSheetSchema.tsx
🔇 Additional comments (12)
app/client/src/PluginActionEditor/components/PluginActionResponse/components/Schema/CurrentDataSource.tsx (2)
19-24: LGTM! Clean Redux selector implementation
The component correctly uses Redux selectors to fetch plugin data, and the implementation aligns with the PR objective of including datasource information.
25-27: LGTM! Well-implemented navigation callback
The callback is properly memoized and uses the correct URL builder for navigation.
app/client/src/PluginActionEditor/components/PluginActionResponse/components/Schema/TableColumns.tsx (2)
1-12: LGTM! Clean and well-organized imports.
The imports are properly structured with clear separation between external libraries and internal components.
13-18: Props interface looks good!
The interface is well-defined with appropriate types for loading state, structure, and table selection.
app/client/src/pages/Editor/DatasourceInfo/DatasourceField.tsx (1)
21-21: LGTM: Styling improvements enhance readability
The font size increase and color system updates align well with the UI clean-up objectives.
Also applies to: 28-31
app/client/src/pages/Editor/DatasourceInfo/DatasourceStructureNotFound.tsx (1)
Line range hint 72-89: LGTM! Clean and consistent UI implementation
The render logic effectively implements the error state UI with clear error message display and action button, aligning with the PR objectives for UI improvements.
app/client/src/PluginActionEditor/components/PluginActionResponse/components/Schema/SchemaTables.tsx (3)
1-11: LGTM! Clean and well-organized imports.
The imports are properly organized and all dependencies are relevant to the component's functionality.
13-20: Props interface is well-defined and type-safe.
The interface clearly defines all required props with appropriate types, supporting the schema tab redesign objectives.
33-91: Component implementation looks solid, but verify the fixed width.
The implementation aligns well with the PR objectives. However, the fixed width of 400px on line 59 needs verification against the design specs.
✅ Verification successful
Let me try a different approach to search for width-related patterns in the codebase. The previous command had syntax errors due to improper escaping of special characters. The new command should help us find any instances of fixed 400px widths.
Fixed width of 400px is consistent with design system patterns
The 400px width is aligned with other components in the codebase, particularly the design system's Popover and AnnouncementModal components which use the same width. This appears to be a standardized width for similar UI elements.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for other instances of fixed widths in schema-related components
rg -g '*.{tsx,ts,js,jsx}' '(width|w)(:|\s*=)\s*("|\')?400(px)?("|\')?'
Length of output: 207
Script:
#!/bin/bash
# Search for fixed widths in schema-related components with proper escaping
rg -g "*.{tsx,ts,js,jsx}" "(width|w):\s*['\"]?400['\"]?|w=['\"]400['\"]"
Length of output: 4001
app/client/src/PluginActionEditor/components/PluginActionResponse/components/Schema/StatusDisplay.tsx (2)
116-116: LGTM!
The named export follows the module pattern consistently.
35-69: Verify CDN assets and consider constants extraction
The implementation looks good, but let's ensure the assets are properly managed.
Consider extracting the asset paths to a dedicated constants file:
+// Add to constants/AssetConstants.ts
+export const SCHEMA_ASSETS = {
+ EMPTY_STATE: `${ASSETS_CDN_URL}/empty-state.svg`,
+ FAILED_STATE: `${ASSETS_CDN_URL}/failed-state.svg`,
+};app/client/src/ce/constants/messages.ts (1)
2277-2282: LGTM! Loading and error states follow consistent patterns
The loading and error state messages follow the established pattern and provide clear user feedback.
| const columns = | ||
| find(datasourceStructure?.tables, ["name", selectedTable])?.columns || []; | ||
|
|
||
| const selectedTableItems = find(datasourceStructure?.tables, [ | ||
| "name", | ||
| selectedTable, | ||
| ]); |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Optimize duplicate table lookup.
The find operation on datasourceStructure.tables is performed twice. Consider storing the result in a variable.
- const columns =
- find(datasourceStructure?.tables, ["name", selectedTable])?.columns || [];
-
- const selectedTableItems = find(datasourceStructure?.tables, [
- "name",
- selectedTable,
- ]);
+ const selectedTableItems = find(datasourceStructure?.tables, ["name", selectedTable]);
+ const columns = selectedTableItems?.columns || [];📝 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 columns = | |
| find(datasourceStructure?.tables, ["name", selectedTable])?.columns || []; | |
| const selectedTableItems = find(datasourceStructure?.tables, [ | |
| "name", | |
| selectedTable, | |
| ]); | |
| const selectedTableItems = find(datasourceStructure?.tables, ["name", selectedTable]); | |
| const columns = selectedTableItems?.columns || []; |
...t/src/PluginActionEditor/components/PluginActionResponse/components/Schema/StatusDisplay.tsx
Outdated
Show resolved
Hide resolved
.../PluginActionEditor/components/PluginActionResponse/components/Schema/DatasourceSelector.tsx
Outdated
Show resolved
Hide resolved
.../PluginActionEditor/components/PluginActionResponse/components/Schema/DatasourceSelector.tsx
Outdated
Show resolved
Hide resolved
| </MenuGroupName> | ||
| <MenuGroup> | ||
| {DATASOURCES_OPTIONS.map((option) => ( | ||
| <MenuItem key={option.value} onSelect={() => {}}> |
There was a problem hiding this comment.
Implement missing onSelect handler for datasource selection.
The MenuItem's onSelect handler is empty, making the datasource selection non-functional.
- <MenuItem key={option.value} onSelect={() => {}}>
+ <MenuItem
+ key={option.value}
+ onSelect={() => {
+ // Implement datasource selection logic here
+ // Example: dispatch(selectDatasource(option.value));
+ }}>Committable suggestion skipped: line range outside the PR's diff.
.../PluginActionEditor/components/PluginActionResponse/components/Schema/DatasourceSelector.tsx
Show resolved
Hide resolved
There was a problem hiding this comment.
Caution
Inline review comments failed to post. This is likely due to GitHub's limits when posting large numbers of comments.
Actionable comments posted: 9
🧹 Outside diff range and nitpick comments (19)
app/client/src/PluginActionEditor/components/PluginActionResponse/components/Schema/CurrentDataSource.tsx (2)
13-17: Consider improving interface naming and documentationThe interface could be more descriptive:
- Rename
iPropstoCurrentDataSourcePropsfollowing React conventions- Add JSDoc to explain the difference between "link" and "trigger" types
-interface iProps { +/** + * Props for CurrentDataSource component + * @param type - Determines rendering behavior: + * - "link": Renders as clickable link to datasource editor + * - "trigger": Renders as static content + */ +interface CurrentDataSourceProps { type: "link" | "trigger"; datasourceId: string; datasourceName: string; }
31-42: Consider adding loading stateThe component might benefit from showing a loading state while the plugin images are being fetched.
const content = ( <Flex alignItems="center" gap="spaces-2" justifyContent={type === "link" ? "center" : "start"} > <EntityIcon height="16px" width="16px"> + {pluginImages === undefined ? ( + <Spinner size="small" /> + ) : ( <img alt={`${datasourceName} plugin icon`} src={getAssetUrl(datasourceIcon)} /> + )} </EntityIcon> {datasourceName} </Flex> );app/client/src/PluginActionEditor/components/PluginActionResponse/components/Schema/TableColumns.tsx (1)
13-17: Add JSDoc comments and consider prop types.The Props interface could benefit from documentation and type improvements.
Consider adding JSDoc comments and making selectedTable non-optional if it's required:
+/** + * Props for TableColumns component + * @property {boolean} isLoading - Indicates if the data is being loaded + * @property {DatasourceStructure} datasourceStructure - Structure of the datasource + * @property {string} selectedTable - Name of the selected table + */ interface Props { isLoading: boolean; datasourceStructure: DatasourceStructure; - selectedTable: string | undefined; + selectedTable: string; }app/client/src/pages/Editor/DatasourceInfo/DatasourceStructureNotFound.tsx (1)
45-45: Consider alternative approaches to avoid disabling ESLint rule.The ESLint rule
react-perf/jsx-no-new-function-as-propis disabled here. Consider usinguseCallbackto memoize the function instead.- // eslint-disable-next-line react-perf/jsx-no-new-function-as-prop - const editDatasource = () => { + const editDatasource = useCallback(() => { let entryPoint = DatasourceEditEntryPoints.QUERY_EDITOR_DATASOURCE_SCHEMA; // ... rest of the function - }; + }, [basePageId, datasourceId, pluginName, props.context, props.customEditDatasourceFn]);app/client/src/PluginActionEditor/components/PluginActionResponse/components/Schema/SchemaTables.tsx (2)
13-20: Add JSDoc documentation for the Props interfaceAdding documentation will improve maintainability and help other developers understand the purpose of each prop.
+/** + * Props for the SchemaTables component + * @param datasourceId - Unique identifier for the datasource + * @param datasourceName - Display name of the datasource + * @param currentActionId - Current action identifier + * @param datasourceStructure - Structure information of the datasource + * @param setSelectedTable - Callback to update the selected table + * @param selectedTable - Currently selected table identifier + */ interface Props { datasourceId: string; datasourceName: string; currentActionId: string; datasourceStructure: DatasourceStructure; setSelectedTable: (table: string) => void; selectedTable: string | undefined; }
22-31: Consider using CSS custom properties for grid templateThe grid template columns could be more maintainable using CSS custom properties.
const StyledFlex = styled(Flex)` + --entity-item-columns: 0 auto 1fr auto auto auto auto auto; & .t--entity-item { height: 28px; - grid-template-columns: 0 auto 1fr auto auto auto auto auto; + grid-template-columns: var(--entity-item-columns); .entity-icon > .ads-v2-icon { display: none; } } `;app/client/src/PluginActionEditor/components/PluginActionResponse/components/Schema/StatusDisplay.tsx (3)
1-33: Consider following TypeScript interface naming conventions.The code structure and organization look good. However, consider removing the "I" prefix from interface names (IStates, IProps) as it's not a recommended TypeScript convention.
Apply this diff:
-type IStates = +type States = | "SCHEMA_LOADING" | "LOADING" | "NOSCHEMA" | "NODATA" | "FAILED" | "NOCOLUMNS"; -interface IProps { +interface Props { state: IStates; editDatasource?: () => void; }
35-69: Add error handling for image loading.The state mapping is well-structured, but consider adding error handling for image loading failures to ensure a good user experience even when CDN assets fail to load.
Consider adding an onError handler to the img element where these images are rendered:
-<img alt={title} className="h-full" src={image} /> +<img + alt={title} + className="h-full" + src={image} + onError={(e) => { + e.currentTarget.src = 'fallback-image-url'; + }} +/>
74-84: Optimize scroll behavior for varying content sizes.The current implementation always shows a scrollbar (overflowY="scroll"). This might not be optimal for smaller content.
- overflowY={"scroll"} + overflowY={"auto"}app/client/src/PluginActionEditor/components/PluginActionResponse/components/Schema/DatasourceSelector.tsx (3)
56-86: Consider memoizing selector results.Multiple complex selectors are being used which could impact performance on re-renders. Consider using useMemo for computed values.
+ const memoizedDatasources = useMemo(() => + getDatasourceByPluginId(state, currentActionConfig?.pluginId || ""), + [currentActionConfig?.pluginId] + );
45-49: Consider using TypeScript utility types for better type safety.The interfaces could benefit from using TypeScript utility types to ensure required properties.
-interface Props { +type Props = { datasourceId: string; datasourceName: string; -} +} & { + readonly [K in keyof Props]: Props[K]; +};Also applies to: 50-55
87-100: Consider extracting analytics event to a separate utility.The analytics event logging could be moved to a separate utility function for better maintainability.
+const logDatasourceCreation = (entryPoint: string) => { + AnalyticsUtil.logEvent("NAVIGATE_TO_CREATE_NEW_DATASOURCE_PAGE", { + entryPoint, + }); +};app/client/src/ce/constants/messages.ts (2)
2270-2274: Consider consolidating duplicate message patternsThe schema-related messages follow similar patterns to the table-related messages. Consider creating a reusable message factory function to reduce duplication:
-export const EMPTY_TABLE_TITLE_TEXT = () => "Empty table"; -export const EMPTY_SCHEMA_TITLE_TEXT = () => "Empty schema"; +const createEmptyStateMessages = (type: string) => ({ + title: () => `Empty ${type}`, + message: () => `There are no ${type} records to show` +}); +export const EMPTY_TABLE_TEXT = createEmptyStateMessages("table"); +export const EMPTY_SCHEMA_TEXT = createEmptyStateMessages("schema");Also applies to: 2277-2282
Line range hint
1-2282: Consider using TypeScript namespaces for better organizationThe file has grown large with many message constants. Consider organizing related messages into TypeScript namespaces for better maintainability:
export namespace SchemaMessages { export const EMPTY_TITLE = () => "Empty schema"; export const EMPTY_MESSAGE = () => "There are no schema records to show"; // ... other schema related messages }app/client/src/PluginActionEditor/components/PluginActionResponse/components/Schema/index.tsx (5)
70-70: Check for null or undefined indatasourceStructureConsider using
datasourceStructure == nullto check for bothnullandundefinedvalues.Apply this diff:
- datasourceStructure === undefined && + datasourceStructure == null &&
86-86: Omitdispatchfrom dependency arrayThe
dispatchfunction is stable and doesn't need to be included in the dependency array ofuseEffect.Apply this diff:
- [props.datasourceId, datasourceStructure, dispatch, pluginDatasourceForm], + [props.datasourceId, datasourceStructure, pluginDatasourceForm],
95-95: OptimizeuseEffectdependencies
props.datasourceIdisn't used inside the effect and can be removed from the dependencies.Apply this diff:
- [selectedTable, props.datasourceId, isLoading, datasourceStructure], + [selectedTable, isLoading, datasourceStructure],
118-126: SimplifygetStatusStatefunctionYou can streamline the
getStatusStatefunction for better readability.Apply this diff:
const getStatusState = () => { if (isLoading) return "SCHEMA_LOADING"; if (!datasourceStructure) return "NOSCHEMA"; - if (datasourceStructure && "error" in datasourceStructure) return "FAILED"; + if ("error" in datasourceStructure) return "FAILED"; return null; };
36-38: Add type annotation todatasourceStructureFor better type safety, specify the state type in
useSelector.Apply this diff:
- const datasourceStructure = useSelector((state) => + const datasourceStructure = useSelector((state: AppState) =>
🛑 Comments failed to post (9)
app/client/src/PluginActionEditor/components/PluginActionResponse/components/Schema/CurrentDataSource.tsx (2)
37-39: 🛠️ Refactor suggestion
Improve image accessibility
The alt text should be more descriptive for screen readers.
-<img alt="entityIcon" src={getAssetUrl(datasourceIcon)} /> +<img alt={`${datasourceName} plugin icon`} src={getAssetUrl(datasourceIcon)} />Committable suggestion skipped: line range outside the PR's diff.
29-29: 🛠️ Refactor suggestion
Add null check for pluginImages access
The optional chaining on
pluginImagesmight not be sufficient ifpluginIdis truthy but invalid.-const datasourceIcon = pluginId ? pluginImages?.[pluginId] : undefined; +const datasourceIcon = pluginId && pluginImages ? pluginImages[pluginId] : undefined;📝 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 datasourceIcon = pluginId && pluginImages ? pluginImages[pluginId] : undefined;app/client/src/PluginActionEditor/components/PluginActionResponse/components/Schema/TableColumns.tsx (2)
56-64: 🛠️ Refactor suggestion
Improve key generation and add performance optimizations.
The current implementation has potential issues with key generation and performance.
Consider these improvements:
- Use a more reliable key generation method:
- key={`${field.name}${index}`} + key={`${selectedTable}-${field.name}`}
- Memoize the columnsAndKeys array to prevent unnecessary re-renders:
const columnsAndKeys = useMemo(() => { if (!selectedTableItems) return []; return [...selectedTableItems.keys, ...selectedTableItems.columns]; }, [selectedTableItems]);
24-30: 🛠️ Refactor suggestion
Optimize table lookup operations.
The component performs the same table lookup twice which is inefficient.
Consolidate the lookups:
- const columns = - find(datasourceStructure?.tables, ["name", selectedTable])?.columns || []; - - const selectedTableItems = find(datasourceStructure?.tables, [ - "name", - selectedTable, - ]); + const selectedTableItems = find(datasourceStructure?.tables, ["name", selectedTable]); + const columns = selectedTableItems?.columns || [];📝 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 selectedTableItems = find(datasourceStructure?.tables, ["name", selectedTable]); const columns = selectedTableItems?.columns || [];app/client/src/PluginActionEditor/components/PluginActionResponse/components/Schema/SchemaTables.tsx (1)
33-91: 🛠️ Refactor suggestion
Consider adding error boundary and loading states
The component handles the happy path well, but should consider error states and loading indicators for better UX.
const SchemaTables = ({ currentActionId, datasourceId, datasourceName, datasourceStructure, selectedTable, setSelectedTable, }: Props) => { const dispatch = useDispatch(); + const [isLoading, setIsLoading] = useState(false); const refreshStructure = useCallback(() => { + setIsLoading(true); dispatch( refreshDatasourceStructure( datasourceId, DatasourceStructureContext.QUERY_EDITOR, ), - ); + ).finally(() => setIsLoading(false)); }, [dispatch, datasourceId]); return ( <StyledFlex data-testid="t--datasource-schema-container" flexDirection="column" gap="spaces-3" overflow="hidden" padding="spaces-3" w="400px" > + {isLoading && <LoadingSpinner />} {/* Rest of the component */} </StyledFlex> ); };Committable suggestion skipped: line range outside the PR's diff.
app/client/src/PluginActionEditor/components/PluginActionResponse/components/Schema/StatusDisplay.tsx (1)
101-110:
⚠️ Potential issueEnsure type safety for editDatasource in FAILED state.
The editDatasource callback is optional in Props but required when state is "FAILED". This could lead to runtime errors.
Consider either:
- Making editDatasource required and handling its absence in other states, or
- Adding a runtime check:
{state === "FAILED" && ( + editDatasource && ( <Button className="mt-[16px]" kind="secondary" onClick={editDatasource} size={"sm"} > {createMessage(EDIT_DATASOURCE)} </Button> + ) )}📝 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.{state === "FAILED" && ( editDatasource && ( <Button className="mt-[16px]" kind="secondary" onClick={editDatasource} size={"sm"} > {createMessage(EDIT_DATASOURCE)} </Button> ) )}app/client/src/PluginActionEditor/components/PluginActionResponse/components/Schema/DatasourceSelector.tsx (3)
161-165: 🛠️ Refactor suggestion
Add error handling for image loading.
The img tag should have an onError handler to handle cases where the image fails to load.
<img alt="Datasource" className="plugin-image h-[12px] w-[12px]" src={getAssetUrl(option.image)} + onError={(e) => { + e.currentTarget.src = fallbackImageUrl; + }} />Committable suggestion skipped: line range outside the PR's diff.
118-119:
⚠️ Potential issueRemove debug console.log statement.
Debug logging should not be committed to production code.
- // eslint-disable-next-line no-console - console.log(`AB -> showDatasourceSelector = ${showDatasourceSelector}`);📝 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.
159-159:
⚠️ Potential issueImplement the empty onSelect handler.
The MenuItem's onSelect handler is empty, which means selecting a datasource has no effect.
- <MenuItem key={option.value} onSelect={() => {}}> + <MenuItem + key={option.value} + onSelect={() => handleDatasourceSelect(option.value)}>Committable suggestion skipped: line range outside the PR's diff.
|
/build-deploy-preview skip-test=true |
|
Deploying Your Preview: https://github.com/appsmithorg/appsmith/actions/runs/11887349304. |
|
Deploy-Preview-URL: https://ce-37420.dp.appsmith.com |
|
/build-deploy-preview skip-test=true |
|
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. |
|
Deploying Your Preview: https://github.com/appsmithorg/appsmith/actions/runs/11887962889. |
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/Schema/StatusDisplay.tsx (3)
23-35: Add JSDoc comments for better documentation.Consider adding JSDoc comments to describe the purpose of each state and the props interface. This would improve code maintainability and IDE support.
+/** + * Possible states for schema display + */ type IStates = | "SCHEMA_LOADING" | "LOADING" | "NOSCHEMA" | "NODATA" | "FAILED" | "NOCOLUMNS" | "CANTSHOW"; +/** + * Props for StatusDisplay component + * @property {IStates} state - Current display state + * @property {() => void} [editDatasource] - Optional callback for editing datasource + */ interface IProps { state: IStates; editDatasource?: () => void; }
37-75: Extract image URLs to constants for better maintainability.Consider moving the image URLs to a separate constants file to improve maintainability and reusability.
+// In a new file: constants/schemaImages.ts +export const SCHEMA_IMAGES = { + EMPTY_STATE: `${ASSETS_CDN_URL}/empty-state.svg`, + FAILED_STATE: `${ASSETS_CDN_URL}/failed-state.svg`, +} as const; // In StatusDisplay.tsx const StateData: Record< IStates, { title?: string; message?: string; image: string | ReactNode } > = { // ... NOSCHEMA: { title: createMessage(EMPTY_SCHEMA_TITLE_TEXT), message: createMessage(EMPTY_SCHEMA_MESSAGE_TEXT), - image: getAssetUrl(`${ASSETS_CDN_URL}/empty-state.svg`), + image: getAssetUrl(SCHEMA_IMAGES.EMPTY_STATE), }, // Update other occurrences similarly
91-97: Add error handling for image loading failures.Consider adding an error boundary or fallback handling for image loading failures to ensure graceful degradation.
{typeof image === "string" ? ( <Flex alignItems={"center"} h="150px" justifyContent={"center"}> - <img alt={title} className="h-full" src={image} /> + <img + alt={title} + className="h-full" + src={image} + onError={(e) => { + e.currentTarget.src = getAssetUrl(SCHEMA_IMAGES.FALLBACK); + console.error('Failed to load schema status image:', image); + }} + /> </Flex> ) : ( image )}
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
📒 Files selected for processing (3)
app/client/src/PluginActionEditor/components/PluginActionResponse/components/Schema/StatusDisplay.tsx(1 hunks)app/client/src/PluginActionEditor/components/PluginActionResponse/components/Schema/index.tsx(1 hunks)app/client/src/ce/constants/messages.ts(2 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- app/client/src/PluginActionEditor/components/PluginActionResponse/components/Schema/index.tsx
🔇 Additional comments (3)
app/client/src/PluginActionEditor/components/PluginActionResponse/components/Schema/StatusDisplay.tsx (1)
1-22: LGTM! Clean and well-organized imports.
The imports are properly organized and all imported items are utilized in the component.
app/client/src/ce/constants/messages.ts (2)
2270-2274: LGTM! Empty schema constants follow the established pattern
The new constants for empty schema states are well-named and consistent with the existing codebase's conventions.
2277-2284: LGTM! Loading schema and datasource switcher constants are properly implemented
The new constants for loading schema states and datasource switcher follow the established pattern and provide clear, user-friendly messages.
|
Deploy-Preview-URL: https://ce-37420.dp.appsmith.com |
|
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 (1)
app/client/cypress/e2e/Regression/ClientSide/BugTests/DatasourceSchema_spec.ts (1)
Line range hint
44-44: Remove Sleep() call and use proper Cypress waiting mechanisms.Using
agHelper.Sleep(1500)violates Cypress best practices. Replace with appropriate wait conditions using Cypress's built-in retry-ability.Replace with a proper wait condition:
- agHelper.Sleep(1500); + agHelper.WaitUntilAllToastsDisappear(); + // or wait for a specific element to be visible/ready + cy.get(dataSources._dsTabSchema).should('be.visible');
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
📒 Files selected for processing (1)
app/client/cypress/e2e/Regression/ClientSide/BugTests/DatasourceSchema_spec.ts(1 hunks)
🧰 Additional context used
📓 Path-based instructions (1)
app/client/cypress/e2e/Regression/ClientSide/BugTests/DatasourceSchema_spec.ts (1)
Pattern app/client/cypress/**/**.*: Review the following e2e test code written using the Cypress test library. Ensure that:
- Follow best practices for Cypress code and e2e automation.
- Avoid using cy.wait in code.
- Avoid using cy.pause in code.
- Avoid using agHelper.sleep().
- Use locator variables for locators and do not use plain strings.
- Use data-* attributes for selectors.
- Avoid Xpaths, Attributes and CSS path.
- Avoid selectors like .btn.submit or button[type=submit].
- Perform logins via API with LoginFromAPI.
- Perform logout via API with LogOutviaAPI.
- Perform signup via API with SignupFromAPI.
- Avoid using it.only.
- Avoid using after and aftereach in test cases.
- Use multiple assertions for expect statements.
- Avoid using strings for assertions.
- Do not use duplicate filenames even with different paths.
- Avoid using agHelper.Sleep, this.Sleep in any file in code.
🔇 Additional comments (1)
app/client/cypress/e2e/Regression/ClientSide/BugTests/DatasourceSchema_spec.ts (1)
82-82: Verify the index change in schema verification.
The index parameter change from 1 to 0 in VerifyColumnSchemaOnQueryEditor needs verification to ensure it's not masking a potential issue with the schema order or structure.
✅ Verification successful
Let me check the implementation of VerifyColumnSchemaOnQueryEditor to understand the index parameter usage.
Let me try a different approach to get the implementation details.
Index parameter change is correct and safe
The change from index 1 to 0 is valid as:
- The
VerifyColumnSchemaOnQueryEditormethod has a default parameter ofindex = 0 - The implementation uses
eq(index)to select elements, which is 0-based in Cypress - The method is designed to verify schema columns by their position in the list
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for other usages of VerifyColumnSchemaOnQueryEditor to understand the expected index patterns
rg "VerifyColumnSchemaOnQueryEditor" -A 2
Length of output: 639
Script:
#!/bin/bash
# Get the implementation details of VerifyColumnSchemaOnQueryEditor
ast-grep --pattern 'VerifyColumnSchemaOnQueryEditor($$$) {
$$$
}'
Length of output: 69
Script:
#!/bin/bash
# Get the implementation details using ripgrep with context
rg -A 10 "public VerifyColumnSchemaOnQueryEditor" app/client/cypress/support/Pages/DataSources.ts
Length of output: 387
…o action-redesign/schema-tab
|
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. |
…o action-redesign/schema-tab
…o action-redesign/schema-tab
|
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: 5
🧹 Outside diff range and nitpick comments (15)
app/client/src/PluginActionEditor/components/PluginActionResponse/components/Schema/Schema.tsx (2)
99-100: WrapeditDatasourcewithuseCallbackTo prevent unnecessary re-renders, consider wrapping the
editDatasourcefunction withuseCallback.Apply this change:
- const editDatasource = () => { + const editDatasource = useCallback(() => { // function body + }, [currentPageId, props.datasourceId]);
119-130: Simplify thegetStatusStatefunctionThe logic in
getStatusStatecan be streamlined for better readability.Consider this refactor:
const getStatusState = () => { if (isLoading) return SchemaDisplayStatus.SCHEMA_LOADING; - if (!datasourceStructure) return SchemaDisplayStatus.NOSCHEMA; - if (datasourceStructure && "error" in datasourceStructure) + if (datasourceStructure?.error) return SchemaDisplayStatus.FAILED; - if (isEmpty(datasourceStructure)) return SchemaDisplayStatus.CANTSHOW; + if (!datasourceStructure || isEmpty(datasourceStructure)) + return SchemaDisplayStatus.NOSCHEMA; return null; };app/client/src/ce/PluginActionEditor/hooks/useGoToDatasource.ts (1)
6-10: Consider adding input validation.The function should validate the datasourceId before navigation to prevent potential issues with invalid IDs.
const goToDatasource = useCallback((datasourceId: string) => { + if (!datasourceId?.trim()) { + return; + } history.push( datasourcesEditorIdURL({ datasourceId, generateEditorPath: true }), ); }, []);app/client/src/ce/PluginActionEditor/hooks/useCreateDatasource.ts (1)
17-19: Consider making entry point configurableThe analytics entry point is hardcoded to
QUERY_EDITOR, but the hook might be used from different contexts.+ (selectedTab: string, pageId?: string, entryPoint = DatasourceCreateEntryPoints.QUERY_EDITOR) => { // ... AnalyticsUtil.logEvent("NAVIGATE_TO_CREATE_NEW_DATASOURCE_PAGE", { - entryPoint: DatasourceCreateEntryPoints.QUERY_EDITOR, + entryPoint, });app/client/src/PluginActionEditor/components/PluginActionResponse/components/Schema/CurrentDataSourceLink.tsx (2)
13-18: Consider adding error handling for navigation failuresThe click handler implementation is clean and properly memoized. Consider adding error handling for cases where navigation might fail.
const handleClick = useCallback( - () => goToDatasource(datasourceId), + async () => { + try { + await goToDatasource(datasourceId); + } catch (error) { + // Handle navigation error + console.error('Failed to navigate to datasource:', error); + } + }, [datasourceId, goToDatasource], );
20-27: Add accessibility attributes to the Link componentConsider adding accessibility attributes to improve the user experience for screen readers.
- <Link onClick={handleClick}> + <Link + onClick={handleClick} + aria-label={`Navigate to ${datasourceName} datasource details`} + role="link" + >app/client/src/PluginActionEditor/components/PluginActionResponse/components/Schema/CurrentDataSource.tsx (1)
16-32: Consider adding error handling and performance optimizationsA few suggestions to improve the component:
- Add error handling for invalid datasourceId
- Improve accessibility with meaningful alt text
- Consider memoizing the component to prevent unnecessary re-renders
-const CurrentDataSource = ({ datasourceId, datasourceName }: Props) => { +const CurrentDataSource = React.memo(({ datasourceId, datasourceName }: Props) => { const { pluginId, pluginImages } = useSelector((state) => ({ pluginId: getPluginIdFromDatasourceId(state, datasourceId), pluginImages: getPluginImages(state), })); const datasourceIcon = pluginId ? pluginImages?.[pluginId] : undefined; + if (!pluginId) { + console.warn(`No plugin found for datasource: ${datasourceId}`); + } return ( <Flex alignItems="center" gap="spaces-2"> <EntityIcon height="16px" width="16px"> - <img alt="entityIcon" src={getAssetUrl(datasourceIcon)} /> + <img + alt={`${datasourceName} plugin icon`} + src={getAssetUrl(datasourceIcon)} + /> </EntityIcon> {datasourceName} </Flex> ); -}; +});app/client/src/PluginActionEditor/components/PluginActionResponse/components/Schema/StatusDisplay.tsx (3)
33-37: Add JSDoc documentation for Props interfaceAdding documentation will improve code maintainability and help other developers understand the purpose of each prop.
+/** + * Props for the StatusDisplay component + * @property {SchemaDisplayStatus} state - Current state of the schema display + * @property {() => void} [editDatasource] - Optional callback to edit the datasource + * @property {string} [errorMessage] - Optional error message to display + */ interface Props { state: SchemaDisplayStatus; editDatasource?: () => void; errorMessage?: string; }
39-77: Extract common image paths to constantsMultiple states use the same image paths. Consider extracting these to named constants to improve maintainability.
+const EMPTY_STATE_IMAGE = getAssetUrl(`${ASSETS_CDN_URL}/empty-state.svg`); +const FAILED_STATE_IMAGE = getAssetUrl(`${ASSETS_CDN_URL}/failed-state.svg`); const StateData: Record< SchemaDisplayStatus, { title?: string; message?: string; image: string | ReactNode } > = { // ... other states ... NOSCHEMA: { title: createMessage(EMPTY_SCHEMA_TITLE_TEXT), message: createMessage(EMPTY_SCHEMA_MESSAGE_TEXT), - image: getAssetUrl(`${ASSETS_CDN_URL}/empty-state.svg`), + image: EMPTY_STATE_IMAGE, }, // Update other states similarly
108-110: Improve error message handlingThe current error message handling could be more robust. Consider adding error message trimming and fallback text.
<Text kind="body-m"> - {state === "FAILED" && errorMessage ? errorMessage : message} + {state === "FAILED" && errorMessage + ? errorMessage.trim() || message + : message} </Text>app/client/src/PluginActionEditor/components/PluginActionResponse/components/Schema/TableColumns.tsx (3)
22-36: Consider restricting prop spreading for better type safety.The spread operator
{...props}could allow unexpected props to be passed to the Flex component. Consider explicitly listing the allowed props.- {...props} + width={props.width} + minWidth={props.minWidth} + maxWidth={props.maxWidth} + gap={props.gap}
54-66: Consider optimizing nested operations for large datasets.The nested filter operation inside the map could impact performance with large datasets. Consider pre-processing the keys.
const columns = useMemo(() => { + const columnToKeys = new Map(); + selectedTableItems.keys.forEach((key) => { + key.columnNames?.forEach((col) => { + if (!columnToKeys.has(col)) columnToKeys.set(col, []); + columnToKeys.get(col).push(key.type); + }); + key.fromColumns?.forEach((col) => { + if (!columnToKeys.has(col)) columnToKeys.set(col, []); + columnToKeys.get(col).push(key.type); + }); + }); + return selectedTableItems.columns.map((column) => ({ name: column.name, type: column.type, - keys: selectedTableItems.keys - .filter( - (key) => - key.columnNames?.includes(column.name) || - key.fromColumns?.includes(column.name), - ) - .map((key) => key.type), + keys: columnToKeys.get(column.name) || [], })); }, [selectedTableItems]);
69-78: Consider caching Fuse options.The Fuse options object is recreated on every render. Consider extracting it to a constant.
+const FUSE_OPTIONS = { + keys: ["name"], + shouldSort: true, + threshold: 0.5, + location: 0, +}; const columnsFuzy = useMemo( () => - new Fuse(columns, { - keys: ["name"], - shouldSort: true, - threshold: 0.5, - location: 0, - }), + new Fuse(columns, FUSE_OPTIONS), [columns], );app/client/src/PluginActionEditor/components/PluginActionResponse/components/Schema/DatasourceSelector.tsx (2)
1-46: Consider organizing imports for better maintainabilityThe heavy reliance on enterprise edition imports suggests this component might need better separation of concerns.
Consider:
- Creating a base component for common functionality
- Extending it with enterprise features using composition
- Moving shared types to a common types file
134-138: Add form validationThe Redux form configuration lacks type validation for the form values.
export default reduxForm<Action, CustomProps>({ form: QUERY_EDITOR_FORM_NAME, destroyOnUnmount: false, enableReinitialize: true, + validate: (values: Action) => { + const errors: Record<string, string> = {}; + if (!values.datasource?.id) { + errors['datasource.id'] = 'Datasource is required'; + } + return errors; + }, })(DatasourceSelector);
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
📒 Files selected for processing (17)
app/client/src/PluginActionEditor/components/PluginActionResponse/components/Schema/CurrentDataSource.tsx(1 hunks)app/client/src/PluginActionEditor/components/PluginActionResponse/components/Schema/CurrentDataSourceLink.tsx(1 hunks)app/client/src/PluginActionEditor/components/PluginActionResponse/components/Schema/DatasourceSelector.tsx(1 hunks)app/client/src/PluginActionEditor/components/PluginActionResponse/components/Schema/Schema.tsx(1 hunks)app/client/src/PluginActionEditor/components/PluginActionResponse/components/Schema/SchemaTables.tsx(1 hunks)app/client/src/PluginActionEditor/components/PluginActionResponse/components/Schema/StatusDisplay.tsx(1 hunks)app/client/src/PluginActionEditor/components/PluginActionResponse/components/Schema/TableColumns.tsx(1 hunks)app/client/src/PluginActionEditor/components/PluginActionResponse/components/Schema/constants.tsx(1 hunks)app/client/src/PluginActionEditor/components/PluginActionResponse/components/Schema/index.tsx(1 hunks)app/client/src/PluginActionEditor/components/PluginActionResponse/components/Schema/styles.ts(1 hunks)app/client/src/ce/PluginActionEditor/components/PluginActionResponse/hooks/usePluginActionResponseTabs.tsx(1 hunks)app/client/src/ce/PluginActionEditor/hooks/useCreateDatasource.ts(1 hunks)app/client/src/ce/PluginActionEditor/hooks/useGoToDatasource.ts(1 hunks)app/client/src/ee/PluginActionEditor/hooks/useCreateDatasource.ts(1 hunks)app/client/src/ee/PluginActionEditor/hooks/useGoToDatasource.ts(1 hunks)app/client/src/pages/Editor/DatasourceInfo/DatasourceStructureNotFound.tsx(3 hunks)app/client/src/pages/Editor/QueryEditor/QueryDebuggerTabs.tsx(1 hunks)
✅ Files skipped from review due to trivial changes (7)
- app/client/src/PluginActionEditor/components/PluginActionResponse/components/Schema/constants.tsx
- app/client/src/PluginActionEditor/components/PluginActionResponse/components/Schema/index.tsx
- app/client/src/PluginActionEditor/components/PluginActionResponse/components/Schema/styles.ts
- app/client/src/ce/PluginActionEditor/components/PluginActionResponse/hooks/usePluginActionResponseTabs.tsx
- app/client/src/ee/PluginActionEditor/hooks/useCreateDatasource.ts
- app/client/src/ee/PluginActionEditor/hooks/useGoToDatasource.ts
- app/client/src/pages/Editor/QueryEditor/QueryDebuggerTabs.tsx
🚧 Files skipped from review as they are similar to previous changes (2)
- app/client/src/PluginActionEditor/components/PluginActionResponse/components/Schema/SchemaTables.tsx
- app/client/src/pages/Editor/DatasourceInfo/DatasourceStructureNotFound.tsx
🔇 Additional comments (12)
app/client/src/ce/PluginActionEditor/hooks/useGoToDatasource.ts (4)
1-3: LGTM! Clean and well-organized imports.
The imports follow a logical order and include all necessary dependencies.
5-13: LGTM! Well-structured hook implementation.
The hook follows React conventions and properly memoizes the navigation function.
15-15: LGTM! Clean export statement.
Using named export is the correct approach here.
5-13: Verify integration with CurrentDataSourceLink component.
The AI summary mentions this hook is used in CurrentDataSourceLink component. Let's verify the integration.
✅ Verification successful
Integration with CurrentDataSourceLink component is properly maintained
The hook is correctly imported and used in app/client/src/PluginActionEditor/components/PluginActionResponse/components/Schema/CurrentDataSourceLink.tsx. The enterprise edition exports the hook from the community edition, maintaining proper integration.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for usage of useGoToDatasource hook
rg "useGoToDatasource" --type ts --type tsx -A 5
Length of output: 76
Script:
#!/bin/bash
# Search for usage of useGoToDatasource hook with correct file types
rg "useGoToDatasource" -t ts -t jsx -t tsx
# Search for CurrentDataSourceLink component
rg "CurrentDataSourceLink" -t ts -t jsx -t tsx -A 5
Length of output: 150
Script:
#!/bin/bash
# Search for files containing useGoToDatasource
fd -e ts -e tsx -e js -e jsx | xargs rg "useGoToDatasource"
# Search for files containing CurrentDataSourceLink
fd -e ts -e tsx -e js -e jsx | xargs rg "CurrentDataSourceLink"
Length of output: 1387
app/client/src/PluginActionEditor/components/PluginActionResponse/components/Schema/CurrentDataSourceLink.tsx (2)
6-12: LGTM! Clean TypeScript props definition
The component props are well-typed and follow TypeScript best practices.
1-4: Verify enterprise edition dependency availability
The component imports from the enterprise edition (ee) path. Ensure this dependency is available in all deployment scenarios.
✅ Verification successful
Based on the search results, I can see that the ee directory imports are extensively used throughout the codebase, including in many core features and components. Let me verify the specific hook being imported.
Let me try another search to verify the hook's implementation.
Let me check the implementation in both CE and EE paths to verify the hook's availability.
Let me check one final thing - the datasourcesEditorIdURL import path.
The import from 'ee' path is safe to use
The hook is properly implemented in both CE and EE paths. The EE version simply re-exports the CE implementation, and the required URL builder functionality is available in both editions. The import path follows the established pattern used throughout the codebase.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Check for enterprise edition imports across the codebase
rg -l "from ['\"]ee/" | sort
Length of output: 83686
Script:
# Check if the useGoToDatasource hook exists in both CE and EE paths
ast-grep --pattern 'export const useGoToDatasource = $_'
Length of output: 142003
Script:
# Search for useGoToDatasource implementation
rg -l "useGoToDatasource" --type ts
Length of output: 273
Script:
#!/bin/bash
# Check the implementation in both CE and EE paths
echo "=== CE Implementation ==="
cat app/client/src/ce/PluginActionEditor/hooks/useGoToDatasource.ts
echo -e "\n=== EE Implementation ==="
cat app/client/src/ee/PluginActionEditor/hooks/useGoToDatasource.ts
Length of output: 731
Script:
#!/bin/bash
# Check the RouteBuilder implementation
cat app/client/src/ce/RouteBuilder.ts
Length of output: 4953
app/client/src/PluginActionEditor/components/PluginActionResponse/components/Schema/CurrentDataSource.tsx (3)
11-14: LGTM! Props interface is well-defined
The Props interface clearly specifies the required properties with appropriate types.
34-34: LGTM! Export statement follows best practices
The named export is appropriate for this component.
1-10: Verify enterprise edition imports availability
The component uses enterprise edition utilities. Ensure these imports are available in all deployment contexts.
✅ Verification successful
Enterprise edition imports are properly structured
The imports are correctly implemented with proper fallback to Community Edition (CE):
airgapHelpersandentitiesSelectorexist in both CE and EE paths- EE versions re-export from CE versions, ensuring availability in all contexts
- Multiple components across the codebase successfully use these imports
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Check if enterprise edition utilities are properly exported
rg -A 3 "export.*getAssetUrl" "ee/utils/airgapHelpers"
rg -A 3 "export.*getPluginIdFromDatasourceId|getPluginImages" "ee/selectors/entitiesSelector"
Length of output: 281
Script:
#!/bin/bash
# Let's check if these files exist in a different location
fd -g "*airgapHelpers*"
fd -g "*entitiesSelector*"
# Let's also check for any references to these imports
rg "from.*airgapHelpers"
rg "from.*entitiesSelector"
# Check if there's a CE (Community Edition) version of these files
fd -g "*airgapHelpers*" -g "*entitiesSelector*" app/client/src/ce
Length of output: 33690
app/client/src/PluginActionEditor/components/PluginActionResponse/components/Schema/StatusDisplay.tsx (1)
83-92: Previous review comment is still valid
app/client/src/PluginActionEditor/components/PluginActionResponse/components/Schema/TableColumns.tsx (1)
38-52: Well-structured component with efficient memoization!
The use of useMemo for selectedTableItems computation prevents unnecessary recalculations.
app/client/src/PluginActionEditor/components/PluginActionResponse/components/Schema/DatasourceSelector.tsx (1)
79-93: 🛠️ Refactor suggestion
Optimize datasource options construction
The DATASOURCES_OPTIONS array is recreated on every render, which could impact performance.
-const DATASOURCES_OPTIONS: Array<DATASOURCES_OPTIONS_TYPE> =
- dataSources.reduce(
+const DATASOURCES_OPTIONS = React.useMemo(
+ () => dataSources.reduce(
(acc: Array<DATASOURCES_OPTIONS_TYPE>, dataSource: Datasource) => {
if (dataSource.pluginId === plugin?.id) {
acc.push({
label: dataSource.name,
value: dataSource.id,
image: pluginImages[dataSource.pluginId],
});
}
return acc;
},
[],
- );
+ ),
+ [dataSources, plugin?.id, pluginImages]
+);Likely invalid or redundant comment.
...nt/src/PluginActionEditor/components/PluginActionResponse/components/Schema/TableColumns.tsx
Show resolved
Hide resolved
.../PluginActionEditor/components/PluginActionResponse/components/Schema/DatasourceSelector.tsx
Show resolved
Hide resolved
…o action-redesign/schema-tab
|
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 (2)
app/client/src/PluginActionEditor/components/PluginActionResponse/hooks/useGoToDatasource.ts (2)
6-10: Add type safety improvements and error handlingConsider adding parameter validation and explicit typing:
- const goToDatasource = useCallback((datasourceId: string) => { + const goToDatasource = useCallback((datasourceId: EntityId) => { + if (!datasourceId) { + throw new Error("Invalid datasource ID"); + } history.push( datasourcesEditorIdURL({ datasourceId, generateEditorPath: true }), ); }, []);
5-13: Add explicit return type definitionConsider adding an explicit return type for better type safety:
-function useGoToDatasource() { +interface GoToDatasourceHook { + goToDatasource: (datasourceId: EntityId) => void; +} + +function useGoToDatasource(): GoToDatasourceHook {
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
📒 Files selected for processing (2)
app/client/src/PluginActionEditor/components/PluginActionResponse/components/Schema/CurrentDataSourceLink.tsx(1 hunks)app/client/src/PluginActionEditor/components/PluginActionResponse/hooks/useGoToDatasource.ts(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- app/client/src/PluginActionEditor/components/PluginActionResponse/components/Schema/CurrentDataSourceLink.tsx
🔇 Additional comments (2)
app/client/src/PluginActionEditor/components/PluginActionResponse/hooks/useGoToDatasource.ts (2)
1-3: LGTM! Clean and well-organized imports
The imports are properly structured and all necessary dependencies are included.
15-15: LGTM! Clean export statement
The export is properly defined and follows best practices.
## Description Updated schema tab as per new design. https://www.figma.com/design/8L9BXMzNTKboGWlHpdXyYP/Appsmith-IDE?node-id=3071-101845&node-type=text&m=dev Fixes appsmithorg#35289 ## Automation /ok-to-test tags="@tag.Sanity" ### 🔍 Cypress test results <!-- This is an auto-generated comment: Cypress test results --> > [!TIP] > 🟢 🟢 🟢 All cypress tests have passed! 🎉 🎉 🎉 > Workflow run: <https://github.com/appsmithorg/appsmith/actions/runs/12022221992> > Commit: c38fc99 > <a href="https://internal.appsmith.com/app/cypress-dashboard/rundetails-65890b3c81d7400d08fa9ee5?branch=master&workflowId=12022221992&attempt=2" target="_blank">Cypress dashboard</a>. > Tags: `@tag.Sanity` > Spec: > <hr>Tue, 26 Nov 2024 03:45:12 UTC <!-- end of auto-generated comment: Cypress test results --> ## Communication Should the DevRel and Marketing teams inform users about this change? - [ ] Yes - [x] No <!-- This is an auto-generated comment: release notes by coderabbit.ai --> ## Summary by CodeRabbit ## Release Notes - **New Features** - Introduced new components: `CurrentDataSource`, `DatasourceSelector`, `SchemaTables`, `TableColumns`, `StatusDisplay`, `CurrentDataSourceLink`, `Schema`, `MenuField`, and custom hooks `useCreateDatasource` and `useGoToDatasource`. - Added constants for improved user feedback in schema-related messages. - **Improvements** - Enhanced layout and styling for various components, including `BottomView` and `Schema`. - Updated test identifiers for better consistency and testability. - **Bug Fixes** - Adjusted test cases to ensure accurate schema validation. - **Refactor** - Updated internal logic for handling datasource interactions and state management across components. <!-- end of auto-generated comment: release notes by coderabbit.ai -->
Description
Updated schema tab as per new design. https://www.figma.com/design/8L9BXMzNTKboGWlHpdXyYP/Appsmith-IDE?node-id=3071-101845&node-type=text&m=dev
Fixes #35289
Automation
/ok-to-test tags="@tag.Sanity"
🔍 Cypress test results
Tip
🟢 🟢 🟢 All cypress tests have passed! 🎉 🎉 🎉
Workflow run: https://github.com/appsmithorg/appsmith/actions/runs/12022221992
Commit: c38fc99
Cypress dashboard.
Tags:
@tag.SanitySpec:
Tue, 26 Nov 2024 03:45:12 UTC
Communication
Should the DevRel and Marketing teams inform users about this change?
Summary by CodeRabbit
Release Notes
New Features
CurrentDataSource,DatasourceSelector,SchemaTables,TableColumns,StatusDisplay,CurrentDataSourceLink,Schema,MenuField, and custom hooksuseCreateDatasourceanduseGoToDatasource.Improvements
BottomViewandSchema.Bug Fixes
Refactor