fix: session storage retrieval for url messes with the url history #6238

AdityaHegde · 2024-12-09T13:11:50Z

Since we were redirecting url when visiting the home page. Any saved state would lead to a redirect loop when back button was pressed. Using history.replaceState instead to update the url to fix this.
Switching between dashboards using the breadcrumbs would pollute the other dashboard's state. Using exploreName from loader function to keep things consistent.
Disable TTD for dashboards without timeseries. Earlier the button was clickable but nothing would happen. Now there will not be a button at all.
Figure out the E2E failure
Load metrics view schema for bookmarks.

ericpgreen2 · 2024-12-10T19:20:06Z

I added a filter, pressed the browser's back button, the filter was removed from the URL, but the filter was not actually removed from the dashboard.

ericpgreen2 · 2024-12-10T19:27:43Z

web-common/src/features/explores/selectors.ts

@@ -86,6 +77,7 @@ export function useExploreValidSpec(
 export async function fetchExploreSpec(


This is confusing as the function does not actually return an explore spec (or validSpec). A better name might be fetchExploreResourceAndState, but then it's clear that this function is doing more than one thing. While a better name would be an improvement, consider if this function should really be split in two.

web-common/src/features/dashboards/stores/persistent-dashboard-state.ts

ericpgreen2 · 2024-12-10T19:52:00Z

web-common/src/features/dashboards/url-state/DashboardURLStateSync.svelte

  export let defaultExplorePreset: V1ExplorePreset;
+  export let initExploreState: Partial<MetricsExplorerEntity>;
+  export let initUrlSearch: string;
  export let partialExploreState: Partial<MetricsExplorerEntity>;
+  export let urlSearchForPartial: string;


This confuses me that we're passing state from a subset of sources (defaultExplorePreset, initUrlSearch) and passing initExploreState (which should be a synthesis of the all state sources). We should either:

Pass all state sources (e.g. including localStorageExploreState) and not pass initExploreState (which would be computed downstream).

Pass no individual state sources (e.g. defaultExplorePreset, initUrlSearch) and instead pass initExploreState (which would be computed somewhere upstream).

Given this component seems to be in charge of state synthesis, we should do option 1.

Passing down all states to DashboardURLStateSync and handling the merging logic there.

web-common/src/features/dashboards/big-number/MeasureBigNumber.svelte

web-admin/src/routes/[organization]/[project]/explore/[dashboard]/+page.ts

web-common/src/features/dashboards/url-state/DashboardURLStateSync.svelte

ericpgreen2 · 2024-12-10T20:15:10Z

web-common/src/features/dashboards/url-state/convertURLToExplorePreset.ts

General reminder that this should be a conversion to ExploreState not to ExplorePreset

Yes. This PR is not an exhaustive refactor. Lets do that in a separate PR.

web-common/src/features/explores/selectors.ts

briangregoryholmes · 2024-12-10T22:50:49Z

Related issue here: #6248

web-common/src/features/dashboards/url-state/convertURLToExplorePreset.ts

web-common/src/features/metrics-views/editor/MetricsEditor.svelte

ericpgreen2 · 2024-12-12T22:42:07Z

web-common/src/features/dashboards/url-state/DashboardURLStateSync.svelte

  export let defaultExplorePreset: V1ExplorePreset;
-  export let partialExploreState: Partial<MetricsExplorerEntity>;
+  export let exploreStateFromYAMLConfig: Partial<MetricsExplorerEntity>;


These props are covering two different state sources: the Rill-opinionated default state (A) and the developer-specified default state (B).

defaultExplorePreset combines A + B.
exploreStateFromYAMLConfig is just B.

We should have 1 prop for A and 1 prop for B.

"A" should always be of the type ExploreState. I think "B" should be converted from ExplorePreset to ExploreState as soon as we get it back from the GetResource API.

Lets take that refactor in a future PR. ExploreState should inherit from ExplorePreset to avoid mapping of individual fields.

web-common/src/features/dashboards/url-state/DashboardURLStateSync.svelte

web-admin/src/routes/[organization]/[project]/explore/[dashboard]/+layout.ts

ericpgreen2 · 2024-12-12T23:09:32Z

web-common/src/features/dashboards/url-state/DashboardURLStateSync.svelte

  export let defaultExplorePreset: V1ExplorePreset;
-  export let partialExploreState: Partial<MetricsExplorerEntity>;
+  export let exploreStateFromYAMLConfig: Partial<MetricsExplorerEntity>;
+  export let initExploreState: Partial<MetricsExplorerEntity> | undefined =


"initExploreState" doesn't specify which source of state it is.

Hmmm this is supposed to be bookmark state on cloud explore, token's state for public URLs and undefined for rill-dev. Not sure what a good name would be to capture this. Adding them explicitly like bookmarkExploreState etc doesnt make sense since it is irrelevant on rill-dev.

I think optional props would be most clear:

export let bookmarkExploreState: Partial<ExploreState> | undefined = undefined; export let tokenExploreState: Partial<ExploreState> | undefined = undefined;

I have renamed the variables until the call to DashboardURLStateSync. We should avoid having cloud/dev specific login in common code.

ericpgreen2

UXQA:

Navigate from the project page to a dashboard, add a filter, click the back button, the filter is not removed. Click the back button again, and you're navigated directly to the project page. Jam.
When I highlight a time range, clicking "back" does not remove it: Jam
When I move a highlighted time range to the end of the time series, it gets stuck. I'm unable to move it, and clicking esc does not remove it. Jam
Go from a small timerange & grain (e.g. Last 14 Days, Daily) to a large one (e.g. All Time, which flips to Monthly by default). The dashboard reflects the new time grain (Monthly), but the URL does not (Daily).
In the GitHub Analytics example, there are some commit values that can't be filtered on: Jam. (Maybe this fix belongs in a separate PR.)

I found the above issues from playing with this branch for 5-10 minutes. I will stop there, so, after fixing, please do a thorough round of QA yourself.

ericpgreen2 · 2024-12-16T19:27:00Z

web-common/src/features/dashboards/stores/dashboard-store-defaults.ts

 // TODO: Remove this in favour of just `getBasePreset`
 export function getDefaultExploreState(
  name: string,
-  metricsView: V1MetricsViewSpec,
-  explore: V1ExploreSpec,
-  fullTimeRange: V1MetricsViewTimeRangeResponse | undefined,
-  defaultExplorePreset = getDefaultExplorePreset(explore, fullTimeRange),
+  initState: Partial<MetricsExplorerEntity>,
 ): MetricsExplorerEntity {


For now, can you update this comment and describe the difference between the "default" explore state and the "init" explore state? (Though we should disentangle the defaults in a follow-up PR)

Update the variable names. getFullInitExploreState for the function and partialInitState for the argument.

web-common/src/features/explores/selectors.ts

ericpgreen2 · 2024-12-16T19:34:04Z

web-common/src/features/entity-management/file-artifact.ts

@@ -74,6 +74,7 @@ export class FileArtifact {
  readonly fileName: string;
  readonly disableAutoSave: boolean;
  readonly autoSave: Writable<boolean>;
+  lastSeenResource: V1Resource;


Why is this needed? This seems redundant to resourceName

We need to get all explores that are referring to a metrics view exploreResource?.explore?.state?.validSpec?.metricsView so just the name is not enough.

ericpgreen2 · 2024-12-16T19:39:56Z

web-common/src/features/metrics-views/editor/MetricsEditor.svelte

+      // Reset local persisted explore state derived from this metrics view
+      fileArtifacts
+        .getResourcesForKind(ResourceKind.Explore)
+        .forEach((exploreResource) => {
+          const exploreName = exploreResource?.meta?.name?.name;
+          if (
+            !exploreName ||
+            !exploreResource?.explore?.state?.validSpec?.metricsView ||
+            exploreResource?.explore?.state?.validSpec?.metricsView !==
+              metricsViewName
+          )
+            return;
+          clearExploreSessionStore(exploreName, undefined);
+        });


Rather than clearing the session store here, what if we cleared the session store whenever a user navigates away from the dashboard? Similarly, in Cloud, I'm thinking we should clear the session store whenever the user navigates away from the dashboard (say, to the project homepage).

The session store is primarily meant to facilitate navigation between a dashboard's different views. It's not intended to be a "last viewed state" feature when leaving & returning to a dashboard.

ericpgreen2

I'll approve and merge, so we can get this progress into the main branch and take the follow-ups in smaller PRs. Let's please try to work via more focused, smaller-in-scope PRs. Most of the bullet points in the PR's description could've merited its own dedicated PR.

ericpgreen2 · 2024-12-17T21:27:51Z

web-common/src/features/dashboards/url-state/getExploreStateFromSessionStorage.ts

+    ? FromURLParamViewMap[viewFromUrl]
+    : (defaultExplorePreset.view ??
+      V1ExploreWebView.EXPLORE_WEB_VIEW_UNSPECIFIED);
+  const explorePresetFromSessionStorage = getExplorePresetForWebView(


We should be getting ExploreState from session storage, not ExplorePreset

ericpgreen2 · 2024-12-17T21:28:46Z

web-common/src/features/dashboards/url-state/getExploreStateFromSessionStorage.ts

+ * 2. If only view param is set then load the params from session storage.
+ * 3. If view=ttd and `measure` is the only other param set load from session storage.
+ */
+export function getExploreStateFromSessionStorage(


I'd expect to see a call to session storage in this function. Perhaps getExplorePresetForWebView (where I see the call to session storage actually happens) is an unnecessary additional layer.

AdityaHegde added 4 commits December 9, 2024 18:40

Use history API to redirect URL

234b304

Fix issues with cloud

ddb1aa1

Merge branch 'main' into fix/url-history-issues

010a89f

Fix tests

66d545a

AdityaHegde force-pushed the fix/url-history-issues branch from 32a414f to 66d545a Compare December 9, 2024 15:07

AdityaHegde added 4 commits December 10, 2024 12:18

Fix race condition in reactive statement

6250069

Disable TTD for no timeseries dashboards

97ac340

Update rill-dev

1a46aca

Update token and emebd

fbe200d

AdityaHegde marked this pull request as ready for review December 10, 2024 10:33

AdityaHegde requested a review from ericpgreen2 December 10, 2024 10:33

AdityaHegde added 2 commits December 10, 2024 16:23

Merge branch 'main' into fix/url-history-issues

fc5c5f8

Remove console.log

30b0075

AdityaHegde force-pushed the fix/url-history-issues branch from 0f5ee6b to 30b0075 Compare December 10, 2024 11:16

AdityaHegde added the blocker A release blocker issue that should be resolved before a new release label Dec 10, 2024

Reset expore state when metrics view is changed

d88e4ba

ericpgreen2 requested changes Dec 10, 2024

View reviewed changes

AdityaHegde self-assigned this Dec 11, 2024

AdityaHegde added 3 commits December 12, 2024 11:37

Move all state loading decision to DashboardURLStateSync

6b526ae

Fix race condition on cloud

7c3ce36

Update tests

3fc4354

AdityaHegde force-pushed the fix/url-history-issues branch from 676a774 to 3fc4354 Compare December 12, 2024 07:16

Intercept url in beforeNavigate

fa2f85e

ericpgreen2 reviewed Dec 12, 2024

View reviewed changes

AdityaHegde added 2 commits December 13, 2024 12:07

Use goto with replaceState instead of beforeNavigate

c9b0fe9

Fix missing home bookmark while siwtching using breadcrumbs

9cafbd1

AdityaHegde force-pushed the fix/url-history-issues branch from 61fc74a to 9cafbd1 Compare December 13, 2024 10:08

Fix CI

14d76a9

AdityaHegde force-pushed the fix/url-history-issues branch from b25ea0c to 14d76a9 Compare December 13, 2024 12:56

AdityaHegde added 2 commits December 16, 2024 12:16

Rename initState before Sync component

a902ded

Add schema requests

273a675

AdityaHegde force-pushed the fix/url-history-issues branch from a8fbef9 to 273a675 Compare December 16, 2024 17:14

ericpgreen2 requested changes Dec 16, 2024

View reviewed changes

AdityaHegde added 5 commits December 17, 2024 14:33

Fix misc issues

df6b483

Fix explore preview to not retain state from visual editor

8f3b759

Merge branch 'main' into fix/url-history-issues

6e127c9

Fix timeranges with invalid comparison

33a0ac9

Use token id as key

dfd93a8

ericpgreen2 mentioned this pull request Dec 17, 2024

State Management: merge state in one place #6219

Open

ericpgreen2 approved these changes Dec 17, 2024

View reviewed changes

ericpgreen2 merged commit e5ffc5d into main Dec 17, 2024
7 checks passed

ericpgreen2 deleted the fix/url-history-issues branch December 17, 2024 21:37

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

fix: session storage retrieval for url messes with the url history #6238

fix: session storage retrieval for url messes with the url history #6238

AdityaHegde commented Dec 9, 2024 •

edited

Loading

ericpgreen2 commented Dec 10, 2024

ericpgreen2 Dec 10, 2024 •

edited

Loading

ericpgreen2 Dec 10, 2024

AdityaHegde Dec 12, 2024

ericpgreen2 Dec 10, 2024

AdityaHegde Dec 11, 2024

briangregoryholmes commented Dec 10, 2024

ericpgreen2 Dec 12, 2024

AdityaHegde Dec 13, 2024

ericpgreen2 Dec 12, 2024

AdityaHegde Dec 13, 2024

ericpgreen2 Dec 13, 2024

AdityaHegde Dec 16, 2024

ericpgreen2 left a comment •

edited

Loading

ericpgreen2 Dec 16, 2024

AdityaHegde Dec 17, 2024

ericpgreen2 Dec 16, 2024

AdityaHegde Dec 17, 2024

ericpgreen2 Dec 16, 2024

ericpgreen2 left a comment

ericpgreen2 Dec 17, 2024

ericpgreen2 Dec 17, 2024

		@@ -86,6 +77,7 @@ export function useExploreValidSpec(
		export async function fetchExploreSpec(

fix: session storage retrieval for url messes with the url history #6238

fix: session storage retrieval for url messes with the url history #6238

Conversation

AdityaHegde commented Dec 9, 2024 • edited Loading

ericpgreen2 commented Dec 10, 2024

ericpgreen2 Dec 10, 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

Choose a reason for hiding this comment

briangregoryholmes commented Dec 10, 2024

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

ericpgreen2 left a comment • 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

ericpgreen2 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

AdityaHegde commented Dec 9, 2024 •

edited

Loading

ericpgreen2 Dec 10, 2024 •

edited

Loading

ericpgreen2 left a comment •

edited

Loading