Skip to content

Commit

Permalink
fix: human readable params issues like special chars in filters and u…
Browse files Browse the repository at this point in the history
…rl history issues (#6301)

* Allow special chars in column names in filters

* Use time controls store in url params calcs

* Remove session store after navigating away from dashboard

* Fix bookmarks not being applied if the store was initialised

* PR comments
  • Loading branch information
AdityaHegde authored Dec 19, 2024
1 parent 8e5b31f commit 671a1a6
Show file tree
Hide file tree
Showing 17 changed files with 342 additions and 163 deletions.
1 change: 1 addition & 0 deletions web-admin/src/features/bookmarks/selectors.ts
Original file line number Diff line number Diff line change
Expand Up @@ -156,6 +156,7 @@ export function convertBookmarkToUrlSearchParams(
...exploreStateFromBookmark,
} as MetricsExplorerEntity,
exploreSpec,
undefined, // TODO
defaultExplorePreset,
);
}
Expand Down
8 changes: 8 additions & 0 deletions web-admin/src/features/dashboards/query-mappers/utils.ts
Original file line number Diff line number Diff line change
@@ -1,5 +1,6 @@
import { createInExpression } from "@rilldata/web-common/features/dashboards/stores/filter-utils";
import type { MetricsExplorerEntity } from "@rilldata/web-common/features/dashboards/stores/metrics-explorer-entity";
import { getTimeControlState } from "@rilldata/web-common/features/dashboards/time-controls/time-control-store";
import { PreviousCompleteRangeMap } from "@rilldata/web-common/features/dashboards/time-controls/time-range-mappers";
import { convertExploreStateToURLSearchParams } from "@rilldata/web-common/features/dashboards/url-state/convertExploreStateToURLSearchParams";
import { getDefaultExplorePreset } from "@rilldata/web-common/features/dashboards/url-state/getDefaultExplorePreset";
Expand Down Expand Up @@ -182,6 +183,7 @@ export async function getExplorePageUrl(
const url = new URL(`${curPageUrl.protocol}//${curPageUrl.host}`);
url.pathname = `/${organization}/${project}/explore/${exploreName}`;

const metricsViewSpec = metricsView?.metricsView?.state?.validSpec ?? {};
const exploreSpec = explore?.explore?.state?.validSpec ?? {};
const metricsViewName = exploreSpec.metricsView;

Expand All @@ -206,6 +208,12 @@ export async function getExplorePageUrl(
url.search = convertExploreStateToURLSearchParams(
exploreState,
exploreSpec,
getTimeControlState(
metricsViewSpec,
exploreSpec,
fullTimeRange?.timeRangeSummary,
exploreState,
),
getDefaultExplorePreset(exploreSpec, fullTimeRange),
);
return url.toString();
Expand Down
2 changes: 0 additions & 2 deletions web-common/src/features/dashboards/stores/dashboard-stores.ts
Original file line number Diff line number Diff line change
Expand Up @@ -168,8 +168,6 @@ function syncDimensions(
const metricsViewReducers = {
init(name: string, initState: Partial<MetricsExplorerEntity> = {}) {
update((state) => {
if (state.entities[name]) return state;

state.entities[name] = getFullInitExploreState(name, initState);

updateMetricsExplorerProto(state.entities[name]);
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -193,6 +193,7 @@ export const AD_BIDS_METRICS_3_MEASURES_DIMENSIONS: V1MetricsViewSpec = {
table: AD_BIDS_SOURCE_NAME,
measures: AD_BIDS_THREE_MEASURES,
dimensions: AD_BIDS_THREE_DIMENSIONS,
timeDimension: AD_BIDS_TIMESTAMP_DIMENSION,
};

export const AD_BIDS_EXPLORE_INIT: V1ExploreSpec = {
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -30,6 +30,7 @@ import {
type V1MetricsViewSpec,
type V1MetricsViewTimeRangeResponse,
V1TimeGrain,
type V1TimeRangeSummary,
} from "@rilldata/web-common/runtime-client";
import type { QueryObserverResult } from "@tanstack/svelte-query";
import type { Readable } from "svelte/store";
Expand Down Expand Up @@ -68,6 +69,12 @@ export type TimeControlState = {
ComparisonTimeRangeState;
export type TimeControlStore = Readable<TimeControlState>;

/**
* Returns a TimeControlState. Calls getTimeControlState internally.
*
* Consumers of this will have a QueryObserverResult for time range summary.
* They will need `isFetching` to wait for this response.
*/
export const timeControlStateSelector = ([
metricsView,
explore,
Expand All @@ -80,70 +87,99 @@ export const timeControlStateSelector = ([
MetricsExplorerEntity,
]): TimeControlState => {
const hasTimeSeries = Boolean(metricsView?.timeDimension);
const timeDimension = metricsView?.timeDimension;
if (
!metricsView ||
!explore ||
!metricsExplorer ||
!timeRangeResponse ||
!timeRangeResponse.isSuccess ||
!timeRangeResponse.data.timeRangeSummary?.min ||
!timeRangeResponse.data.timeRangeSummary?.max
!hasTimeSeries
) {
return {
isFetching: timeRangeResponse.isRefetching,
ready: !metricsExplorer || !hasTimeSeries,
} as TimeControlState;
}

const state = getTimeControlState(
metricsView,
explore,
timeRangeResponse.data?.timeRangeSummary,
metricsExplorer,
);
if (!state) {
return {
ready: false,
isFetching: false,
};
}

return {
...state,
isFetching: false,
ready: true,
} as TimeControlState;
};

/**
* Generates TimeControlState
*
* Consumers of this will already have a V1TimeRangeSummary.
*/
export function getTimeControlState(
metricsViewSpec: V1MetricsViewSpec,
exploreSpec: V1ExploreSpec,
timeRangeSummary: V1TimeRangeSummary | undefined,
exploreState: MetricsExplorerEntity,
) {
const hasTimeSeries = Boolean(metricsViewSpec.timeDimension);
const timeDimension = metricsViewSpec.timeDimension;
if (!hasTimeSeries || !timeRangeSummary?.max || !timeRangeSummary?.min)
return undefined;

const allTimeRange = {
name: TimeRangePreset.ALL_TIME,
start: new Date(timeRangeResponse.data.timeRangeSummary.min),
end: new Date(timeRangeResponse.data.timeRangeSummary.max),
start: new Date(timeRangeSummary.min),
end: new Date(timeRangeSummary.max),
};
const minTimeGrain =
(metricsView.smallestTimeGrain as V1TimeGrain) ||
(metricsViewSpec.smallestTimeGrain as V1TimeGrain) ||
V1TimeGrain.TIME_GRAIN_UNSPECIFIED;
const defaultTimeRange = isoDurationToFullTimeRange(
explore?.defaultPreset?.timeRange,
exploreSpec.defaultPreset?.timeRange,
allTimeRange.start,
allTimeRange.end,
metricsExplorer.selectedTimezone,
exploreState.selectedTimezone,
);

const timeRangeState = calculateTimeRangePartial(
metricsExplorer,
exploreState,
allTimeRange,
defaultTimeRange,
minTimeGrain,
);
if (!timeRangeState) {
return {
ready: false,
isFetching: false,
};
return undefined;
}

const comparisonTimeRangeState = calculateComparisonTimeRangePartial(
explore,
metricsExplorer,
exploreSpec,
exploreState,
allTimeRange,
timeRangeState,
);

return {
isFetching: false,
minTimeGrain,
allTimeRange,
defaultTimeRange,
timeDimension,
ready: true,

...timeRangeState,

...comparisonTimeRangeState,
} as TimeControlState;
};
}

export function createTimeControlStore(ctx: StateManagers) {
return derived(
Expand Down
Loading

1 comment on commit 671a1a6

@github-actions
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please sign in to comment.