fix(sqllab): avoid unexpected re-rendering on DatabaseSelector

[justinpark]

SUMMARY

Reopen: #21141

Since DatabaseSelector component has an unnecessary key, DatabaseSelector will be re-mounted rather than update whenever currentDatabase object is updated. This causes the unexpected duplicate database list api request.
This commit removes this key so will skip unnecessary re-rendering and data fetching.
It also fixes the logic for currentDb that directly transform from db instead of useState. (This resolves the issue for #21174)

BEFORE/AFTER SCREENSHOTS OR ANIMATED GIF

Before:

After:

TESTING INSTRUCTIONS

go to Sql Editor and check the network list

ADDITIONAL INFORMATION

• Has associated issue:
• Required feature flags:
• Changes UI
• Includes DB Migration (follow approval process in SIP-59)
  • Migration is atomic, supports rollback & is backwards-compatible
  • Confirm DB migration upgrade and downgrade tested
  • Runtime estimates and downtime expectations provided
• Introduces new feature or API
• Removes existing feature or API

cc: @ktmud

[codecov]

Codecov Report

Merging #21316 (2b1abdc) into master (d67b046) will increase coverage by 0.00%.
The diff coverage is 88.88%.

@@           Coverage Diff           @@
##           master   #21316   +/-   ##
=======================================
  Coverage   66.66%   66.66%           
=======================================
  Files        1794     1794           
  Lines       68639    68643    +4     
  Branches     7300     7301    +1     
=======================================
+ Hits        45755    45764    +9     
+ Misses      21014    21011    -3     
+ Partials     1870     1868    -2     

Flag	Coverage Δ
javascript	52.82% <88.88%> (+0.01%)	⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

Impacted Files	Coverage Δ
...et-frontend/src/components/TableSelector/index.tsx	80.00% <ø> (ø)
...frontend/src/components/DatabaseSelector/index.tsx	93.93% <80.00%> (-1.30%)	⬇️
...set-frontend/src/components/Select/AsyncSelect.tsx	87.97% <100.00%> (+3.36%)	⬆️

📣 We’re building smart automated test selection to slash your CI/CD build times. Learn more

[michael-s-molina]

Thanks for the PR @justinpark. Is there a reason for removing AsyncSelect useEffect dependencies?

[justinpark]

Is there a reason for removing AsyncSelect useEffect dependencies?

@michael-s-molina Because fetchPage kept changing while initializing due to the object dependencies, useEffect has been triggered multiple times which causes unnecessary fetchPage calls.

I made a change to reduce and fix the dependencies for fetchPage and rollback this useEffect dependencies.

[michael-s-molina]

Is there a reason for removing AsyncSelect useEffect dependencies?

@michael-s-molina Because fetchPage kept changing while initializing due to the object dependencies, useEffect has been triggered multiple times which causes unnecessary fetchPage calls.

I made a change to reduce and fix the dependencies for fetchPage and rollback this useEffect dependencies.

@justinpark We can't add the responsibility of the immutability of the options to the Select component because there are some cases where we want to fire the requests again when the function changes. To fix your problem, you just need to make sure options only change when you in fact want them to. This is generally accomplished by the use of useMemo or useCallback on the parent component depending on the case.

[justinpark]

Is there a reason for removing AsyncSelect useEffect dependencies?

michael-s-molina Because fetchPage kept changing while initializing due to the object dependencies, useEffect has been triggered multiple times which causes unnecessary fetchPage calls.
I made a change to reduce and fix the dependencies for fetchPage and rollback this useEffect dependencies.

justinpark We can't add the responsibility of the immutability of the options to the Select component because there are some cases where we want to fire the requests again when the function changes. To fix your problem, you just need to make sure options only change when you in fact want them to. This is generally accomplished by the use of useMemo or useCallback on the parent component depending on the case.

Okay. @michael-s-molina I finally cleaned up the main cause.

1. I removed the selectValue dependency on sortSelectedFirst and useRef instead because this is the major reason that triggers fetchPage multiple times. mergeData function will be updated whenever selectedValue is updated due to the dependency of sortSelectedFirst.

superset/superset-frontend/src/components/Select/AsyncSelect.tsx

Lines 196 to 200 in 8c16806

	const sortSelectedFirst = useCallback(
	(a: AntdLabeledValue, b: AntdLabeledValue) =>
	sortSelectedFirstHelper(a, b, selectValue),
	[selectValue],
	);

superset/superset-frontend/src/components/Select/AsyncSelect.tsx

Lines 214 to 223 in 8c16806

	const sortComparatorForNoSearch = useCallback(
	(a: AntdLabeledValue, b: AntdLabeledValue) =>
	sortComparatorForNoSearchHelper(
	a,
	b,
	sortSelectedFirst,
	sortComparator,
	),
	[sortComparator, sortSelectedFirst],
	);

superset/superset-frontend/src/components/Select/AsyncSelect.tsx

Lines 302 to 319 in 8c16806

	const mergeData = useCallback(
	(data: SelectOptionsType) => {
	let mergedData: SelectOptionsType = [];
	if (data && Array.isArray(data) && data.length) {
	// unique option values should always be case sensitive so don't lowercase
	const dataValues = new Set(data.map(opt => opt.value));
	// merges with existing and creates unique options
	setSelectOptions(prevOptions => {
	mergedData = prevOptions
	.filter(previousOption => !dataValues.has(previousOption.value))
	.concat(data)
	.sort(sortComparatorForNoSearch);
	return mergedData;
	});
	}
	return mergedData;
	},
	[sortComparatorForNoSearch],

superset/superset-frontend/src/components/Select/AsyncSelect.tsx

Lines 356 to 364 in 8c16806

	[
	allValuesLoaded,
	fetchOnlyOnSearch,
	mergeData,
	internalOnError,
	options,
	pageSize,
	value,
	],

superset/superset-frontend/src/components/Select/AsyncSelect.tsx

Lines 487 to 496 in 8c16806

	useEffect(() => {
	if (loadingEnabled && allowFetch) {
	// trigger fetch every time inputValue changes
	if (inputValue) {
	debouncedFetchPage(inputValue, 0);
	} else {
	fetchPage('', 0);
	}
	}
	}, [loadingEnabled, fetchPage, allowFetch, inputValue, debouncedFetchPage]);

2. I fixed the invalid checking for setAllValuesLoaded. (Since allValuesLoaded should check whether the current request is for the full list or not, it should check the search rather than value)

3. When removes the key in DatabaseSelector, its initial database selection has gone. (fix(sqllab): reverts #21141 #21174) The issue is caused by the missing currentDb mapping when db prop is updated. I added the useEffect logic to handle this issue.

superset/superset-frontend/src/components/DatabaseSelector/index.tsx

Lines 135 to 144 in b739e27

	const [currentDb, setCurrentDb] = useState<DatabaseValue | undefined>(
	db
	? {
	label: (
	<SelectLabel backend={db.backend} databaseName={db.database_name} />
	),
	value: db.id,
	...db,
	}
	: undefined,

5. Lastly I found the same useEffect logic on AsyncSelect so removed one

superset/superset-frontend/src/components/Select/AsyncSelect.tsx

Lines 473 to 477 in b739e27

	}, [initialOptions]);
	useEffect(() => {
	setSelectValue(value);
	}, [value]);

superset/superset-frontend/src/components/Select/AsyncSelect.tsx

Lines 513 to 517 in b739e27

	);
	useEffect(() => {
	setSelectValue(value);
	}, [value]);

[michael-s-molina]

LGTM. Thanks for the fixes @justinpark!