feat: create new browser history entry on some user actions #128

ifrost · 2024-08-23T11:08:12Z

✨ Description

Related issue(s):

Fixes #121

📖 Summary of the changes

Adds basic support for browser history. A new history is created upon user actions:

Changing exploration type
Changing profile type
Changing service name
Changing group by in labels exploration

Here's how it works:

browser-history.mp4

Technical details

The approach is to make sure we add new user actions to the history stack. Just before the user action handler is called we create a new empty entry in the stack and then UrlSyncManager takes care of replacing that history with any updates to variables. This will include multiple updates, and chain of updates. Any changes to URL that are not coming from the user action will be automatically added to the current history.

Alternative approaches:

Push history change on each variable change or every time the URL changes.

That can work fine only when there's one variable updated upon user action. When there are multiple updates, they need to be somehow batched into single entry and it's tricky because the update is asynchronous. It's also visible during loading initial state as it usually requires multiple vars to be updated.

Use drilldowns

That works fine but requires some boilerplate just to support it in our case (SceneApp and SceneAppPage). It works great when it's used with native tabs coming with scenes but we don't use it for exploration type. Hence, to support drilldowns it would require even more boilerplate to create correct URLs for each exploration type (basically duplicating something that scenes handles internally with preserverUrlKeys

Change the variable via URL directly

Instead of calling variable.changeValueTo we can change the value directly by modifying the URL. Dashboards do it to handle history (example). This way there's no need to depend on location.partial happening after pushing a new entry (like in this PR). The drawback might be that if a variable name is not unique it's hard to determine it's name as it's mapped by UrlSyncManager/UniqueUrlKeyMapper.

🧪 How to test?

It requires manual testing. Please try changing elements mentioned above (and others) and see if back/forward buttons behave as expected.

ifrost · 2024-08-23T11:08:48Z

src/pages/ProfilesExplorerView/components/SceneProfilesExplorer/SceneProfilesExplorer.tsx

    if (typeof values.explorationType === 'string' && values.explorationType !== this.state.explorationType) {
-      stateUpdate.explorationType = values.explorationType as ExplorationType;
+      const type = values.explorationType as ExplorationType;
+      this.setExplorationType({ type });


Switching to the getter to get the body updated correctly when going back in history

👍🏾 The only drawback is that the same body is built twice - only when landing on the page for the 1st time, if there's an explorationType parameter in the URL.

On one side, we should prevent it in onActivate:

if (!this.state.explorationType) { this.setExplorationType({ type: SceneProfilesExplorer.DEFAULT_EXPLORATION_TYPE, }); }

And on the other side, updateFromUrl should become:

updateFromUrl(values: SceneObjectUrlValues) { if (typeof values.explorationType === 'string' && values.explorationType !== this.state.explorationType) { const type = values.explorationType as ExplorationType; this.setExplorationType({ type: Object.values(ExplorationType).includes(type) ? type : SceneProfilesExplorer.DEFAULT_EXPLORATION_TYPE, }); } }

Thanks! Should be fixed by c66235c

github-actions · 2024-08-23T11:09:25Z

Unit test coverage

Lines	Statements	Branches	Functions
	10.63% (476/4477)	8.24% (136/1650)	7.99% (109/1363)

ifrost · 2024-08-23T11:09:24Z

src/pages/ProfilesExplorerView/domain/variables/ProfileMetricVariable.tsx

@@ -103,6 +104,9 @@ export class ProfileMetricVariable extends QueryVariable {
  onSelect = (newValue: string) => {
    reportInteraction('g_pyroscope_app_profile_metric_selected');

+    if (!this.state.skipUrlSync) {


Checking skipUrlSync because we use this variable with skipping in other place

another variant would be

if (this.state.skipUrlSync) { this.changeValueTo(newValue); } else { locationService.partial({ [`var-{this.state.name}`]: newValue }) }

need to URL encode the value there.

doing it this way would be compatible (not conflict) with any built-in browser history history support in UrlSyncManager / interfaces

ifrost · 2024-08-23T11:10:11Z

src/shared/domain/url-params/useUrlSearchParams.ts

@@ -14,7 +14,7 @@ export function useUrlSearchParams() {
        newSearchParams.set(key, value);
      }

-      history.push({ search: newSearchParams.toString() });
+      history.replace({ search: newSearchParams.toString() });


Switching to replace to avoid creating redundant history items

ifrost · 2024-08-23T11:19:22Z

cc @torkelo @dprokop, would be great to get your feedback on this approach as well to check if we're not setting ourselves for failure and if there's a better approach.

torkelo · 2024-08-23T12:22:17Z

src/pages/ProfilesExplorerView/components/SceneProfilesExplorer/SceneProfilesExplorer.tsx

@@ -225,6 +223,7 @@ export class SceneProfilesExplorer extends SceneObjectBase<SceneProfilesExplorer
    item?: GridItemData;
  }) {
    if (comesFromUserAction) {
+      prepareHistoryEntry();


could this not just do a locationService.partial as well (you would get history step and update state in one action)

this is what we do for most URL synced "view" state in core dashboards (update the URL via normal link or locationService)

You mean to update the URL instead of changing the state and let the updateFromUrl handle the update? 🤔 Does it mean you need to know the final name of the variable when doing locationService.partial()? How do you handle it if variable name may be different after processing with urlKeyMapper.getUniqueKey in UrlSyncManager?

true, you would need t know the real URL key (so would only work for things where you know this).

Was just an idea to automatically get a history step for a user action

been making progress on grafana/scenes#760 , can you check it out?

torkelo · 2024-08-23T12:26:28Z

src/pages/ProfilesExplorerView/domain/variables/ProfileMetricVariable.tsx

@@ -103,6 +104,9 @@ export class ProfileMetricVariable extends QueryVariable {
  onSelect = (newValue: string) => {
    reportInteraction('g_pyroscope_app_profile_metric_selected');

+    if (!this.state.skipUrlSync) {


another variant would be

if (this.state.skipUrlSync) { this.changeValueTo(newValue); } else { locationService.partial({ [`var-{this.state.name}`]: newValue }) }

need to URL encode the value there.

doing it this way would be compatible (not conflict) with any built-in browser history history support in UrlSyncManager / interfaces

grafakus · 2024-08-26T08:22:32Z

Thank you for the PR! It works as expected except in two cases:

In "Labels": after clicking on the "Select" panel action:

We should also call prepareHistory when changing the groupBy variable value

In "Labels" and "Flame graph", when there are both the "Service" and the "Profile type" dropdowns, here's a short video:
https://github.com/user-attachments/assets/e18ac321-0d42-4a11-8b36-62a7cffeb338

ifrost · 2024-08-26T20:36:19Z

Thanks @grafakus 🙌

In "Labels": after clicking on the "Select" panel action:
We should also call prepareHistory when changing the groupBy variable value

Great catch! I've added prepping history directly toselectLabel. It's easy to make errors like this, though and forgot that variable may be change in other place, though :( I wonder if there's some other pattern that could ensure all user actions go via the same route (e.g. I noticed that select label has no user tracking, should we add it?)

Should be fixed by 584db4b

In "Labels" and "Flame graph", when there are both the "Service" and the "Profile type" dropdowns.

Another nice catch! Turns out when variables are updated from the URL both loading and value property are updated straight away and options are loaded with a delay. However, once options are loaded we do not re-render the Cascader (it's forced only when value and loading prop change). It all feels a bit hacky but seems like it's all because of the way Cascader works. There's no way to control it and pass new options and value without forcing a re-render.

Changed in e55d05e

Alternatively we can also revert adding a new history for service and profile for now and think how to unhack Cascader first 🤔

grafakus · 2024-08-28T08:47:13Z

@ifrost thank you for making the changes!

I noticed that select label has no user tracking, should we add it?

The tracking is centralized in SelectAction

Alternatively we can also revert adding a new history for service and profile for now and think how to unhack Cascader first 🤔

I suppose what's lacking is to be able to control the Cascader value... In the meantime, if we end up creating a key by concatenating several values and stringifying options, I'd rather go with a simpler hack to force a render each time:

    return (
      <Cascader
        // TODO: ...
        key={Math.random()}
...

It shouldn't be a big perf issue, wdyt?

ifrost · 2024-08-28T15:00:37Z

It shouldn't be a big perf issue, wdyt?

It should be fine. We may get some additional renders (2 or 3) on first paint but shouldn't have a big impact. Essentially we need to re-render Cascader when value, loading state or options change which is anyway is the only props that cause the re-render of the parent component (and hence Cascader too) so it should have almost identical outcome. Updated in c05ef57

grafakus · 2024-08-29T13:33:26Z

Thank you for helping on this @ifrost!

Create new history item on user actions

cd6cbdf

github-actions bot added the feature label Aug 23, 2024

ifrost commented Aug 23, 2024

View reviewed changes

ifrost marked this pull request as ready for review August 23, 2024 11:17

ifrost requested a review from grafakus August 23, 2024 11:19

torkelo reviewed Aug 23, 2024

View reviewed changes

ifrost added 3 commits August 26, 2024 12:48

Merge branch 'refs/heads/main' into ifrost/support-browser-history

2983531

Create new history item when "select" button is clicked

584db4b

Force re-render of Cascader when options change

e55d05e

Force re-render of Cascader when options change

c05ef57

ifrost added 2 commits August 29, 2024 10:42

Merge branch 'main' into ifrost/support-browser-history

0101f57

Ensure the body is built once

c66235c

grafakus approved these changes Aug 29, 2024

View reviewed changes

grafakus merged commit 5439ab3 into main Aug 29, 2024
5 checks passed

grafakus deleted the ifrost/support-browser-history branch August 29, 2024 13:35

grafakus mentioned this pull request Dec 16, 2024

Add a pattern to let users navigate back grafana/explore-traces#34

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat: create new browser history entry on some user actions #128

feat: create new browser history entry on some user actions #128

ifrost commented Aug 23, 2024 •

edited

Loading

ifrost Aug 23, 2024

grafakus Aug 26, 2024

grafakus Aug 28, 2024

ifrost Aug 29, 2024

github-actions bot commented Aug 23, 2024 •

edited

Loading

ifrost Aug 23, 2024

torkelo Aug 23, 2024

ifrost Aug 23, 2024

ifrost commented Aug 23, 2024

torkelo Aug 23, 2024

torkelo Aug 23, 2024 •

edited

Loading

ifrost Aug 23, 2024

torkelo Aug 27, 2024

torkelo Aug 23, 2024

grafakus commented Aug 26, 2024 •

edited

Loading

ifrost commented Aug 26, 2024

grafakus commented Aug 28, 2024 •

edited

Loading

ifrost commented Aug 28, 2024

grafakus commented Aug 29, 2024

feat: create new browser history entry on some user actions #128

feat: create new browser history entry on some user actions #128

Conversation

ifrost commented Aug 23, 2024 • edited Loading

✨ Description

📖 Summary of the changes

Technical details

🧪 How to test?

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

github-actions bot commented Aug 23, 2024 • edited Loading

Unit test coverage

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ifrost commented Aug 23, 2024

Choose a reason for hiding this comment

torkelo Aug 23, 2024 • 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

grafakus commented Aug 26, 2024 • edited Loading

ifrost commented Aug 26, 2024

grafakus commented Aug 28, 2024 • edited Loading

ifrost commented Aug 28, 2024

grafakus commented Aug 29, 2024

ifrost commented Aug 23, 2024 •

edited

Loading

github-actions bot commented Aug 23, 2024 •

edited

Loading

torkelo Aug 23, 2024 •

edited

Loading

grafakus commented Aug 26, 2024 •

edited

Loading

grafakus commented Aug 28, 2024 •

edited

Loading