feat: Vega crossfilter in notebook cells - #488

packages/cells/src/components/VegaCellContent.tsx (3)

88-95: ⚠️ Potential issue | 🟠 Major

Clear this chart's stored selection in the effect cleanup.

The cleanup only removes the Vega listener. When cross-filtering is turned off, the brush field or SQL source changes, or the cell unmounts, crossFilterSelections[sqlId][cellId] survives and sibling charts keep filtering against stale state.

Suggested fix

     return () => {
       if (timerRef.current) clearTimeout(timerRef.current);
       try {
         embed.view.removeSignalListener(BRUSH_PARAM_NAME, handler);
       } catch {
         // View may have been finalized
       }
+      setCrossFilterSelection(cellId, sqlId, null);
     };

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@packages/cells/src/components/VegaCellContent.tsx` around lines 88 - 95, The
effect cleanup currently clears timerRef and removes the Vega listener
(embed.view.removeSignalListener(BRUSH_PARAM_NAME, handler)) but does not clear
the stored cross-filter selection, so stale state remains in
crossFilterSelections[sqlId][cellId]; update the returned cleanup function to
also remove or reset that entry (e.g., delete
crossFilterSelections[sqlId][cellId] or set it to undefined via the same state
updater you use elsewhere) using the sqlId and cellId identifiers so sibling
charts stop filtering against stale selections when the brush/SQL/source changes
or the cell unmounts.

---

63-74: ⚠️ Potential issue | 🟠 Major

Verify the interval payload lookup matches Vega-Lite's channel-based shape.

If this brush is still projected via encodings: ['x'], Line 63 is reading the wrong property. Vega-Lite interval selections projected over encodings expose channel keys like x/y, and the official rectangular-brush example initializes the selection as value: {x: [...], y: [...]}. In that shape, value[brushField] stays undefined, so the store never receives an active selection. (vega.github.io)

In Vega-Lite, for an interval selection parameter defined with `select: { type: 'interval', encodings: ['x'] }`, is the emitted signal value keyed by the encoding channel (`x`) or by the underlying field name?

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@packages/cells/src/components/VegaCellContent.tsx` around lines 63 - 74, The
code in VegaCellContent reads interval payloads by looking up value[brushField],
but Vega-Lite interval selections projected via encodings provide channel-keyed
values (e.g., value.x/value.y), so VegaCellContent must check for both
channel-keyed and field-keyed shapes; update the logic around fieldValue
(currently computed from value[brushField]) to first attempt channel lookup
using brushField (e.g., value.x or value.y) and fall back to the existing field
lookup, and ensure CrossFilterSelection (constructed in the selection creation
code path) receives the interval array whether it came from the channel key or
the field key; adjust any variable references to brushField, brushFieldType and
the selection creation to use the found interval array.

---

189-191: ⚠️ Potential issue | 🟠 Major

Wrap the source query before appending the brush predicate.

Line 190 concatenates WHERE ... directly onto baseSqlQuery, and Lines 176-182 allow that string to be the raw SQL text from the source cell. That breaks queries that already contain WHERE, GROUP BY, ORDER BY, LIMIT, or a trailing semicolon.

Suggested fix

+  const filteredSqlSource = baseSqlQuery.replace(/;+\s*$/, '');
   const selectedSqlQuery = crossFilterPredicate
-    ? `${baseSqlQuery} WHERE ${crossFilterPredicate}`
+    ? `SELECT * FROM (${filteredSqlSource}) AS crossfilter_source WHERE ${crossFilterPredicate}`
     : baseSqlQuery;

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@packages/cells/src/components/VegaCellContent.tsx` around lines 189 - 191,
selectedSqlQuery currently appends "WHERE ..." directly to baseSqlQuery which
can break if baseSqlQuery already contains WHERE/GROUP BY/ORDER BY/LIMIT or a
trailing semicolon; update the construction in VegaCellContent.tsx (symbols:
baseSqlQuery, crossFilterPredicate, selectedSqlQuery) to sanitize and wrap the
source SQL as a subquery (e.g., remove any trailing semicolon, then use `SELECT
* FROM ( <baseSqlQuery> ) AS _source WHERE <crossFilterPredicate>`) so the brush
predicate is always applied safely regardless of the original query's clauses.

packages/cells/src/cellsSlice.ts (1)

118-145: Consider batching cell invalidations to avoid multiple state updates.

The loop calls invalidateCellStatus separately for each affected cell, triggering N set() calls and N re-renders. When many cells depend on changed tables, this could cause performance issues.

♻️ Proposed refactor to batch invalidations

-    store.subscribe((state, prevState) => {
+    store.subscribe((state, prevState) => {
       if (state.db.tables === prevState.db.tables) return;
       const toName = (t: {table: {schema?: string; table: string}}) =>
         `${t.table.schema ?? 'main'}.${t.table.table}`;
       const oldNames = new Set(prevState.db.tables.map(toName));
       const newNames = new Set(state.db.tables.map(toName));
       const changed = new Set<string>();
       for (const n of oldNames) {
         if (!newNames.has(n)) changed.add(n);
       }
       for (const n of newNames) {
         if (!oldNames.has(n)) changed.add(n);
       }
       if (!changed.size) return;
 
+      const cellsToInvalidate: string[] = [];
       for (const sheet of Object.values(state.cells.config.sheets)) {
         const cache = sheet.graphCache;
         if (!cache?.tableDependencies) continue;
         for (const [cellId, tables] of Object.entries(
           cache.tableDependencies,
         )) {
           if (tables.some((t) => changed.has(t))) {
-            state.cells.invalidateCellStatus(cellId);
+            cellsToInvalidate.push(cellId);
           }
         }
       }
+      if (cellsToInvalidate.length) {
+        set((s) =>
+          produce(s, (draft) => {
+            for (const cellId of cellsToInvalidate) {
+              const cell = draft.cells.config.data[cellId];
+              if (!cell) continue;
+              const registryItem = cellRegistry[cell.type];
+              const status = draft.cells.status[cellId];
+              if (status && registryItem?.invalidateStatus) {
+                draft.cells.status[cellId] = registryItem.invalidateStatus(status);
+              }
+            }
+          }),
+        );
+      }
     });

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@packages/cells/src/cellsSlice.ts` around lines 118 - 145, The current
store.subscribe handler loops and calls state.cells.invalidateCellStatus(cellId)
for each affected cell, causing N separate state updates; change this to batch
invalidations by collecting affected cellIds into a Set and then invoking a
single batch method (e.g. add a new cells slice method named
invalidateCellsStatus(cellIds: string[])) from the subscription. Specifically,
inside the subscribe callback (store.subscribe) compute the affectedIds when
table changes (reuse cache.tableDependencies logic), then call
state.cells.invalidateCellsStatus(Array.from(affectedIds)) instead of calling
invalidateCellStatus repeatedly; implement invalidateCellsStatus in the cells
slice to perform one atomic update that marks all provided cellIds stale.

packages/cells/src/components/VegaCellContent.tsx (3)

48-63: ⚠️ Potential issue | 🟠 Major

Use the encoding channel key (x) instead of the field name to access the interval selection range.

The interval selection parameter emits values keyed by the encoding channel name (e.g., x), not by the data field name. Line 48 reads value[brushField], but the payload actually has value.x (or whatever encoding channel is configured). This will always be undefined for typical x-axis brush selections.

🐛 Proposed fix

-        const fieldValue = value[brushField];
+        // Vega interval selections emit values keyed by encoding channel, not field name.
+        // For a typical x-axis brush, the value is in value.x
+        const fieldValue = value.x;

If the brush encoding channel is configurable, you may need to store/pass the encoding channel separately from the field name and use that for lookup.

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@packages/cells/src/components/VegaCellContent.tsx` around lines 48 - 63, The
code is using the data field name (brushField) to read the interval selection
from the Vega event (fieldValue = value[brushField]) but Vega emits interval
values keyed by the encoding channel (e.g., value.x); update the logic to read
the selection using the encoding channel key instead of the field name:
introduce or use an existing variable representing the encoding channel (e.g.,
brushChannel or brushEncoding) to access value[brushChannel], validate it is an
array of length 2, then build the CrossFilterSelection using field: brushField
and fieldType: brushFieldType and call setCrossFilterSelection(cellId, sqlId,
selection) (or null) as before; if the encoding channel can be configured,
ensure it is stored/passed to VegaCellContent so the handler can access it.

---

73-80: ⚠️ Potential issue | 🟠 Major

Clear this chart's stored selection when the listener is torn down.

The cleanup only removes the Vega subscription but doesn't clear the entry from crossFilterSelections[sqlId][cellId]. When this chart disables cross-filter, changes its SQL source, or is removed from the sheet, sibling charts will continue filtering based on the stale selection.

🐛 Proposed fix

     return () => {
       if (timerRef.current) clearTimeout(timerRef.current);
+      setCrossFilterSelection(cellId, sqlId, null);
       try {
         embed.view.removeSignalListener(BRUSH_PARAM_NAME, handler);
       } catch {
         // View may have been finalized
       }
     };

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@packages/cells/src/components/VegaCellContent.tsx` around lines 73 - 80, The
cleanup currently removes the Vega signal listener but fails to clear the stored
cross-filter state; update the teardown inside the returned cleanup function
(near timerRef.current and embed.view.removeSignalListener(BRUSH_PARAM_NAME,
handler)) to also remove/clear crossFilterSelections[sqlId][cellId] (or delete
the cellId key) so that when the view is destroyed, cross-filter state for this
chart is removed; ensure you guard for existence of crossFilterSelections and
sqlId/cellId before modifying and keep this step inside the same try/catch or a
safe conditional to avoid throwing if already missing.

---

152-154: ⚠️ Potential issue | 🟠 Major

Wrap the fallback SQL before appending the brush predicate.

When resultView is unavailable, baseSqlQuery falls back to the raw SQL cell text. Concatenating WHERE ... directly onto that string breaks any query that already has its own WHERE, GROUP BY, ORDER BY, LIMIT, or a trailing semicolon.

🐛 Proposed fix

+  const wrappedBaseSqlQuery = baseSqlQuery.replace(/;+\s*$/, '');
   const selectedSqlQuery = crossFilterPredicate
-    ? `${baseSqlQuery} WHERE ${crossFilterPredicate}`
+    ? `SELECT * FROM (${wrappedBaseSqlQuery}) AS _crossfilter_base WHERE ${crossFilterPredicate}`
     : baseSqlQuery;

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@packages/cells/src/components/VegaCellContent.tsx` around lines 152 - 154,
selectedSqlQuery currently appends "WHERE ..." directly to baseSqlQuery which
breaks when baseSqlQuery is the raw SQL fallback (no resultView) and already
contains WHERE/GROUP/ORDER/LIMIT or trailing semicolons; change the construction
so when resultView is falsy (i.e. using the raw SQL fallback) you wrap the
fallback SQL into a subquery and select from it before applying the
crossFilterPredicate (e.g. build selectedSqlQuery as "SELECT * FROM
(<baseSqlQuery-without-trailing-semicolon>) AS _vf WHERE
<crossFilterPredicate>"), otherwise keep the existing `${baseSqlQuery} WHERE
${crossFilterPredicate}` behavior; update the logic around selectedSqlQuery,
crossFilterPredicate, baseSqlQuery and resultView accordingly.

packages/cells/src/hooks/useVegaCellQuery.ts (1)

37-39: ⚠️ Potential issue | 🟠 Major

Trim trailing semicolons before nesting baseSqlQuery.

baseSqlQuery can still be the raw SQL cell text. If that text ends with ;, Line 38 produces SELECT * FROM (<sql>; ) AS _cells_base ..., which is invalid as a subquery.

Suggested fix

+  const wrappedBaseSqlQuery = baseSqlQuery.replace(/;+\s*$/, '');
   const selectedSqlQuery = crossFilterPredicate
-    ? `SELECT * FROM (${baseSqlQuery}) AS _cells_base WHERE ${crossFilterPredicate}`
+    ? `SELECT * FROM (${wrappedBaseSqlQuery}) AS _cells_base WHERE ${crossFilterPredicate}`
     : baseSqlQuery;

In DuckDB SQL, is `SELECT * FROM (SELECT 1;) AS t` valid, or must the inner query omit the trailing semicolon when it is used as a subquery?

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@packages/cells/src/hooks/useVegaCellQuery.ts` around lines 37 - 39, The code
that builds selectedSqlQuery nests baseSqlQuery directly which can produce an
invalid subquery if baseSqlQuery ends with a trailing semicolon; in the logic
that builds selectedSqlQuery (useVegaCellQuery -> selectedSqlQuery,
crossFilterPredicate, baseSqlQuery) trim any trailing semicolons/whitespace from
baseSqlQuery before wrapping it (e.g. remove trailing ";" characters) so the
nested form becomes `SELECT * FROM (cleanBaseSqlQuery) AS _cells_base WHERE
...`.

packages/cells/src/components/SelectionListener.tsx (1)

46-57: ⚠️ Potential issue | 🟠 Major

Read the brush interval from value.x, not value[brushField].

The brush is projected over encodings: ['x'], so the signal payload is keyed by the channel name. Using the data field name here makes normal selections fall through to the clear path and drops the cross-filter.

Suggested fix

-        const fieldValue = value[brushField];
+        const fieldValue = value.x;

In Vega-Lite, when an interval selection is defined with `select: { type: 'interval', encodings: ['x'] }`, is the emitted parameter/signal payload keyed by `x` or by the encoded field name?

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@packages/cells/src/components/SelectionListener.tsx` around lines 46 - 57,
The code reads the brush interval from value[brushField], but Vega-Lite emits
the interval keyed by the channel name (e.g., 'x') when encodings: ['x'] is
used; change the selection extraction to read the interval from value.x (or the
brush channel key) instead of value[brushField], then keep the same
Array.isArray + length check and call setCrossFilterSelection(cellId, sqlId,
selection) or setCrossFilterSelection(cellId, sqlId, null) when absent; update
the variable referenced (currently fieldValue) where used and preserve
brushFieldType, field, type, and value in the created CrossFilterSelection
object.