fix: app crash when rill time syntax is used in comparison (#9072)

* fix: app crash when rill time syntax is used in comparison

* Fix isNewSyntax and add more tests

Author: AdityaHegde

web-common/src/features/dashboards/stores/test-data/store-mutations.ts

@@ -48,7 +48,11 @@ import {
   RandomPublishers,
 } from "@rilldata/web-common/features/dashboards/stores/test-data/random";
 import { TDDChart } from "@rilldata/web-common/features/dashboards/time-dimension-details/types";
-import { TimeRangePreset } from "@rilldata/web-common/lib/time/types";
+import {
+  type DashboardTimeControls,
+  type TimeRange,
+  TimeRangePreset,
+} from "@rilldata/web-common/lib/time/types";
 import { asyncWait } from "@rilldata/web-common/lib/waitUtils.ts";
 import { DashboardState_LeaderboardSortType } from "@rilldata/web-common/proto/gen/rill/ui/v1/dashboard_pb";
 import { V1TimeGrain } from "@rilldata/web-common/runtime-client";
@@ -136,23 +140,23 @@ export const AD_BIDS_CLEAR_FILTERS: TestDashboardMutation = (mut) =>
 export const AD_BIDS_SET_P7D_TIME_RANGE_FILTER: TestDashboardMutation = () =>
   metricsExplorerStore.selectTimeRange(
     AD_BIDS_EXPLORE_NAME,
-    { name: TimeRangePreset.LAST_7_DAYS } as any,
+    { name: TimeRangePreset.LAST_7_DAYS } as TimeRange,
     V1TimeGrain.TIME_GRAIN_DAY,
     undefined,
     AD_BIDS_METRICS_INIT,
   );
 export const AD_BIDS_SET_P4W_TIME_RANGE_FILTER: TestDashboardMutation = () =>
   metricsExplorerStore.selectTimeRange(
     AD_BIDS_EXPLORE_NAME,
-    { name: TimeRangePreset.LAST_4_WEEKS } as any,
+    { name: TimeRangePreset.LAST_4_WEEKS } as TimeRange,
     V1TimeGrain.TIME_GRAIN_WEEK,
     undefined,
     AD_BIDS_METRICS_INIT,
   );
 export const AD_BIDS_SET_ALL_TIME_RANGE_FILTER: TestDashboardMutation = () =>
   metricsExplorerStore.selectTimeRange(
     AD_BIDS_EXPLORE_NAME,
-    { name: TimeRangePreset.ALL_TIME } as any,
+    { name: TimeRangePreset.ALL_TIME } as TimeRange,
     V1TimeGrain.TIME_GRAIN_DAY,
     undefined,
     AD_BIDS_METRICS_INIT,
@@ -166,7 +170,7 @@ export const AD_BIDS_SET_PREVIOUS_PERIOD_COMPARE_TIME_RANGE_FILTER: TestDashboar
     metricsExplorerStore.displayTimeComparison(AD_BIDS_EXPLORE_NAME, true);
     metricsExplorerStore.setSelectedComparisonRange(
       AD_BIDS_EXPLORE_NAME,
-      { name: "rill-PP" } as any,
+      { name: "rill-PP" } as DashboardTimeControls,
       AD_BIDS_METRICS_INIT,
     );
   };
@@ -175,7 +179,16 @@ export const AD_BIDS_SET_PREVIOUS_WEEK_COMPARE_TIME_RANGE_FILTER: TestDashboardM
     metricsExplorerStore.displayTimeComparison(AD_BIDS_EXPLORE_NAME, true);
     metricsExplorerStore.setSelectedComparisonRange(
       AD_BIDS_EXPLORE_NAME,
-      { name: "rill-PW" } as any,
+      { name: "rill-PW" } as DashboardTimeControls,
+      AD_BIDS_METRICS_INIT,
+    );
+  };
+export const AD_BIDS_SET_PREVIOUS_WEEK_RILL_TIME_COMPARE_TIME_RANGE_FILTER: TestDashboardMutation =
+  () => {
+    metricsExplorerStore.displayTimeComparison(AD_BIDS_EXPLORE_NAME, true);
+    metricsExplorerStore.setSelectedComparisonRange(
+      AD_BIDS_EXPLORE_NAME,
+      { name: "7D offset -7D" } as DashboardTimeControls,
       AD_BIDS_METRICS_INIT,
     );
   };

web-common/src/features/dashboards/url-state/time-ranges/parser.ts

@@ -11,6 +11,15 @@ export function parseRillTime(rillTimeRange: string): RillTime {
   return rt;
 }
 
+export function isNewRillTimeFormat(rillTime: string): boolean {
+  try {
+    const parser = parseRillTime(rillTime);
+    return !parser.isOldFormat;
+  } catch {
+    return false;
+  }
+}
+
 export function validateRillTime(rillTime: string): Error | undefined {
   try {
     const parser = parseRillTime(rillTime);

web-common/src/features/dashboards/url-state/url-state-variations.spec.ts

@@ -44,6 +44,7 @@ import {
   AD_BIDS_SET_PIVOT_ROW_LIMIT_UNLIMITED,
   AD_BIDS_SET_PREVIOUS_PERIOD_COMPARE_TIME_RANGE_FILTER,
   AD_BIDS_SET_PREVIOUS_WEEK_COMPARE_TIME_RANGE_FILTER,
+  AD_BIDS_SET_PREVIOUS_WEEK_RILL_TIME_COMPARE_TIME_RANGE_FILTER,
   AD_BIDS_SET_PUBLISHER_COMPARE_DIMENSION,
   AD_BIDS_SET_TIME_DIMENSION_OFFSET,
   AD_BIDS_SET_TIME_DIMENSION_PRIMARY,
@@ -158,6 +159,13 @@ const TestCases: {
     expectedSearch:
       "tr=inf&tz=Asia%2FKathmandu&grain=day&measures=impressions&dims=publisher&sort_type=percent&sort_dir=ASC",
   },
+  {
+    title: "Time range with preset and ALL_TIME selected",
+    mutations: [AD_BIDS_SET_PREVIOUS_WEEK_RILL_TIME_COMPARE_TIME_RANGE_FILTER],
+    preset: AD_BIDS_PRESET,
+    expectedSearch:
+      "tr=P7D&tz=Asia%2FKathmandu&compare_tr=7D+offset+-7D&grain=day&measures=impressions&dims=publisher&sort_type=percent&sort_dir=ASC",
+  },
 
   {
     title: "Time range comparison without preset",

web-common/src/lib/time/comparisons/index.spec.ts

@@ -90,16 +90,47 @@ const periodicComparisonTests = [
   },
 ];
 
+const invalidComparisionTests = [
+  {
+    description: "should return undefined when comparison is invalid",
+    input: {
+      start: periodStart,
+      end: periodEnd,
+      comparison: "invalid" as TimeComparisonOption,
+    },
+    output: undefined,
+  },
+  {
+    description: "should return undefined when comparison is new syntax",
+    input: {
+      start: periodStart,
+      end: periodEnd,
+      comparison: "7D as of latest/D-7D",
+    },
+    output: undefined,
+  },
+  {
+    description:
+      "should return undefined when comparison is new syntax using offset keyword",
+    input: {
+      start: periodStart,
+      end: periodEnd,
+      comparison: "7D as of latest/D offset -7D",
+    },
+    output: undefined,
+  },
+];
+
 const getComparisonIntervalTests = [
   ...contiguousAndCustomComparisonRanges,
   ...periodicComparisonTests,
+  ...invalidComparisionTests,
 ];
 
 describe("getComparisonInterval", () => {
   getComparisonIntervalTests.forEach((test) => {
     it(test.description, () => {
       const { start, end, comparison } = test.input;
-      const { start: expectedStart, end: expectedEnd } = test.output;
       const interval = Interval.fromDateTimes(
         DateTime.fromJSDate(start, { zone: "UTC" }),
         DateTime.fromJSDate(end, { zone: "UTC" }),
@@ -110,6 +141,12 @@ describe("getComparisonInterval", () => {
         comparison,
         "UTC",
       );
+      if (test.output === undefined) {
+        expect(comparisonInterval).toBeUndefined();
+        return;
+      }
+
+      const { start: expectedStart, end: expectedEnd } = test.output;
       expect(comparisonInterval?.start.toJSDate()).toEqual(expectedStart);
       expect(comparisonInterval?.end.toJSDate()).toEqual(expectedEnd);
     });

web-common/src/lib/time/comparisons/index.ts

@@ -13,6 +13,7 @@ import {
   TimeOffsetType,
   TimeRangePreset,
 } from "../types";
+import { isNewRillTimeFormat } from "@rilldata/web-common/features/dashboards/url-state/time-ranges/parser.ts";
 
 export function getComparisonTransform(
   start: Date,
@@ -316,7 +317,8 @@ export function getComparisonInterval(
   comparisonRange: string | undefined,
   activeTimeZone: string,
 ): Interval<true> | undefined {
-  if (!interval || !comparisonRange) return undefined;
+  if (!interval || !comparisonRange || isNewRillTimeFormat(comparisonRange))
+    return undefined;
 
   let comparisonInterval: Interval | undefined = undefined;
 
@@ -348,7 +350,11 @@ export function getComparisonInterval(
     }
   } else {
     const normalizedRange = comparisonRange.replace(",", "/");
-    comparisonInterval = Interval.fromISO(normalizedRange).mapEndpoints((dt) =>
+    const normalizedInterval = Interval.fromISO(normalizedRange);
+    // Safeguard against unknown comparison range format.
+    if (!normalizedInterval.isValid) return undefined;
+
+    comparisonInterval = normalizedInterval.mapEndpoints((dt) =>
       dt.setZone(activeTimeZone),
     );
   }