[Infra UI] Store asset details page state in the URL state #164601

mykolaharmash · 2023-08-23T14:59:40Z

Summary

This PR moves the responsibility of updating the URL state into the <AssetDetails> instead of providing callbacks for parent components to manage the state themselves. This way logic for updating the URL state is centralized and shared between the Host details flyout and the Host details page.

How to test

Run from the branch locally
Navigate to "Hosts" screen
Open the host details flyout
Switch between tabs and refresh the page
Make sure your last active tab stays active after the refresh
Type into search fields on the "Metadata", "Processes" or "Logs" tab and refresh the page
Make sure the search term persists
Change the date range using the date picker and refresh the page
Make sure that the date range persists
All of the above should work the same for the full-page host details view

x-pack/plugins/infra/public/pages/metrics/metric_detail/asset_detail_page.tsx

elasticmachine · 2023-08-28T10:37:51Z

Pinging @elastic/infra-monitoring-ui (Team:Infra Monitoring UI)

mykolaharmash · 2023-08-28T11:45:20Z

@elasticmachine merge upstream

mykolaharmash · 2023-08-28T11:47:29Z

/oblt-deploy

crespocarlos

Great job Mycola 👏!

I quickly tested it and everything seems to be working. Just a few comments and one adjustments in the use_asset_details_url_state regarding the itemId attribute.

x-pack/plugins/infra/public/pages/metrics/metric_detail/asset_detail_page.tsx

crespocarlos · 2023-08-28T12:11:34Z

x-pack/plugins/infra/public/components/asset_details/hooks/use_asset_details_url_state.ts

@@ -53,10 +53,10 @@ const FlyoutTabIdRT = rt.union([
  rt.literal(FlyoutTabIds.OSQUERY),
 ]);

-const HostFlyoutStateRT = rt.intersection([
+const AssetDetailsStateRT = rt.intersection([
  rt.type({
    itemId: rt.string,


The itemId is only used in the use_hosts_table. It can be moved over to the use_hosts_table state. With that, the asset_details_url_state will contain only attributes relevant to the Asset Details component. Wdyt?

Good point, will move it to the useHostsTableUrlState

crespocarlos · 2023-08-28T12:11:59Z

x-pack/plugins/infra/public/components/asset_details/types.ts

@@ -28,21 +28,14 @@ export type TabIds = `${FlyoutTabIds}`;

 export interface OverridableTabState {


crespocarlos · 2023-08-28T12:14:57Z

x-pack/plugins/infra/public/pages/metrics/hosts/hooks/use_hosts_table.tsx

@@ -140,7 +140,7 @@ export const useHostsTable = () => {
  } = useKibanaContextForPlugin();
  const { dataView } = useMetricsDataViewContext();

-  const [hostFlyoutState, setHostFlyoutState] = useHostFlyoutUrlState();
+  const [hostFlyoutState, setHostFlyoutState] = useAssetDetailsUrlState();


We're only using the useAssetDetailsUrlState to get the itemId and control the flyout's visibility. It seems like this attribute should be owned by the use_hosts_table.

The URL state I'm thinking of is the useHostsTableUrlState. Looks like itemId could be moved to that hook .

x-pack/plugins/infra/public/pages/metrics/metric_detail/asset_detail_page.tsx

crespocarlos · 2023-08-28T12:19:43Z

x-pack/test/functional/apps/infra/hosts_view.ts

+     * running effects (like updating the URL) before
+     * refreshing the page.
+     */
+    await pageObjects.common.sleep(1000);


Q: Have you encountered any problems due to a delay in updating the state before refreshing the page?

Yeah, unfortunately sometimes the browser starts to refresh before URL is changed. I assume it's because we don't write to the URL directly but rather use router hooks, making URL updates basically part of the react rendering cycle. Which is convenient for business logic of course, but for these kinds of tests I could not find a better solution other than to introduce an artificial delay, unfortunately 😔

crespocarlos · 2023-08-28T12:20:37Z

x-pack/test/functional/apps/infra/hosts_view.ts

+            await searchInput.type('test');
+            await refreshPageWithDelay();
+
+            expect(await searchInput.getAttribute('value')).to.be('test');


Asserts after page refresh are tricky and often cause flakiness.

The "safest" approach would be to wrap the expect in a retry.try function.

retry.try(async () => { expect(await searchInput.getAttribute('value')).to.be('test'); })

Good to know, thanks!

crespocarlos · 2023-08-28T12:25:15Z

x-pack/test/functional/apps/infra/hosts_view.ts

@@ -284,6 +294,16 @@ export default ({ getPageObjects, getService }: FtrProviderContext) => {
          });
        });

+        it('preserves selected tab between page reloads', async () => {


It might be better to centralize these "preserves..." test scenarios in the node_details test suite. The host_view suite would become less focused on the asset details functionality, while the node_details could focus on more in-depth test cases. Wdyt?

crespocarlos · 2023-08-28T12:47:15Z

x-pack/plugins/infra/public/components/asset_details/hooks/use_asset_details_state.ts

-    AssetDetailsProps,
-    'asset' | 'assetType' | 'overrides' | 'onTabsStateChange' | 'renderMode'
-  >;
+  state: Pick<AssetDetailsProps, 'asset' | 'assetType' | 'overrides' | 'renderMode'>;
 }

 export function useAssetDetailsState({ state }: UseAssetDetailsStateProps) {


I'm under the impression that the names here have become confusing with the introduction of the use_asset_details_url_state. I wonder if it should be called something along the lines of useAssetDetailsRenderInfo.

makes sense, will rename it 👍

jennypavlova

Well done! Checked it and it works fine 🙌 Added some small comments (nothing blocking the PR)

x-pack/plugins/infra/public/components/asset_details/hooks/use_date_range.ts

x-pack/test/functional/apps/infra/node_details.ts

x-pack/test/functional/apps/infra/hosts_view.ts

mykolaharmash · 2023-08-29T09:27:00Z

x-pack/plugins/infra/public/components/asset_details/context_providers.tsx

@@ -6,7 +6,7 @@
 */

 import React from 'react';
-import { AssetDetailsStateProvider } from './hooks/use_asset_details_state';
+import { AssetDetailsRenderPropsProvider } from './hooks/use_asset_details_render_props';


Settled on AssetDetailsRenderProps name, seemed like it better conveys the purpose, but let me know if that does not look right 🙌

kibana-ci · 2023-08-29T10:28:04Z

💚 Build Succeeded

Buildkite Build
Commit: 8da5194

Metrics [docs]

Async chunks

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

id	before	after	diff
`infra`	2.0MB	2.0MB	-780.0B

History

💚 Build #154058 succeeded 8928efe
💛 Build #154029 was flaky a526204
💔 Build #153993 failed 4c44838aad0420f7df8331c67b5edae1e1be3cf1
💔 Build #153586 failed 072f5b6560db0c067a4bca26544291fe642b98be
💚 Build #153047 succeeded 95123a16ac36c9f814b4dd286ebdad2a63edb542
💔 Build #152820 failed 844d428c86003cbd38842bab675514fd547ec2fa

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

crespocarlos

LGTM 🚀 ! Thanks for applying the suggestions.

mykolaharmash force-pushed the 164147-asset-details-url-state branch from fc75cb3 to 844d428 Compare August 23, 2023 15:03

mykolaharmash force-pushed the 164147-asset-details-url-state branch 2 times, most recently from 072f5b6 to 4c44838 Compare August 28, 2023 08:55

Save assets details state to the URL

a526204

mykolaharmash force-pushed the 164147-asset-details-url-state branch from 4c44838 to a526204 Compare August 28, 2023 10:24

mykolaharmash commented Aug 28, 2023

View reviewed changes

x-pack/plugins/infra/public/pages/metrics/metric_detail/asset_detail_page.tsx Show resolved Hide resolved

mykolaharmash marked this pull request as ready for review August 28, 2023 10:37

mykolaharmash requested a review from a team as a code owner August 28, 2023 10:37

jennypavlova self-requested a review August 28, 2023 10:49

Merge branch 'main' into 164147-asset-details-url-state

8928efe

crespocarlos reviewed Aug 28, 2023

View reviewed changes

crespocarlos changed the title ~~Store asset details page state in the URL state~~ [Infra UI] Store asset details page state in the URL state Aug 28, 2023

jennypavlova approved these changes Aug 28, 2023

View reviewed changes

x-pack/plugins/infra/public/components/asset_details/hooks/use_date_range.ts Outdated Show resolved Hide resolved

x-pack/test/functional/apps/infra/node_details.ts Outdated Show resolved Hide resolved

x-pack/test/functional/apps/infra/hosts_view.ts Outdated Show resolved Hide resolved

Move itemId into the hosts table URL state + other PR comments

8da5194

mykolaharmash commented Aug 29, 2023

View reviewed changes

mykolaharmash requested a review from crespocarlos August 29, 2023 09:28

crespocarlos approved these changes Aug 29, 2023

View reviewed changes

mykolaharmash merged commit fc6034a into elastic:main Aug 29, 2023

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[Infra UI] Store asset details page state in the URL state #164601

[Infra UI] Store asset details page state in the URL state #164601

mykolaharmash commented Aug 23, 2023 •

edited

Loading

elasticmachine commented Aug 28, 2023

mykolaharmash commented Aug 28, 2023

mykolaharmash commented Aug 28, 2023

crespocarlos left a comment

crespocarlos Aug 28, 2023

mykolaharmash Aug 28, 2023

crespocarlos Aug 28, 2023

crespocarlos Aug 28, 2023

crespocarlos Aug 28, 2023

mykolaharmash Aug 28, 2023

crespocarlos Aug 28, 2023 •

edited

Loading

mykolaharmash Aug 28, 2023

crespocarlos Aug 28, 2023

mykolaharmash Aug 28, 2023

crespocarlos Aug 28, 2023

mykolaharmash Aug 28, 2023

jennypavlova left a comment

mykolaharmash Aug 29, 2023

crespocarlos Aug 29, 2023

kibana-ci commented Aug 29, 2023

crespocarlos left a comment

		@@ -28,21 +28,14 @@ export type TabIds = `${FlyoutTabIds}`;

		export interface OverridableTabState {

[Infra UI] Store asset details page state in the URL state #164601

[Infra UI] Store asset details page state in the URL state #164601

Conversation

mykolaharmash commented Aug 23, 2023 • edited Loading

Summary

How to test

elasticmachine commented Aug 28, 2023

mykolaharmash commented Aug 28, 2023

mykolaharmash commented Aug 28, 2023

crespocarlos left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

crespocarlos Aug 28, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jennypavlova left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

kibana-ci commented Aug 29, 2023

💚 Build Succeeded

Metrics [docs]

Async chunks

History

crespocarlos left a comment

Choose a reason for hiding this comment

mykolaharmash commented Aug 23, 2023 •

edited

Loading

crespocarlos Aug 28, 2023 •

edited

Loading