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

• [x] 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[bot]]

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%)	:arrow_up:

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%)	:arrow_down:
...set-frontend/src/components/Select/AsyncSelect.tsx	87.97% <100.00%> (+3.36%)	:arrow_up:

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

[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.

https://github.com/apache/superset/blob/8c16806f5759ecc53ecef88a2e96e2e0964bffc6/superset-frontend/src/components/Select/AsyncSelect.tsx#L196-L200

https://github.com/apache/superset/blob/8c16806f5759ecc53ecef88a2e96e2e0964bffc6/superset-frontend/src/components/Select/AsyncSelect.tsx#L214-L223

https://github.com/apache/superset/blob/8c16806f5759ecc53ecef88a2e96e2e0964bffc6/superset-frontend/src/components/Select/AsyncSelect.tsx#L302-L319

https://github.com/apache/superset/blob/8c16806f5759ecc53ecef88a2e96e2e0964bffc6/superset-frontend/src/components/Select/AsyncSelect.tsx#L356-L364

https://github.com/apache/superset/blob/8c16806f5759ecc53ecef88a2e96e2e0964bffc6/superset-frontend/src/components/Select/AsyncSelect.tsx#L487-L496

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. (#21174) The issue is caused by the missing currentDb mapping when db prop is updated. I added the useEffect logic to handle this issue.

https://github.com/apache/superset/blob/b739e27f6dc4b159d766074e3e353a5546d00adb/superset-frontend/src/components/DatabaseSelector/index.tsx#L135-L144

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

https://github.com/apache/superset/blob/b739e27f6dc4b159d766074e3e353a5546d00adb/superset-frontend/src/components/Select/AsyncSelect.tsx#L473-L477

https://github.com/apache/superset/blob/b739e27f6dc4b159d766074e3e353a5546d00adb/superset-frontend/src/components/Select/AsyncSelect.tsx#L513-L517