Fix race condition in reactive statement

rilldata · Dec 10, 2024 · 6250069 · 6250069
1 parent 66d545a
commit 6250069
Show file tree

Hide file tree

Showing 19 changed files with 269 additions and 286 deletions.
diff --git a/web-admin/src/features/bookmarks/selectors.ts b/web-admin/src/features/bookmarks/selectors.ts
@@ -67,6 +67,7 @@ export function categorizeBookmarks(
     personal: [],
     shared: [],
   };
+  if (!exploreState) return bookmarks;
   bookmarkResp?.forEach((bookmarkResource) => {
     const bookmark = parseBookmark(
       bookmarkResource,

diff --git a/web-admin/src/routes/[organization]/[project]/-/share/[token]/+page.svelte b/web-admin/src/routes/[organization]/[project]/-/share/[token]/+page.svelte
@@ -16,6 +16,7 @@
   $: ({
     defaultExplorePreset,
     partialExploreState,
+    loadedOutsideOfURL,
     token: { resourceName },
   } = data);
   $: ({ organization, project } = $page.params);
@@ -55,7 +56,11 @@
       metricsViewName={explore.metricsView.meta.name.name}
       exploreName={resourceName}
     >
-      <DashboardURLStateSync {defaultExplorePreset} {partialExploreState}>
+      <DashboardURLStateSync
+        {defaultExplorePreset}
+        {partialExploreState}
+        {loadedOutsideOfURL}
+      >
         <DashboardThemeProvider>
           <Dashboard
             exploreName={resourceName}

diff --git a/web-admin/src/routes/[organization]/[project]/-/share/[token]/+page.ts b/web-admin/src/routes/[organization]/[project]/-/share/[token]/+page.ts
@@ -6,8 +6,9 @@ import { convertURLToExploreState } from "@rilldata/web-common/features/dashboar
 import { redirect } from "@sveltejs/kit";
 import { get } from "svelte/store";
 
-export const load = async ({ url, parent }) => {
+export const load = async ({ url, parent, params }) => {
   const { explore, metricsView, defaultExplorePreset, token } = await parent();
+  const { organization, project } = params;
   const exploreName = token.resourceName;
   const metricsViewSpec = metricsView.metricsView?.state?.validSpec;
   const exploreSpec = explore.explore?.state?.validSpec;
@@ -35,23 +36,29 @@ export const load = async ({ url, parent }) => {
 
   // Get Explore state from URL params
   let partialExploreState: Partial<MetricsExplorerEntity> = {};
+  let loadedOutsideOfURL = false;
   const errors: Error[] = [];
   if (metricsViewSpec && exploreSpec) {
     const {
       partialExploreState: partialExploreStateFromUrl,
+      loadedOutsideOfURL: partialLoadedOutsideOfURL,
       errors: errorsFromConvert,
     } = convertURLToExploreState(
+      exploreName,
+      `__${organization}__${project}`,
       url.searchParams,
       metricsViewSpec,
       exploreSpec,
       defaultExplorePreset,
     );
     partialExploreState = partialExploreStateFromUrl;
     errors.push(...errorsFromConvert);
+    loadedOutsideOfURL = partialLoadedOutsideOfURL;
   }
 
   return {
     partialExploreState,
+    loadedOutsideOfURL,
     errors,
   };
 };
diff --git a/web-admin/src/routes/[organization]/[project]/explore/[dashboard]/+layout.ts b/web-admin/src/routes/[organization]/[project]/explore/[dashboard]/+layout.ts
@@ -2,6 +2,9 @@ import {
   fetchBookmarks,
   isHomeBookmark,
 } from "@rilldata/web-admin/features/bookmarks/selectors";
+import { getDashboardStateFromUrl } from "@rilldata/web-common/features/dashboards/proto-state/fromProto";
+import type { MetricsExplorerEntity } from "@rilldata/web-common/features/dashboards/stores/metrics-explorer-entity";
+import { getUpdatedUrlForExploreState } from "@rilldata/web-common/features/dashboards/url-state/getUpdatedUrlForExploreState";
 import { fetchExploreSpec } from "@rilldata/web-common/features/explores/selectors";
 import {
   type V1ExplorePreset,
@@ -11,31 +14,72 @@ import {
 export const load = async ({ params, depends, parent }) => {
   const { project, runtime } = await parent();
 
-  const exploreName = params.dashboard;
+  const { organization, project: projectName, dashboard: exploreName } = params;
 
   depends(exploreName, "explore");
 
+  let explore: V1Resource | undefined;
+  let metricsView: V1Resource | undefined;
+  let defaultExplorePreset: V1ExplorePreset | undefined;
+  let initExploreState: Partial<MetricsExplorerEntity> = {};
+  let initLoadedOutsideOfURL = false;
   try {
-    const { explore, metricsView, defaultExplorePreset } =
-      await fetchExploreSpec(runtime?.instanceId, exploreName);
-
-    // used to merge home bookmark to url state
-    const bookmarks = await fetchBookmarks(project.id, exploreName);
-
-    return {
-      explore,
-      metricsView,
-      defaultExplorePreset,
-      homeBookmark: bookmarks.find(isHomeBookmark),
-    };
+    const fetchedExploreSpecDetails = await fetchExploreSpec(
+      runtime?.instanceId,
+      exploreName,
+      `__${organization}__${projectName}`,
+    );
+    explore = fetchedExploreSpecDetails.explore;
+    metricsView = fetchedExploreSpecDetails.metricsView;
+    defaultExplorePreset = fetchedExploreSpecDetails.defaultExplorePreset;
+    initExploreState = fetchedExploreSpecDetails.initExploreState;
+    initLoadedOutsideOfURL = fetchedExploreSpecDetails.initLoadedOutsideOfURL;
   } catch {
     // error handled in +page.svelte for now
     // TODO: move it here
     return {
       explore: <V1Resource>{},
       metricsView: <V1Resource>{},
       defaultExplorePreset: <V1ExplorePreset>{},
-      homeBookmark: undefined,
+      initExploreState: {},
+      initLoadedOutsideOfURL: false,
     };
   }
+
+  const metricsViewSpec = metricsView.metricsView?.state?.validSpec ?? {};
+  const exploreSpec = explore.explore?.state?.validSpec ?? {};
+
+  try {
+    const bookmarks = await fetchBookmarks(project.id, exploreName);
+    const homeBookmark = bookmarks.find(isHomeBookmark);
+
+    if (homeBookmark) {
+      const exploreStateFromBookmark = getDashboardStateFromUrl(
+        homeBookmark.data ?? "",
+        metricsViewSpec,
+        exploreSpec,
+        {}, // TODO
+      );
+      Object.assign(initExploreState, exploreStateFromBookmark);
+      initLoadedOutsideOfURL = true;
+    }
+  } catch {
+    // TODO
+  }
+  const initUrlSearch = initLoadedOutsideOfURL
+    ? getUpdatedUrlForExploreState(
+        exploreSpec,
+        defaultExplorePreset,
+        initExploreState,
+        new URLSearchParams(),
+      )
+    : "";
+
+  return {
+    explore,
+    metricsView,
+    defaultExplorePreset,
+    initExploreState,
+    initUrlSearch,
+  };
 };
diff --git a/web-admin/src/routes/[organization]/[project]/explore/[dashboard]/+page.svelte b/web-admin/src/routes/[organization]/[project]/explore/[dashboard]/+page.svelte
@@ -19,7 +19,15 @@
   // const PollIntervalWhenDashboardOk = 60000; // This triggers a layout shift, so removing for now
 
   export let data: PageData;
-  $: ({ defaultExplorePreset, partialExploreState, loaded, errors } = data);
+  $: ({
+    defaultExplorePreset,
+    initExploreState,
+    initUrlSearch,
+    partialExploreState,
+    urlSearchForPartial,
+    errors,
+    exploreName,
+  } = data);
   $: if (errors?.length) {
     setTimeout(() => {
       eventBus.emit("notification", {
@@ -33,11 +41,7 @@
   }
   $: instanceId = $runtime?.instanceId;
 
-  $: ({
-    organization: orgName,
-    project: projectName,
-    dashboard: exploreName,
-  } = $page.params);
+  $: ({ organization: orgName, project: projectName } = $page.params);
 
   $: explore = useExplore(instanceId, exploreName, {
     refetchInterval: () => {
@@ -99,9 +103,13 @@
     {#key exploreName}
       <StateManagersProvider {metricsViewName} {exploreName}>
         <DashboardURLStateSync
+          {metricsViewName}
+          {exploreName}
           {defaultExplorePreset}
+          {initExploreState}
+          {initUrlSearch}
           {partialExploreState}
-          {loaded}
+          {urlSearchForPartial}
         >
           <DashboardThemeProvider>
             <Dashboard {metricsViewName} {exploreName} />

diff --git a/web-admin/src/routes/[organization]/[project]/explore/[dashboard]/+page.ts b/web-admin/src/routes/[organization]/[project]/explore/[dashboard]/+page.ts
@@ -1,49 +1,35 @@
-import { convertBookmarkToUrlSearchParams } from "@rilldata/web-admin/features/bookmarks/selectors";
-import { metricsExplorerStore } from "@rilldata/web-common/features/dashboards/stores/dashboard-stores";
 import { convertURLToExploreState } from "@rilldata/web-common/features/dashboards/url-state/convertPresetToExploreState";
-import { redirect } from "@sveltejs/kit";
-import { get } from "svelte/store";
+import { getUpdatedUrlForExploreState } from "@rilldata/web-common/features/dashboards/url-state/getUpdatedUrlForExploreState";
 
 export const load = async ({ url, parent, params }) => {
-  const { explore, metricsView, defaultExplorePreset, homeBookmark } =
-    await parent();
+  const { explore, metricsView, defaultExplorePreset } = await parent();
   const { organization, project, dashboard: exploreName } = params;
   const metricsViewSpec = metricsView.metricsView?.state?.validSpec;
   const exploreSpec = explore.explore?.state?.validSpec;
 
-  // On the first dashboard load, if there are no URL params, redirect to the "Home" bookmark.
-  if (
-    homeBookmark &&
-    ![...url.searchParams.keys()].length &&
-    !(exploreName in get(metricsExplorerStore).entities)
-  ) {
-    const newUrl = new URL(url);
-    newUrl.search = convertBookmarkToUrlSearchParams(
-      homeBookmark,
+  const { partialExploreState, loadedOutsideOfURL, errors } =
+    convertURLToExploreState(
+      exploreName,
+      `__${organization}__${project}`,
+      url.searchParams,
       metricsViewSpec,
       exploreSpec,
-      {}, // TODO
-      undefined,
       defaultExplorePreset,
     );
+  const urlSearchForPartial = loadedOutsideOfURL
+    ? getUpdatedUrlForExploreState(
+        exploreSpec,
+        defaultExplorePreset,
+        partialExploreState,
+        url.searchParams,
+      )
+    : url.searchParams.toString();
 
-    if (newUrl.search !== url.search) {
-      throw redirect(307, `${newUrl.pathname}${newUrl.search}`);
-    }
-  }
-
-  const { partialExploreState, loaded, errors } = convertURLToExploreState(
-    exploreName,
-    `__${organization}__${project}`,
-    url.searchParams,
-    metricsViewSpec,
-    exploreSpec,
-    defaultExplorePreset,
-  );
-
+  console.log(exploreName, urlSearchForPartial);
   return {
     partialExploreState,
-    loaded,
+    urlSearchForPartial,
     errors,
+    exploreName,
   };
 };
diff --git a/web-common/src/features/dashboards/state-managers/StateManagersProvider.svelte b/web-common/src/features/dashboards/state-managers/StateManagersProvider.svelte
@@ -1,6 +1,6 @@
 <script lang="ts">
   import { page } from "$app/stores";
-  import { setContext } from "svelte";
+  import { onMount, setContext } from "svelte";
   import { useQueryClient } from "@tanstack/svelte-query";
   import { createStateManagers, DEFAULT_STORE_KEY } from "./state-managers";
 
@@ -21,6 +21,8 @@
   });
   setContext(DEFAULT_STORE_KEY, stateManagers);
 
+  onMount(() => () => stateManagers.cleanup());
+
   // Our state management was not built around the ability to arbitrarily change the explore or metrics view name
   // This needs to change, but this is a workaround for now
   $: if (visualEditing) {

diff --git a/web-common/src/features/dashboards/state-managers/state-managers.ts b/web-common/src/features/dashboards/state-managers/state-managers.ts
@@ -3,10 +3,7 @@ import {
   type MetricsExplorerEntity,
   contextColWidthDefaults,
 } from "@rilldata/web-common/features/dashboards/stores/metrics-explorer-entity";
-import {
-  getPersistentDashboardStore,
-  initPersistentDashboardStore,
-} from "@rilldata/web-common/features/dashboards/stores/persistent-dashboard-state";
+import { createPersistentDashboardStore } from "@rilldata/web-common/features/dashboards/stores/persistent-dashboard-state";
 import { updateExploreSessionStore } from "@rilldata/web-common/features/dashboards/url-state/explore-web-view-store";
 import { getDefaultExplorePreset } from "@rilldata/web-common/features/dashboards/url-state/getDefaultExplorePreset";
 import { initLocalUserPreferenceStore } from "@rilldata/web-common/features/dashboards/user-preferences";
@@ -74,6 +71,8 @@ export type StateManagers = {
    */
   contextColumnWidths: Writable<ContextColWidths>;
   defaultExploreState: Readable<V1ExplorePreset>;
+
+  cleanup: () => void;
 };
 
 export const DEFAULT_STORE_KEY = Symbol("state-managers");
@@ -154,21 +153,22 @@ export function createStateManagers({
       );
     },
   );
-  dashboardStore.subscribe((dashState) => {
+  const unsub = dashboardStore.subscribe((dashState) => {
     const exploreState = get(validSpecStore).data?.explore;
-    if (!dashState || !exploreState) return;
+    if (!dashState || !exploreState || dashState.name !== get(exploreNameStore))
+      return;
     updateExploreSessionStore(
-      exploreName,
+      get(exploreNameStore),
       extraKeyPrefix,
       dashState,
       exploreState,
     );
   });
 
-  // TODO: once we move everything from dashboard-stores to here, we can get rid of the global
-  initPersistentDashboardStore((extraKeyPrefix || "") + exploreName);
+  const persistentDashboardStore = createPersistentDashboardStore(
+    (extraKeyPrefix || "") + exploreName,
+  );
   initLocalUserPreferenceStore(exploreName);
-  const persistentDashboardStore = getPersistentDashboardStore();
 
   return {
     runtime: runtime,
@@ -199,5 +199,9 @@ export function createStateManagers({
     }),
     contextColumnWidths,
     defaultExploreState,
+
+    cleanup: () => {
+      unsub();
+    },
   };
 }
diff --git a/web-common/src/features/dashboards/stores/DashboardStateProvider.svelte b/web-common/src/features/dashboards/stores/DashboardStateProvider.svelte