fix: scrub for vega tdd charts (#9223)

* fix scrub for vega tdd charts

* use usermeta for better signal matching

* remove injected signals

* directly use vega lite and DOM for scrub

* update comment

Author: djbarnwal

web-common/src/components/vega/VegaLiteRenderer.svelte

@@ -48,6 +48,17 @@
     void viewVL.runAsync();
   }
 
+  // Memoize colorMapping/hasComparison so they don't cause options to change
+  // (which triggers svelte-vega to recreate the entire view, losing brush state).
+  let stableColorMapping: ColorMapping = [];
+  let stableHasComparison = false;
+  $: if (JSON.stringify(colorMapping) !== JSON.stringify(stableColorMapping)) {
+    stableColorMapping = colorMapping;
+  }
+  $: if (hasComparison !== stableHasComparison) {
+    stableHasComparison = hasComparison;
+  }
+
   $: options = createEmbedOptions({
     client: runtimeClient,
     width,
@@ -56,14 +67,14 @@
     renderer,
     themeMode,
     expressionFunctions,
-    colorMapping,
-    hasComparison,
+    colorMapping: stableColorMapping,
+    hasComparison: stableHasComparison,
   });
 
   // Create a more efficient key for component remounting
   $: configKey = config ? Object.keys(config).sort().join(",") : "default";
   $: colorMappingKey =
-    colorMapping?.map((m) => `${m.value}:${m.color}`)?.join("|") ?? "";
+    stableColorMapping?.map((m) => `${m.value}:${m.color}`)?.join("|") ?? "";
   $: componentKey = `${themeMode}-${configKey}-${colorMappingKey}`;
 
   const onError = (e: Error) => {

web-common/src/components/vega/vega-signals.ts

@@ -32,24 +32,26 @@ export function resolveSignalTimeField(value: unknown, temporalField?: string) {
 export function resolveSignalIntervalField(
   value: unknown,
 ): { start: Date; end: Date } | undefined {
+  const checkAndCreateTimeRange = (
+    arr: unknown,
+  ): { start: Date; end: Date } | undefined => {
+    if (Array.isArray(arr) && arr.length === 2) {
+      const [start, end] = arr;
+      return { start: new Date(start), end: new Date(end) };
+    }
+    return undefined;
+  };
+
+  // Handle raw [date1, date2] array (e.g. from view.signal() on a brush temporal signal)
+  if (Array.isArray(value)) {
+    return checkAndCreateTimeRange(value);
+  }
+
   /**
    * Time range fields can be either 'ts' or end with '_ts'
    * We check for both cases and return a TimeRange if a valid array of two timestamps is found.
    */
   if (typeof value === "object" && value !== null) {
-    const checkAndCreateTimeRange = (
-      arr: unknown,
-    ): { start: Date; end: Date } | undefined => {
-      if (Array.isArray(arr) && arr.length === 2) {
-        const [start, end] = arr;
-        return {
-          start: new Date(start),
-          end: new Date(end),
-        };
-      }
-      return undefined;
-    };
-
     // Check for 'ts' key first
     if ("ts" in value) {
       return checkAndCreateTimeRange(value["ts"]);

web-common/src/features/components/charts/Chart.svelte

@@ -7,7 +7,6 @@
     resolveSignalTimeField,
   } from "@rilldata/web-common/components/vega/vega-signals";
   import VegaLiteRenderer from "@rilldata/web-common/components/vega/VegaLiteRenderer.svelte";
-  import VegaRenderer from "@rilldata/web-common/components/vega/VegaRenderer.svelte";
   import type { CanvasChartSpec } from "@rilldata/web-common/features/canvas/components/charts";
   import ComponentError from "@rilldata/web-common/features/components/ComponentError.svelte";
   import Spinner from "@rilldata/web-common/features/entity-management/Spinner.svelte";
@@ -20,13 +19,10 @@
   import type { TimeRange } from "@rilldata/web-common/lib/time/types";
   import type { MetricsViewSpecMeasure } from "@rilldata/web-common/runtime-client";
   import { onDestroy } from "svelte";
-  import type { SignalListeners, VegaSpec, View } from "svelte-vega";
+  import type { SignalListeners, View } from "svelte-vega";
   import type { Readable } from "svelte/store";
   import { getChroma } from "../../themes/theme-utils";
-  import {
-    compileToBrushedVegaSpec,
-    createAdaptiveScrubHandler,
-  } from "./brush-builder";
+  import { discoverTemporalBrushSignal } from "./brush-builder";
   import type { ChartDataResult, ChartType } from "./types";
   import { generateSpec, getColorMappingForChart } from "./util";
 
@@ -42,9 +38,7 @@
   export let theme: Record<string, string> | undefined = undefined;
   export let isCanvas: boolean;
 
-  export let isScrubbing: boolean = false;
   export let temporalField: string | undefined = undefined;
-  export let onBrush: ((interval: TimeRange) => void) | undefined = undefined;
   export let onBrushEnd: ((interval: TimeRange) => void) | undefined =
     undefined;
   export let onBrushClear: (() => void) | undefined = undefined;
@@ -54,10 +48,6 @@
 
   export let view: View;
 
-  let vegaSpec: VegaSpec | undefined = undefined;
-  let prevVlSpec: unknown = undefined;
-  let compileGeneration = 0;
-
   $: ({ data, domainValues, hasComparison, isFetching, error } = $chartData);
 
   $: hasNoData = !isFetching && data.length === 0;
@@ -77,28 +67,23 @@
       }
     : $chartData;
 
-  $: spec = generateSpec(chartType, chartSpec, chartDataWithTheme);
+  $: rawSpec = generateSpec(chartType, chartSpec, chartDataWithTheme);
 
-  // Compile VL spec to Vega spec when brush is enabled.
-  // Memoize with deep equality to avoid recompilation on store re-emissions
-  // that produce the same spec, which would reset brush selection state.
-  $: useBrush = "isInteractive" in chartSpec && !!chartSpec.isInteractive;
-  $: {
-    if (
-      useBrush &&
-      spec &&
-      JSON.stringify(spec) !== JSON.stringify(prevVlSpec)
-    ) {
-      prevVlSpec = spec;
-      const gen = ++compileGeneration;
-      void compileToBrushedVegaSpec(spec, isThemeModeDark, theme).then(
-        (compiled) => {
-          if (gen === compileGeneration) vegaSpec = compiled;
-        },
-      );
-    }
+  // Memoize spec with deep equality so VegaLiteRenderer doesn't recreate the
+  // view (and kill brush state) on store re-emissions that produce the same spec.
+  let spec: ReturnType<typeof generateSpec> = {};
+  $: if (JSON.stringify(rawSpec) !== JSON.stringify(spec)) {
+    spec = rawSpec;
   }
 
+  $: useBrush = "isInteractive" in chartSpec && !!chartSpec.isInteractive;
+
+  // Read brushTemporalField from the VL spec's usermeta (set by spec generators)
+  $: brushTemporalField =
+    spec && typeof spec === "object" && "usermeta" in spec
+      ? (spec.usermeta as { brushTemporalField?: string })?.brushTemporalField
+      : undefined;
+
   // TODO: Move this to a central cached store
   $: measureFormatters = measures.reduce(
     (acc, measure) => ({
@@ -135,58 +120,84 @@
     isThemeModeDark,
   );
 
-  const scrubHandler = createAdaptiveScrubHandler((interval) =>
-    onBrush?.(interval),
-  );
-  onDestroy(() => scrubHandler.destroy());
-
-  // Signal listeners for brush and hover events
-  $: signalListeners = buildSignalListeners(
-    useBrush && !!vegaSpec,
-    !!onHover,
-    temporalField,
-  );
+  // Hover signal listeners (passed declaratively to VegaLiteRenderer)
+  $: signalListeners = buildHoverListeners(!!onHover, temporalField);
 
-  function buildSignalListeners(
-    brushEnabled: boolean,
+  function buildHoverListeners(
     hoverEnabled: boolean,
     timeField?: string,
   ): SignalListeners {
     const listeners: SignalListeners = {};
-
     if (hoverEnabled) {
       listeners.hover = (_name: string, value: unknown) => {
         const dimension = resolveSignalField(value, "dimension");
         const ts = resolveSignalTimeField(value, timeField);
         onHover?.(dimension, ts);
       };
     }
+    return listeners;
+  }
 
-    if (brushEnabled) {
-      listeners.brush = (_name: string, value: unknown) => {
-        const interval = resolveSignalIntervalField(value);
-        // Trigger async rendering to prevent race condition
-        void view?.runAsync();
-        if (interval) scrubHandler.update(interval);
-      };
+  // Brush-end and brush-clear detection.
+  // The temporal brush signal is discovered from the live view because its name
+  // includes a timeUnit prefix that varies (e.g. brush_yearmonthdatehours___time).
+  let pointerUpHandler: (() => void) | undefined;
+  let clearHandler: ((name: string, value: unknown) => void) | undefined;
+  let currentBrushSignal: string | undefined;
+
+  function attachBrushListener(v: View) {
+    detachBrushListener();
+
+    const signalName = discoverTemporalBrushSignal(v, brushTemporalField);
+    if (!signalName) return;
+    currentBrushSignal = signalName;
 
-      listeners.brush_end = (_name: string, value: unknown) => {
+    // Detect brush-end via DOM pointerup
+    pointerUpHandler = () => {
+      try {
+        const value = v.signal(signalName);
         const interval = resolveSignalIntervalField(value);
         if (interval) {
           onBrushEnd?.(interval);
-        } else {
-          // Brush was cleared by clicking outside the selection
-          onBrushClear?.();
         }
-      };
+      } catch {
+        // view may have been finalized
+      }
+    };
+    window.addEventListener("pointerup", pointerUpHandler);
 
-      listeners.brush_clear = (_name: string, value: unknown) => {
-        if (value) onBrushClear?.();
-      };
+    // Detect brush-clear (user clicks outside brush or double-clicks)
+    clearHandler = (_name: string, value: unknown) => {
+      if (value === null || value === undefined) {
+        onBrushClear?.();
+      }
+    };
+    v.addSignalListener(signalName, clearHandler);
+  }
+
+  function detachBrushListener() {
+    if (pointerUpHandler) {
+      window.removeEventListener("pointerup", pointerUpHandler);
+      pointerUpHandler = undefined;
+    }
+    if (view && currentBrushSignal && clearHandler) {
+      try {
+        view.removeSignalListener(currentBrushSignal, clearHandler);
+      } catch {
+        // view may have been finalized
+      }
     }
+    clearHandler = undefined;
+    currentBrushSignal = undefined;
+  }
 
-    return listeners;
+  $: if (useBrush && view) {
+    attachBrushListener(view);
   }
+
+  onDestroy(() => {
+    detachBrushListener();
+  });
 </script>
 
 {#if isFetching || measures.length === 0}
@@ -201,19 +212,6 @@
   >
     No Data to Display
   </div>
-{:else if useBrush && vegaSpec}
-  <VegaRenderer
-    bind:view
-    data={{ "metrics-view": data }}
-    {isScrubbing}
-    spec={vegaSpec}
-    {colorMapping}
-    theme={themeMode}
-    {signalListeners}
-    renderer="svg"
-    {expressionFunctions}
-    {hasComparison}
-  />
 {:else}
   <VegaLiteRenderer
     bind:viewVL={view}
@@ -223,7 +221,7 @@
     {spec}
     {colorMapping}
     {signalListeners}
-    renderer="canvas"
+    renderer={useBrush ? "svg" : "canvas"}
     {expressionFunctions}
     {hasComparison}
     config={getRillTheme(isThemeModeDark, theme)}