[TableListView] Fix regression when resetting search

[sebelga]

In #160650 we introduced a regression. When clearing a search request that didn't contain any result we were taking back to the empty prompt.

I reversed the changed in the PR to fix it and I added tests to prevent future regression for this.

@nreese Strangely enough I am not able to reproduce the issue you fixed after reverting your changes. I switched to "slow 3g" network as per your comment here #148557 (comment) but still could not reproduce the UI flashing issue.

I wonder if the fix needed from your PR was adding this useCallback (https://github.com/elastic/kibana/pull/160650/files#diff-7b0b3bede33bed7e2f5f7095697650d92c8ec131e29b9fccc384cacafad84689R84-R88)

[EDIT] I manage to reproduce. It only occurred when there are no items in the table.

Other fix

I also fixed the flaky test (#160178) that Nathan had fixed in his PR by making sure that once we detect that there no items we don't remove the empty prompt.

"Glitch" that has been fixed

Notice how we go from "No files found" (empty prompt) --> "Table" (no files match your search) --> "No files found".

Fixes #161288
Fixes #160178

[elasticmachine]

Pinging @elastic/appex-sharedux (Team:SharedUX)

[sebelga]

Sorry for the ping @Dosant and @nreese. I put the PR back to draft and fix CI first.

[nreese]

@nreese Strangely enough I am not able to reproduce the issue you fixed after reverting your changes. I switched to "slow 3g" network as per your comment here #148557 (comment) but still could not reproduce the UI flashing issue.

I am able to see flashing UI in this PR. To view, open visualize listing page when you have no visualization saved objects. In the empty case, the table should never be displayed.

Screen.Recording.2023-07-17.at.8.53.05.AM.mov

[nreese]

Would it be possible to remove fetchItems from useDebounce dependency check? And instead only call fetchItems when the search changes and not dependency chain.

The problem is that fetchItems is called twice when the page loads.

1. Once when page loads
2. A second time, because the first call to fetchItems dispatches onFetchItemsSuccess. This causes the parent component to pass in a new version of onFetchSuccess since calling onFetchSuccess changes state hasInitialFetchReturned

   const onFetchSuccess = useCallback(() => {
       if (!hasInitialFetchReturned) {
         setHasInitialFetchReturned(true);
       }
     }, [hasInitialFetchReturned]);
   

[sebelga]

I am able to see flashing UI in this PR.

Can you retest now. I fixed it here 2a6f300

Would it be possible to remove fetchItems from useDebounce dependency check?

No because that would open the door for other future regression. We want to fetch the items any time any of the dependency change.

const onFetchSuccess = useCallback(() => {
    if (!hasInitialFetchReturned) {
      setHasInitialFetchReturned(true);
    }
  }, [hasInitialFetchReturned]);

That was a mistake from Andrew. The function that update the state has the state as a dependency. Usually a source of infinite re-render. I changed it here 07a237a

[nreese]

Why are you defaulting to isFetchingItems is true?

[sebelga]

That's the state when he table mount: it fetches the items.

We initially had an useEffect + useDebounce. You correctly removed the effect (#159507) so we only have one of the 2 but we need to wait 300ms before having this state set to true.

[nreese]

LGTM
code review, tested in chrome

[stratoula]

Thanx @sebelga

The only bug I found is also depicted on the failing graph test.
When I have some items in the listing page and I delete them all, I expect to see the empty listing page. On the contrary I see the No {SO type} matched your search. screen.

[sebelga]

Thanks for the review @nreese and @stratoula! 👍

The only bug I found is also depicted on the failing graph test.

Yes indeed, good catch. I fixed it in 4d57f17

[stratoula]

This looks fantastic! Thanx @sebelga ❤️

Tested different scenarios and everything works as expected 🥇

[Dosant]

tested, works well

[kibana-ci]

💚 Build Succeeded

• Buildkite Build
• Commit: 6053fa5

Metrics [docs]

Public APIs missing comments

Total count of every public API that lacks a comment. Target amount is 0. Run node scripts/build_api_docs --plugin [yourplugin] --stats comments for more detailed information.

id	before	after	diff
@kbn/content-management-table-list-view-table	41	40	-1

Async chunks

Total size of all lazy-loaded chunks that will be downloaded as the user navigates the app

id	before	after	diff
dashboard	355.6KB	355.7KB	+50.0B
eventAnnotation	140.4KB	140.5KB	+58.0B
filesManagement	89.2KB	89.2KB	+51.0B
graph	458.3KB	458.4KB	+51.0B
maps	2.8MB	2.8MB	+51.0B
visualizations	262.4KB	262.5KB	+84.0B
total			+345.0B

History

• 💔 Build #143124 failed 4d57f17
• 💔 Build #142932 failed 07a237a
• 💔 Build #142839 failed 2d6647a03b1da5ea749d6ed70dbd32711649f50a

To update your PR or re-run it, just comment with:
@elasticmachine merge upstream

cc @sebelga

[drewdaemon]

Does it make sense to backport this for 8.9?

[nreese]

Does it make sense to backport this for 8.9?

I would vote yes, since #160650 is in 8.9.0

[sebelga]

I would vote yes, since #160650 is in 8.9.0

Is there a label we can add now or do I need to use the script?

[stratoula]

You need to use the script unfortunately afaik

[drewdaemon]

For the record, @pheyos mentioned that adding the backport:last-minor label works even after merge. It didn't seem to work for me, but has worked for others.

Whatever gets the job done 👍

[sebelga]

💚 All backports created successfully

Status	Branch	Result
✅	8.9

Note: Successful backport PRs will be merged automatically after passing CI.

Questions ?

Please refer to the Backport tool documentation

[kibanamachine]

💚 All backports created successfully

Status	Branch	Result
✅	8.9

Note: Successful backport PRs will be merged automatically after passing CI.

Questions ?

Please refer to the Backport tool documentation