[On-week] Add persisting page size and sorting for Management tables

[ElenaStoeva]

Addresses #56406

Summary

This PR is part of my June 2024 On-week project. It adds functionality for persisting table page size (rows per page) and sorting in EUI tables. When a user changes the size or sort, the new values are saved in local storage, so that when the user navigates out of the page and comes back, the page size and sort will be the same.

In this PR, the functionality is added to the following tables:

• Ingest Pipelines
• Index Management - all tables (indices, data streams, index templates, component templates, enrich policies)
• ILM policies

Screen.Recording.2024-06-28.at.16.05.48.mov

[ElenaStoeva]

/ci

[ElenaStoeva]

/ci

[ElenaStoeva]

/ci

[ElenaStoeva]

/ci

[ElenaStoeva]

/ci

[ElenaStoeva]

/ci

[elasticmachine]

Pinging @elastic/kibana-management (Team:Kibana Management)

[elasticmachine]

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

[yuliacech]

That is a great improvement to the tables, @ElenaStoeva!
I verified locally that the page size is persisted between page loads. Code changes LGTM, I would just recommend not naming the function a hook if it doesn't use any react hooks. Also I think some unit tests for the useEuiTablePersist could be useful as well.

[sebelga]

Great work @ElenaStoeva ! I left a few comments regarding code style and readability.
As @yuliacech mentioned currently it is not a hook and I think we could return at least the onTableChange in a useCallback and probably make the 2 other value a state so those are reactive values for consumers.

[sebelga]

It is not easy to parse the double ternary, could we make it more explicit like the below suggestion? Also I would keep the "next" prefix everywhere instead of "new" and "next"

let nextSort: Criteria<T>['sort'];
if (nextValues.sort?.field) {
  nextSort = nextValues.sort;
} else if (!nextValues.sort?.direction) {
  // If both field and direction are undefined, there is no change on sort so use the stored one
  nextSort = storageSort;
}

const nextPageSize = nextValues.page?.size ?? storagePageSize;

const nextPersistData: PersistData<T> = {
  pageSize: nextPageSize,
  sort: nextSort,
};

if (nextPersistData.pageSize !== storagePageSize || nextPersistData.sort !== storageSort) {
  storage.set(tableId, nextPersistData);
}

[ElenaStoeva]

Sure, I updated it as suggested, with some modifications due to some recent changes.

On a side note, I just want to note that with the current logic, if the consumer provides an initial sort, when the user removes a sort (the change criteria would contain just the direction property, with undefined field), then we store undefined sort in local storage, so next time when the user visits this page, the initial sort will be restored (rather than no sort to be preserved). Just wanted to make sure this is the desired behaviour.

[ElenaStoeva]

Many thanks for the thorough reviews, @sebelga and @yuliacech!
I addressed your comments and added some unit tests for the (now already) hook. 😅 Please let me know if I missed anything or if you have further suggestions.

[sebelga]

Looking good! I left one more comment to avoid having the state as a dependency of a callback setting the state.
I'll approve to unblock the PR but it'd be great to apply my suggestion 🙏 Thanks!

[sebelga]

In a callback that it setting the state (setPageSize, setSort), it is usually better to avoid having the state being set as a dependency. It could create an infinite loop.
Here I checked and there are no infinite loop but I think we could avoid it this way

const onTableChange = useCallback(
  (nextValues: Criteria<T>) => {
    if (customOnTableChange) {
      customOnTableChange(nextValues);
    }

    let nextSort: PropertySort<T> | undefined;
    if (nextValues.sort?.field && nextValues.sort?.direction) {
      // Both field and direction are needed for a valid sort criteria
      nextSort = nextValues.sort;
    }

    if (nextValues.sort?.field || nextValues.sort?.direction) {
      setSort(nextSort);
    }

    const nextPageSize = nextValues.page?.size;
    if (nextPageSize) {
      setPageSize(nextPageSize);
    }

    if (
      (nextPageSize && nextPageSize !== storagePageSize) ||
      (nextSort && nextSort !== storageSort)
    ) {
      const nextPersistData: PersistData<T> = {
        pageSize: nextPageSize,
        sort: nextSort,
      };
      storage.set(tableId, nextPersistData);
    }
  },
  [customOnTableChange, storage, storagePageSize, storageSort, tableId]
);

[ElenaStoeva]

Good call! I added your suggestion with a small modification to ensure that sort would be cleaned up in local storage if the user removes the sort on a specific field (when this happens, field is an empty string). 7754207

[kibana-ci]

💛 Build succeeded, but was flaky

• Buildkite Build
• Commit: 6948a64

Failed CI Steps

• FTR Configs #96

Test Failures

• [job] [logs] FTR Configs #96 / Visualizations - Group 3 lens app - TSVB Open in Lens Dashboard to TSVB to Lens should convert a by reference TSVB viz to a Lens viz

Metrics [docs]

Module Count

Fewer modules leads to a faster build time

id	before	after	diff
indexLifecycleManagement	223	229	+6
indexManagement	680	687	+7
ingestPipelines	321	327	+6
total			+19

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/shared-ux-table-persist	-	2	+2
indexManagement	222	233	+11
total			+13

Async chunks

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

id	before	after	diff
indexLifecycleManagement	152.0KB	153.6KB	+1.7KB
indexManagement	676.9KB	679.3KB	+2.4KB
ingestPipelines	373.5KB	375.1KB	+1.7KB
total			+5.7KB

Public APIs missing exports

Total count of every type that is part of your API that should be exported but is not. This will cause broken links in the API documentation system. Target amount is 0. Run node scripts/build_api_docs --plugin [yourplugin] --stats exports for more detailed information.

id	before	after	diff
@kbn/shared-ux-table-persist	-	2	+2

Unknown metric groups

API count

id	before	after	diff
@kbn/shared-ux-table-persist	-	3	+3
indexManagement	227	238	+11
total			+14

History

• 💚 Build #228594 succeeded 2aeb917
• 💔 Build #227853 failed 4a22c21
• 💔 Build #227851 failed a2aa291
• 💔 Build #227655 failed 096abab
• 💛 Build #226621 was flaky ada6dd7
• 💔 Build #226496 failed d6c8a52

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

cc @ElenaStoeva