[Table list view] Add state in URL

[sebelga]

In this PR I've updated the <TableListView /> component to support storing the table state (search term, tag selection, column sorted and sorting direction "asc" or "desc").

This allow deep linking to a particular state of the table list view. This also allows using the back button of the browser and going back to the filtered table to open another content.

Note: Tests will come in a following PR.

Release note

The list view of the Dashboard, Visualize library, Maps and Graphs apps now store the latest state of the table in the URL to allow quickly go back to a filtered table.

Fixes #135203

[jloleysens]

Great work @sebelga ! Still reviewing, overall code LGTM. However when I tested locally in Dashboard

without these changes:

And with these changes

The "my dash" dashboard is not being filtered out which looks like a regression?

[jloleysens]

nit nit: if I read this correctly looks like we are instantiating this object on every render, is it possible to memoize it?

[sebelga]

Good point 👍 Done in c84425b

[sebelga]

Thanks for the review @jloleysens !
I can't reproduce the issue you found. I created a dashboard my dash then went back to the listing and I could filter it out by searching for Web

Full list

Searching for "Web"

Anything else I should do to reproduce the error? Cheers

[jloleysens]

Tested again locally, looks like I had not rebuilt my packages so ignore the issue!

Great work @sebelga !

[jloleysens]

Suggested change

	const updateQuerParams = useCallback(
	const updateQueryParams = useCallback(

[Dosant]

Great work!

I played a bit with the listing pages and I found a couple things that I don't think are expected:

1. As you type the search query, the query is update on each keystroke and each keystroke gets into the history stack. This leads to a very weird back button experience
2. Also I found that back button for sorting doesn't work for me in some case. For example - land on visualise, switch from recently updates to Name A-Z, click back - URL has changed back, but state on the page wasn't update (probably something with the initial state)

sort.back.bug.mov

---

I'd also suggest considering using history.replace. Not sure this really needs history.

[ThomThomson]

I respect this strategy if this is what your team has decided upon, but I personally feel quite strongly that we should avoid storing this sort of state in the URL for the following reasons:

• It exposes our internal state to user bookmarking, which means we need to be extremely careful of BWC moving forward.
• It transforms the back button into an implicit undo / redo system. The browser back and forward buttons are meant to be used for navigation between pages, not to change internal state in one page.
• It requires two-way syncing logic to ensure that the URL always shows the latest state, and that any change in the URL -even a manually written change - gets propagated to the component.
• It's difficult to control when the state gets reset, vs when the state comes with you. Any navigation that isn't the back button, will implicitly reset the state unless you carry the state with you through all intermediate steps.
• You can only ever have one URL. Theoretically, if you had two table list view components on one page, which one would sync with the URL?
• It's sometimes frustrating to the user to have a cluttered URL.

I would suggest an alternative method to get roughly the same behaviour:

• Write a use effect that backs up the state to session storage with a key like tableListView/dashboardListingPage.
• On component mount access that state and use it to override the defaults.
• Write a helper function that makes clearing this state easy. Think through the situations you'd want to lose the state - on app leave for instance - and explicitly clear the state in those situations.

With this implementation any navigation during the session including the back button, would restore the last state unless it was explicitly cleared.

The one caveat of this approach is that it doesn't support deep-linking. Is there a use case that requires deep-linking? If so, there are many ways to accomplish it in this paradigm which I can go into more detail on if required.

Again, I will approve this if the team is adamant that it's the right direction to go, but I believe that browser or session storage is a better approach.

[jloleysens]

@ThomThomson thanks for raising many valid points for consideration. Dashboard was certainly a case in point of why we should try to avoid app state in the URL!

I have a few general comments and refinements/counter points that I wanted to share. Happy to talk more sync!

I think there are kinds of state that are OK living in the URL. State that: (1) will not need to migrate often (fairly constant over time) (2) state that the application/plugin has full ownership of (3) relatively small in size.

A search term and sort state seem to be the kind of "innocuous" state that would be OK to store in the URL given what it enables and relatively simple implementation.

One complication with (2) in this case: this component is imported from a package, this means it is reaching into a global resource (the URL) and updating it. To make this "safer" we could lift the search term and sort state to the plugin/application and let them decide where to store it. But for these table list pages I wonder how useful that would be, might just make integration harder without much benefit.

FWIW, I am also happy if this were stored in sessionStorage and we used locators for deep linking, I just wanted to share thoughts on state+URL and how I was thinking about it in this case.

[sebelga]

Thanks for your input @ThomThomson and @jloleysens 👍

I agree that this deserve a sync meeting as it could impact many pages and workflows (hopefully in a good way! 😊 )

The goal of this PR is to have the state in the URL. Using the URL as the source of truth and being able to deeplink to a table state.

I think we all agree that the fact that Github gives us this URL:

https://github.com/elastic/kibana/issues?q=is%3Aopen+is%3Aissue+label%3A%22Team%3AGlobal+Experience%22

makes our life easier to send a list of issue for our team. It is not a "pretty" URL but a useful one.

The goal here is to reproduce this experience, allowing our users to filter the table list based on certain tags and send it over to other users and collaborate on their content.

Before going any further I also want to mention: keeping the table state in the URL is optional. It can be turned off with the urlStateEnabled prop. Setting it to false disables the behaviour and we are back to how it is currently in main.

This should answers your question on "what is the behaviour when there are multiple tables in one page": we would turn the url state off for both table.

It exposes our internal state to user bookmarking, which means we need to be extremely careful of BWC moving forward.

Indeed we need to be cautious. For that I'd expect to have solid test coverage like the one I am adding in #145618. Once those are in place we should have confidence that we will handle gracefully possible changes in contract (as I've done here to maintain support for the "title" query param from Dashboard)

It transforms the back button into an implicit undo / redo system. The browser back and forward buttons are meant to be used for navigation between pages, not to change internal state in one page.

I changed that behaviour by using history.replace instead of history.push. With that change, going back takes us to the previous page.

It requires two-way syncing logic to ensure that the URL always shows the latest state, and that any change in the URL -even a manually written change - gets propagated to the component.

Not sure I understand your concern. The way it is implemented is (1) update the URL (2) react to url changes. The URL is the source of truth, the app state is derived from it.

It's difficult to control when the state gets reset, vs when the state comes with you. Any navigation that isn't the back button, will implicitly reset the state unless you carry the state with you through all intermediate steps.

Actually I have bumped in the reverse problem. I wanted the state to be reset after navigating to a different app and then clicking on the app link (main menu) to go back I still had the state in the URL... I think there is some magic with the top level Kibana router that I am not aware of and that is currently a blocker. I'll need to investigate.

You can only ever have one URL. Theoretically, if you had two table list view components on one page, which one would sync with the URL?

Answered above. We would turn off the URL state in this case.

It's sometimes frustrating to the user to have a cluttered URL.

From my Github example above, I think the benefit outweights the "cluterness". I would even say the current _g query param on the list page is what makes it cluttered IMO.

[ThomThomson]

Especially with the feature temporarily disabled, Presentation team changes LGTM.

Thanks for all the discussion about this!

[ThomThomson]

Should this be replace now?

[Dosant]

Thanks @Dosant ! yes it seems that this could be it.

@ThomThomson I've disabled the feature by default and hoping to get a green CI. If you agree can you approve and merge the current work and we can re-enable it once we align on the behaviour (and fixes what Anton pointed out)?

Cheers 👍

👍
Let's make sure we've capture the bugs we've found

And I'll create a separate issue to discuss and agree on the kbnUrlTracker stuff. Hope will just agree we can remove it

[Dosant]

Thanks @Dosant ! yes it seems that this could be it.

@ThomThomson I've disabled the feature by default and hoping to get a green CI. If you agree can you approve and merge the current work and we can re-enable it once we align on the behaviour (and fixes what Anton pointed out)?

Cheers 👍

@sebelga, I created an issue to discuss getting rid of app link url changes #146318
Don't think this a very easy and fast to implement decision, so I pointed out a workaround for our case in the description

[sebelga]

so I pointed out a workaround for our case in the description

@Dosant Not sure I see the workaround in the issue description... 🤔

[Dosant]

so I pointed out a workaround for our case in the description

@Dosant Not sure I see the workaround in the issue description... 🤔

By default everything from the URL is preserved in the link. This leads to unwanted state restorations. This was a thing we hit when trying to enable URL syncing for the table list view #145517 (comment) . The workaround could be to manually exclude unwanted url parts like we did for the search session id:

kibana/src/plugins/discover/public/utils/initialize_kbn_url_tracking.ts

Lines 81 to 89 in 74e6282

	// Do not save SEARCH_SESSION_ID into nav link, because of possible edge cases
	// that could lead to session restoration failure.
	// see: https://github.com/elastic/kibana/issues/87149
	if (newNavLink.includes(SEARCH_SESSION_ID_QUERY_PARAM)) {
	newNavLink = replaceUrlHashQuery(newNavLink, (query) => {
	delete query[SEARCH_SESSION_ID_QUERY_PARAM];
	return query;
	});
	}

[sebelga]

Thanks @Dosant The whole thing seem hacky to me. But this workaround could be used to unblock this work. I still think we should remove automagic URL creation and rely on a different mechanism/UX to restore state.

[sebelga]

Update on the PR

• I've removed the state of the table list view from the kbn url tracker (156d5c6). This allows now to navigate back to the "default" listing page when navigating to Dashboards and Visualize library from the main menu 👍
• I've re-enabled by default the state in URL.
• In [Table list view] Component integration tests for URL state #145618 I will add all the necessary tests to make sure URL don't ever break.

@Dosant Can we retake the discussion in case you have any more concern on keeping the filter and sort state of the table in the URL?

[sebelga]

Sorry Anton, I meant to tag @ThomThomson on my above comment.

[ThomThomson]

Hey @sebelga, yes - I'm convinced that adding filter and sort state in the URL is the right choice here, as I don't see any major problems cropping up due to it! Thanks for checking in again.

[sebelga]

Great, thanks @ThomThomson 👍

[stratoula]

Visualizations changes LGTM, I tested it locally on the Visualize listing page and works fine

[kibana-ci]

💚 Build Succeeded

• Buildkite Build
• Commit: c6406c3

Metrics [docs]

Module Count

Fewer modules leads to a faster build time

id	before	after	diff
dashboard	352	353	+1
filesManagement	120	125	+5
graph	255	260	+5
maps	965	970	+5
visualizations	330	331	+1
total			+17

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
savedObjectsTaggingOss	50	52	+2

Async chunks

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

id	before	after	diff
dashboard	368.2KB	371.3KB	+3.1KB
filesManagement	85.7KB	93.9KB	+8.2KB
graph	452.7KB	461.0KB	+8.2KB
maps	2.7MB	2.7MB	+8.2KB
savedObjectsTagging	40.8KB	40.8KB	-1.0B
visualizations	267.7KB	270.8KB	+3.1KB
total			+30.9KB

Page load bundle

Size of the bundles that are downloaded on every page load. Target size is below 100kb

id	before	after	diff
dashboard	25.3KB	25.3KB	+48.0B
filesManagement	3.6KB	3.7KB	+58.0B
savedObjectsTagging	18.6KB	19.0KB	+347.0B
visualizations	56.5KB	56.6KB	+119.0B
total			+572.0B

Unknown metric groups

API count

id	before	after	diff
savedObjectsTaggingOss	98	100	+2

ESLint disabled in files

id	before	after	diff
osquery	1	2	+1

ESLint disabled line counts

id	before	after	diff
enterpriseSearch	19	21	+2
fleet	61	67	+6
osquery	109	115	+6
securitySolution	439	445	+6
total			+20

Total ESLint disabled count

id	before	after	diff
enterpriseSearch	20	22	+2
fleet	70	76	+6
osquery	110	117	+7
securitySolution	516	522	+6
total			+21

History

• 💚 Build #95433 succeeded f06b4be
• 💔 Build #95399 failed 156d5c6
• 💔 Build #95321 failed e5fcc86
• 💔 Build #95081 failed f43830f
• 💔 Build #89311 failed ecda8d9
• 💔 Build #89257 failed 45d288e

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

cc @sebelga

[sebelga]

Thanks for the review @stratoula! 👍