From c79d9a5a8a1d3ac3a72d2d8b07a4e87294691d35 Mon Sep 17 00:00:00 2001 From: Aditya Hegde Date: Mon, 30 Dec 2024 09:37:54 +0530 Subject: [PATCH] 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 --- .../src/features/bookmarks/Bookmarks.svelte | 11 +++--- .../BookmarksDropdownMenuContent.svelte | 23 ++++++------ .../BookmarksDropdownMenuItem.svelte | 21 +++++++---- web-admin/src/features/bookmarks/selectors.ts | 36 +++++-------------- .../url-state/DashboardURLStateSync.svelte | 8 ++++- 5 files changed, 48 insertions(+), 51 deletions(-) diff --git a/web-admin/src/features/bookmarks/Bookmarks.svelte b/web-admin/src/features/bookmarks/Bookmarks.svelte index 5d5cded278f..11596b36820 100644 --- a/web-admin/src/features/bookmarks/Bookmarks.svelte +++ b/web-admin/src/features/bookmarks/Bookmarks.svelte @@ -98,13 +98,14 @@ (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 index 1ad0cf74e84..5cc5aa11d1c 100644 --- 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; $: ({ instanceId } = $runtime); @@ -91,17 +93,14 @@ - dispatch("create")}> + onCreate(false)}>
Bookmark current view
 {#if manageProject} - dispatch("create-home")} - slot="manage-project" - > + onCreate(true)} slot="manage-project">
@@ -132,7 +131,7 @@ {#if filteredBookmarks.personal?.length} {#each filteredBookmarks.personal as bookmark} {#key bookmark.resource.id} - + {/key} {/each} {:else} @@ -152,8 +151,8 @@ {#key filteredBookmarks.home.resource.id} {/key} @@ -162,8 +161,8 @@ {#key bookmark.resource.id} {/key} diff --git a/web-admin/src/features/bookmarks/BookmarksDropdownMenuItem.svelte b/web-admin/src/features/bookmarks/BookmarksDropdownMenuItem.svelte index 19c0cf13b64..6078e90b142 100644 --- 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; 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" > - + {#if bookmark.resource.default} {:else if bookmark.filtersOnly} @@ -69,6 +76,8 @@ diff --git a/web-admin/src/features/bookmarks/selectors.ts b/web-admin/src/features/bookmarks/selectors.ts index cdba5a8f2ba..e77b2274049 100644 --- 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 index 077c0bb1946..50ded537836 100644 --- 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,