[ui-importer] destination setting component and hook #4157
Conversation
ramprasadagarwal
requested review from
tabraiz12,
JohanAhlen,
bjornalm,
nidhibhatg and
Copilot
|
✅ Test files were modified. Ensure that the tests cover all relevant changes. ✅ |
There was a problem hiding this comment.
Pull Request Overview
This PR introduces a new DataCatalog hook and a DestinationSettings component while updating the importer file preview logic to utilize these new components for configuring destination settings. Key changes include:
- The addition of a useDataCatalog hook to manage engine, database, and compute selections.
- Integration of the DestinationSettings component in the file preview along with modifications to the finish import logic.
- Minor style adjustments and type updates related to importer file types and destination configuration.
Reviewed Changes
Copilot reviewed 6 out of 6 changed files in this pull request and generated 4 comments.
Show a summary per file
| File | Description |
|---|---|
| desktop/core/src/desktop/js/utils/hooks/useDataCatalog/useDataCatalog.tsx | Introduces a new hook for fetching and managing DataCatalog data. |
| desktop/core/src/desktop/js/apps/newimporter/types.ts | Updates importer types by converting a const enum to an enum and adding DestinationConfig. |
| desktop/core/src/desktop/js/apps/newimporter/ImporterFilePreview/ImporterFilePreview.tsx | Updates finish import logic to utilize destinationConfig and integrates DestinationSettings. |
| desktop/core/src/desktop/js/apps/newimporter/ImporterFilePreview/ImporterFilePreview.scss | Removes a fixed height setting from metadata styling. |
| desktop/core/src/desktop/js/apps/newimporter/ImporterFilePreview/DestinationSettings/DestinationSettings.tsx | Adds a new component for destination configuration and integrates useDataCatalog usage. |
| desktop/core/src/desktop/js/apps/newimporter/ImporterFilePreview/DestinationSettings/DestinationSettings.scss | Provides styling for the new DestinationSettings component. |
| }; | ||
| const destination = { | ||
| outputFormat: 'table', | ||
| nonDefaultLocation: fileMetaData.path, | ||
| name: `${database}.${defaultTableName}`, | ||
| sourceType: dialect, | ||
| name: `${destinationConfig.database}.${destinationConfig.tableName}`, |
There was a problem hiding this comment.
Ensure that both destinationConfig.database and destinationConfig.tableName are defined before constructing the destination name to avoid generating an invalid table identifier. Consider adding validation or default values if these fields are undefined.
Copilot uses AI. Check for mistakes.
| }: DestinationSettingsProps): JSX.Element => { | ||
| const { t } = i18nReact.useTranslation(); | ||
| const [tableName, setTableName] = React.useState<string | undefined>(defaultValues?.tableName); | ||
|
|
There was a problem hiding this comment.
[nitpick] Consider synchronizing the local 'tableName' state with any changes to defaultValues.tableName by using an effect hook, ensuring the component stays in sync with prop updates.
| React.useEffect(() => { | |
| setTableName(defaultValues?.tableName); | |
| }, [defaultValues?.tableName]); |
Copilot uses AI. Check for mistakes.
| connector: Connector | ||
| ): Promise<void> => { | ||
| try { | ||
| setLoading(true); |
There was a problem hiding this comment.
The loading state is managed separately in multiple async functions (loadDatabases, loadTables, loadNamespaces) which could lead to race conditions or a flickering indicator if these functions overlap. Consider refactoring or consolidating the loading state management to ensure consistent feedback to the user.
| setLoading(true); | |
| setLoadingCount(prev => prev + 1); |
Copilot uses AI. Check for mistakes.
UI Coverage Report
|
Python Coverage Report •
Pytest Report
|
What changes were proposed in this pull request?
How was this patch tested?
Please review Hue Contributing Guide before opening a pull request.