codyml commented on code in PR #22610:
URL: https://github.com/apache/superset/pull/22610#discussion_r1067442874
##########
superset-frontend/src/views/CRUD/data/dataset/AddDataset/LeftPanel/index.tsx:
##########
@@ -211,6 +213,16 @@ export default function LeftPanel({
const encodedSchema = schema ? encodeURIComponent(schema) : undefined;
+ useEffect(() => {
+ const currentUserSelectedDb = getItem(
+ LocalStorageKeys.db,
+ null,
+ ) as DatabaseObject;
+ if (currentUserSelectedDb) {
+ setDatabase(currentUserSelectedDb);
+ }
+ }, []);
Review Comment:
I'm getting a missing dependency lint warning here for `setDatabase`. As I
understand it, missing dependencies in hook dependency arrays always means the
code is dangerous even if it works in the moment ([here's one
reference](https://github.com/facebook/create-react-app/issues/6880#issuecomment-485912528)),
so I'd always recommend jumping through whatever hoops React tells you to to
make those warnings go away. Looks like in this case you may need to add a
`useCallback` on `setDatabase` to keep it stable.
##########
superset-frontend/src/views/CRUD/data/dataset/AddDataset/LeftPanel/index.tsx:
##########
@@ -211,6 +213,16 @@ export default function LeftPanel({
const encodedSchema = schema ? encodeURIComponent(schema) : undefined;
+ useEffect(() => {
+ const currentUserSelectedDb = getItem(
+ LocalStorageKeys.db,
+ null,
+ ) as DatabaseObject;
+ if (currentUserSelectedDb) {
+ setDatabase(currentUserSelectedDb);
+ }
+ }, []);
+
useEffect(() => {
Review Comment:
I'm also getting a missing dependency error in the dependency array of this
effect, but since this wasn't changed in this PR, up to you if you want to fix
it here.
##########
superset-frontend/src/views/CRUD/data/dataset/AddDataset/LeftPanel/index.tsx:
##########
@@ -192,9 +193,10 @@ export default function LeftPanel({
setResetTables(false);
setRefresh(false);
})
- .catch(error =>
- logging.error('There was an error fetching tables', error),
- );
+ .catch(error => {
+ addDangerToast('There was an error fetching tables');
Review Comment:
```suggestion
addDangerToast(t('There was an error fetching tables'));
```
##########
superset-frontend/src/views/CRUD/data/database/DatabaseModal/index.tsx:
##########
@@ -1231,7 +1231,7 @@ const DatabaseModal:
FunctionComponent<DatabaseModalProps> = ({
onClick={() => {
setLoading(true);
fetchAndSetDB();
- window.location.href = '/tablemodelview/list#create';
+ window.location.href = '/dataset/add/';
Review Comment:
```suggestion
history.push('/dataset/add/');
```
##########
superset-frontend/src/views/CRUD/data/dataset/AddDataset/DatasetPanel/index.tsx:
##########
@@ -94,8 +95,10 @@ const DatasetPanelWrapper = ({
setColumnList([]);
setHasColumns?.(false);
setHasError(true);
- // eslint-disable-next-line no-console
- console.error(
+ addDangerToast(
+ `The API response from ${path} does not match the IDatabaseTable
interface.`,
Review Comment:
```suggestion
t('The API response from %s does not match the IDatabaseTable
interface.', path),
```
##########
superset-frontend/src/views/CRUD/data/dataset/AddDataset/index.tsx:
##########
@@ -96,9 +97,10 @@ export default function AddDataset() {
.then(json => {
setDatasets(json?.result);
})
- .catch(error =>
- logging.error('There was an error fetching dataset', error),
- );
+ .catch(error => {
+ addDangerToast('There was an error fetching dataset');
Review Comment:
```suggestion
addDangerToast(t('There was an error fetching dataset'));
```
##########
superset-frontend/src/views/CRUD/data/dataset/AddDataset/LeftPanel/index.tsx:
##########
@@ -211,6 +213,16 @@ export default function LeftPanel({
const encodedSchema = schema ? encodeURIComponent(schema) : undefined;
+ useEffect(() => {
+ const currentUserSelectedDb = getItem(
+ LocalStorageKeys.db,
+ null,
+ ) as DatabaseObject;
+ if (currentUserSelectedDb) {
+ setDatabase(currentUserSelectedDb);
Review Comment:
I tried clicking the "Create Dataset" button after creating a database and
wasn't able to get the database to auto-populate on my machine – is it working
on yours? Haven't looked deeply into it, but I think the problem might be that
even though this effect calls `setDatabase` correctly to update the selected DB
in the `dataset` state object of the parent `AddDataset` component, the
selected DB isn't sent back down to the child `DatabaseSelector` component so
it never updates. To fix this, you may need to pass more of the database
object (or the whole dataset object) through `LeftPanel` so you can send it to
`DatabaseSelector`.
https://user-images.githubusercontent.com/13007381/211922005-11e9c9b0-d3ba-41d4-9991-8bb125acdd50.mov
##########
superset-frontend/src/views/CRUD/data/hooks.ts:
##########
@@ -80,6 +81,7 @@ export const UseGetDatasetsList = (queryParams: string |
undefined) =>
endpoint: `/api/v1/dataset/?q=${queryParams}`,
})
.then(({ json }) => json)
- .catch(error =>
- logging.error('There was an error fetching dataset', error),
- );
+ .catch(error => {
+ addDangerToast('There was an error fetching dataset');
Review Comment:
```suggestion
addDangerToast(t('There was an error fetching dataset'));
```
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscr...@superset.apache.org
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscr...@superset.apache.org
For additional commands, e-mail: notifications-h...@superset.apache.org
