feat: support brush selection in vega-lite chart - #689

packages/vega/src/brushMappingUtils.ts (2)

61-80: ⚡ Quick win

SQL string escaping is correct; static analysis flags are false positives.

The escapeSqlLiteral function properly doubles single quotes (SQL standard), and json_serialize_sql is used for parsing, not execution. The table name derived from the parsed AST is then properly identifier-quoted (line 246). However, this pattern assumes the chart SQL comes from a trusted source (the LLM tool).

Consider adding a comment documenting the trust boundary:

/**
 * Parse a single SQL SELECT statement into DuckDB's JSON AST. Returns `null`
 * when the SQL is invalid or does not contain a SELECT statement.
 * 
 * `@param` sql - Chart SQL from the Vega tool (trusted LLM output)
 */

🤖 Prompt for AI Agents

Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@packages/vega/src/brushMappingUtils.ts` around lines 61 - 80, The
parseSelectStatement function currently uses escapeSqlLiteral and
json_serialize_sql to parse chart SQL but the trust boundary isn’t documented;
add a JSDoc comment immediately above parseSelectStatement explaining it parses
a single SQL SELECT into DuckDB’s JSON AST, returns null on invalid/non-SELECT
SQL, and that the input sql is expected to be trusted chart SQL produced by the
Vega/LLM tool (note that escapeSqlLiteral doubles single quotes and identifiers
are quoted later); reference parseSelectStatement, escapeSqlLiteral, and
json_serialize_sql in the comment for clarity.

---

346-350: 💤 Low value

Consider optimizing Set intersection for large row sets.

The current approach creates a new Set and spreads arrays on each iteration. For large row sets, a mutating approach would be more efficient:

⚡ Optional optimization

 let intersection = new Set(fieldResults[0]!);
 for (let i = 1; i < fieldResults.length; i++) {
   const next = new Set(fieldResults[i]!);
-  intersection = new Set([...intersection].filter((x) => next.has(x)));
+  for (const x of intersection) {
+    if (!next.has(x)) intersection.delete(x);
+  }
 }
 return [...intersection];

The current code is correct and readable; this optimization is only beneficial if brush mappings involve thousands of rows across multiple fields.

🤖 Prompt for AI Agents

Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@packages/vega/src/brushMappingUtils.ts` around lines 346 - 350, The current
Set intersection in brushMappingUtils.ts creates new Sets and spreads elements
on each loop (variables: intersection, fieldResults), which is inefficient for
very large row sets; change to an in-place/mutating intersection: initialize
intersection from the smallest fieldResults[i] (choose the smallest array length
to start), then for each subsequent fieldResults create a Set or use its Set
view and iterate over intersection with for...of, deleting entries not present
in the next set (i.e., if (!next.has(val)) intersection.delete(val)); this
avoids allocating a new Set on every iteration and reduces work for large
datasets while preserving correctness.

packages/kepler/src/components/KeplerVegaChartToolResult.tsx (2)

59-61: ⚖️ Poor tradeoff

Verify selector stability to avoid unnecessary effect re-runs.

The getConnector selector at line 59-61 is used as a dependency in the effect at lines 66-85. Per coding guidelines, Zustand selectors should select stable raw state first rather than derived values. If state.db.getConnector changes reference on store updates (even when the function itself hasn't changed), this effect will cancel and re-run unnecessarily.

Consider verifying that state.db.getConnector is a stable reference in the store implementation. If not, you may need custom equality or selector refactoring.

As per coding guidelines: "When using Zustand or room-store selectors in React, do not create derived arrays, objects, or functions inside the selector; instead select stable raw state first and derive values with React.useMemo"

Also applies to: 66-85

🤖 Prompt for AI Agents

Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@packages/kepler/src/components/KeplerVegaChartToolResult.tsx` around lines 59
- 61, The selector passed to useStoreWithKepler pulls a derived function
(state.db.getConnector) which may change reference and cause the effect that
depends on getConnector to re-run; change the selector to select a stable raw
value (e.g., select state.db or another primitive stable property) and then
derive the getConnector function inside the component with React.useMemo or
useCallback before using it in the effect, or supply a custom equality
comparator to useStoreWithKepler to compare by identity; update references to
getConnector in the effect that currently uses getConnector so they use the
memoized/derived function instead.

---

17-39: ⚖️ Poor tradeoff

Kepler.gl doesn’t provide a public action to set dataset.filteredIndex, so this mutation in highlightRows is likely the intended workaround

Kepler’s filtering is managed via visState/filters, and there’s no setFilteredIndex/updateFilteredIndex-style action available in the API. Given that, the direct dataset.filteredIndex = ... mutation followed by layer.formatLayerData(datasets) and layerSetIsValid is a reasonable approach here. The main improvement would be to add an explicit comment in highlightRows explaining why filteredIndex must be set directly (and why it can’t be replaced with setFilter/related actions).

🤖 Prompt for AI Agents

Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@packages/kepler/src/components/KeplerVegaChartToolResult.tsx` around lines 17
- 39, The function highlightRows directly mutates dataset.filteredIndex (in
highlightRows) because Kepler.gl exposes no public action to set filteredIndex
via visState/filters; add a concise inline comment above the assignment to
explain this explicit mutation as an intentional workaround (mention that there
is no setFilteredIndex/setFilter action available and that we call
layer.formatLayerData(datasets) and dispatchLayerSetIsValid(layer, true)
afterwards to refresh layers), so future readers understand why we must set
dataset.filteredIndex directly instead of using visState actions.

Fix all unresolved CodeRabbit comments on this PR:

• Push a commit to this branch (recommended)
• Create a new PR with the fixes

packages/vega/src/VegaLiteArrowChart.tsx (1)

100-144: 💤 Low value

mode property leaks into Vega-Lite spec params.

buildBrushParam returns {name, select, mode} where mode is only used internally for selecting the signal handler type. However, the entire object is added to params at line 240, including mode which isn't a valid Vega-Lite param property.

While Vega-Lite ignores unknown properties, consider stripping mode when injecting the param:

♻️ Suggested fix

 if (onBrushSelection) {
   const brushParam = buildBrushParam(base as Record<string, unknown>);
   if (brushParam) {
+    const {mode: _, ...paramForSpec} = brushParam;
     const existingParams = Array.isArray(base.params) ? base.params : [];
     const hasBrush = existingParams.some(
       (p: {name?: string}) => p.name === BRUSH_PARAM_NAME,
     );
     if (!hasBrush) {
-      base.params = [...existingParams, brushParam];
+      base.params = [...existingParams, paramForSpec];
     }
   }
 }

Also applies to: 232-243

🤖 Prompt for AI Agents

Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@packages/vega/src/VegaLiteArrowChart.tsx` around lines 100 - 144,
buildBrushParam currently returns an object containing a mode property which is
internal and gets injected into the Vega-Lite params (via the code that pushes
the returned object into params), leaking an invalid property; change the
implementation to keep mode internal by returning only {name, select} from
buildBrushParam (or keep buildBrushParam as-is but strip mode before adding to
the params array), and surface the mode separately to the code that installs the
signal handler (use BRUSH_PARAM_NAME to map back to the mode when needed) so
that the object pushed into params contains no mode field.