fix: bookmark interaction with url params (#6318)

* Disable delete bookmark button while deleting * Fix filterOnly calucation for bookmark edit * Fix dashboards without timestamp and home bookmark
rilldata · Dec 30, 2024 · c79d9a5 · c79d9a5 · github-actions · Dec 30, 2024
1 parent 33dc7d6
commit c79d9a5
Show file tree

Hide file tree

Showing 5 changed files with 48 additions and 51 deletions.
diff --git a/web-admin/src/features/bookmarks/Bookmarks.svelte b/web-admin/src/features/bookmarks/Bookmarks.svelte
@@ -98,13 +98,14 @@
     </Button>
   </DropdownMenuTrigger>
   <BookmarksContent
-    on:create={() => (showDialog = true)}
-    on:create-home={() => createHomeBookmark()}
-    on:delete={({ detail }) => deleteBookmark(detail)}
-    on:edit={({ detail }) => {
+    onCreate={(isHome) => {
+      isHome ? createHomeBookmark() : (showDialog = true);
+    }}
+    onEdit={(editingBookmark) => {
       showDialog = true;
-      bookmark = detail;
+      bookmark = editingBookmark;
     }}
+    onDelete={deleteBookmark}
     {metricsViewName}
     {exploreName}
   />

diff --git a/web-admin/src/features/bookmarks/BookmarksDropdownMenuContent.svelte b/web-admin/src/features/bookmarks/BookmarksDropdownMenuContent.svelte
@@ -6,6 +6,7 @@
   } from "@rilldata/web-admin/client";
   import BookmarkItem from "@rilldata/web-admin/features/bookmarks/BookmarksDropdownMenuItem.svelte";
   import {
+    type BookmarkEntry,
     categorizeBookmarks,
     searchBookmarks,
   } from "@rilldata/web-admin/features/bookmarks/selectors";
@@ -30,12 +31,13 @@
   import { createQueryServiceMetricsViewSchema } from "@rilldata/web-common/runtime-client";
   import { runtime } from "@rilldata/web-common/runtime-client/runtime-store";
   import { BookmarkPlusIcon } from "lucide-svelte";
-  import { createEventDispatcher } from "svelte";
 
   export let metricsViewName: string;
   export let exploreName: string;
 
-  const dispatch = createEventDispatcher();
+  export let onCreate: (isHome: boolean) => void;
+  export let onEdit: (bookmark: BookmarkEntry) => void;
+  export let onDelete: (bookmark: BookmarkEntry) => Promise<void>;
 
   $: ({ instanceId } = $runtime);
 
@@ -91,17 +93,14 @@
 </script>
 
 <DropdownMenuContent class="w-[450px]">
-  <DropdownMenuItem on:click={() => dispatch("create")}>
+  <DropdownMenuItem on:click={() => onCreate(false)}>
     <div class="flex flex-row gap-x-2 items-center">
       <BookmarkPlusIcon size="16px" strokeWidth={1.5} />
       <div class="text-xs">Bookmark current view</div>
     </div>
   </DropdownMenuItem>
   {#if manageProject}
-    <DropdownMenuItem
-      on:click={() => dispatch("create-home")}
-      slot="manage-project"
-    >
+    <DropdownMenuItem on:click={() => onCreate(true)} slot="manage-project">
       <div class="flex flex-row gap-x-2">
         <HomeBookmarkPlus size="16px" />
         <div>
@@ -132,7 +131,7 @@
       {#if filteredBookmarks.personal?.length}
         {#each filteredBookmarks.personal as bookmark}
           {#key bookmark.resource.id}
-            <BookmarkItem {bookmark} on:edit on:select on:delete />
+            <BookmarkItem {bookmark} {onEdit} {onDelete} on:select />
           {/key}
         {/each}
       {:else}
@@ -152,8 +151,8 @@
           {#key filteredBookmarks.home.resource.id}
             <BookmarkItem
               bookmark={filteredBookmarks.home}
-              on:edit
-              on:delete
+              {onEdit}
+              {onDelete}
               readOnly={!manageProject}
             />
           {/key}
@@ -162,8 +161,8 @@
           {#key bookmark.resource.id}
             <BookmarkItem
               {bookmark}
-              on:edit
-              on:delete
+              {onEdit}
+              {onDelete}
               readOnly={!manageProject}
             />
           {/key}

diff --git a/web-admin/src/features/bookmarks/BookmarksDropdownMenuItem.svelte b/web-admin/src/features/bookmarks/BookmarksDropdownMenuItem.svelte
@@ -6,21 +6,28 @@
   import HomeBookmark from "@rilldata/web-common/components/icons/HomeBookmark.svelte";
   import Trash from "@rilldata/web-common/components/icons/Trash.svelte";
   import { BookmarkIcon } from "lucide-svelte";
-  import { createEventDispatcher } from "svelte";
 
   export let bookmark: BookmarkEntry;
   export let readOnly = false;
 
-  const dispatch = createEventDispatcher();
+  export let onEdit: (bookmark: BookmarkEntry) => void;
+  export let onDelete: (bookmark: BookmarkEntry) => Promise<void>;
 
   function editBookmark(e) {
     e.skipSelection = true;
-    dispatch("edit", bookmark);
+    onEdit(bookmark);
   }
 
-  function deleteBookmark(e) {
+  let disableDelete = false;
+  async function deleteBookmark(e) {
+    disableDelete = true;
     e.skipSelection = true;
-    dispatch("delete", bookmark);
+    try {
+      await onDelete(bookmark);
+    } catch {
+      // no-op
+    }
+    disableDelete = false;
   }
 
   let hovered = false;
@@ -34,7 +41,7 @@
     role="menuitem"
     tabindex="-1"
   >
-    <a href={bookmark.url} class="flex flex-row gap-x-2 w-full">
+    <a href={bookmark.url} class="flex flex-row gap-x-2 w-full min-h-7">
       {#if bookmark.resource.default}
         <HomeBookmark size="16px" />
       {:else if bookmark.filtersOnly}
@@ -69,6 +76,8 @@
           <button
             on:click={deleteBookmark}
             class="bg-gray-100 hover:bg-primary-100 px-2 h-7 text-gray-400 hover:text-gray-500"
+            disabled={disableDelete}
+            aria-disabled={disableDelete}
           >
             <Trash size="16px" />
           </button>

diff --git a/web-admin/src/features/bookmarks/selectors.ts b/web-admin/src/features/bookmarks/selectors.ts
@@ -142,15 +142,15 @@ export function getPrettySelectedTimeRange(
   );
 }
 
-export function convertBookmarkToUrlSearchParams(
+function parseBookmark(
   bookmarkResource: V1Bookmark,
   metricsViewSpec: V1MetricsViewSpec,
   exploreSpec: V1ExploreSpec,
   schema: V1StructType,
-  exploreState: MetricsExplorerEntity | undefined,
+  exploreState: MetricsExplorerEntity,
   defaultExplorePreset: V1ExplorePreset,
   timeRangeSummary: V1TimeRangeSummary | undefined,
-) {
+): BookmarkEntry {
   const exploreStateFromBookmark = getDashboardStateFromUrl(
     bookmarkResource.data ?? "",
     metricsViewSpec,
@@ -161,7 +161,9 @@ export function convertBookmarkToUrlSearchParams(
     ...(exploreState ?? {}),
     ...exploreStateFromBookmark,
   } as MetricsExplorerEntity;
-  return convertExploreStateToURLSearchParams(
+
+  const url = new URL(get(page).url);
+  url.search = convertExploreStateToURLSearchParams(
     finalExploreState,
     exploreSpec,
     getTimeControlState(
@@ -172,32 +174,12 @@ export function convertBookmarkToUrlSearchParams(
     ),
     defaultExplorePreset,
   );
-}
-
-function parseBookmark(
-  bookmarkResource: V1Bookmark,
-  metricsViewSpec: V1MetricsViewSpec,
-  exploreSpec: V1ExploreSpec,
-  schema: V1StructType,
-  exploreState: MetricsExplorerEntity,
-  defaultExplorePreset: V1ExplorePreset,
-  timeRangeSummary: V1TimeRangeSummary | undefined,
-): BookmarkEntry {
-  const url = new URL(get(page).url);
-  url.search = convertBookmarkToUrlSearchParams(
-    bookmarkResource,
-    metricsViewSpec,
-    exploreSpec,
-    schema,
-    exploreState,
-    defaultExplorePreset,
-    timeRangeSummary,
-  );
   return {
     resource: bookmarkResource,
     absoluteTimeRange:
-      exploreState.selectedTimeRange?.name === TimeRangePreset.CUSTOM,
-    filtersOnly: !exploreState.pivot,
+      exploreStateFromBookmark.selectedTimeRange?.name ===
+      TimeRangePreset.CUSTOM,
+    filtersOnly: !exploreStateFromBookmark.pivot,
     url: url.toString(),
   };
 }
diff --git a/web-common/src/features/dashboards/url-state/DashboardURLStateSync.svelte b/web-common/src/features/dashboards/url-state/DashboardURLStateSync.svelte
@@ -162,6 +162,8 @@
     }
 
     prevUrl = redirectUrl.toString();
+    // using `replaceState` directly messes up the navigation entries,
+    // `from` and `to` have the old url before being replaced in `afterNavigate` calls leading to incorrect handling.
     void goto(redirectUrl, {
       replaceState: true,
       state: $page.state,
@@ -196,7 +198,11 @@
       };
     }
 
-    await waitUntil(() => !timeRangeSummaryIsLoading);
+    // time range summary query has `enabled` based on `metricsSpec.timeDimension`
+    // isLoading will never be true when the query is disabled, so we need this check before waiting for it.
+    if (metricsSpec.timeDimension) {
+      await waitUntil(() => !timeRangeSummaryIsLoading);
+    }
     metricsExplorerStore.init(exploreName, initState);
     timeControlsState ??= getTimeControlState(
       metricsSpec,