Repository navigation
feat: save chart cell automatically and other improvements - #502
Merged
Merged
Conversation
Contributor
📝 WalkthroughWalkthroughAdds selector/icon React components, a Vega‑Lite spec builder module, refactors Vega cell UI to use the new selectors and persist spec changes via debounced hooks, introduces debouncing utilities in UI package, and adds a Vega cell header component for data-source switching. Changes
Sequence Diagram(s)sequenceDiagram
participant User as User
participant Panel as VegaConfigPanel
participant Selects as Selector Components
participant Builder as vegaSpecBuilder
participant Store as Cells Store
participant Cell as VegaCellContent
participant Chart as VegaLiteChart
User->>Panel: open config / change selector
Panel->>Selects: render/select value
Selects-->>Panel: onValueChange(value)
Panel->>Builder: buildFlatSpec / buildCrossFilterSpec(opts)
Builder-->>Panel: vegaSpec
Panel->>Store: updateCell(cellId, { data.vegaSpec: vegaSpec }) (debounced)
Store-->>Cell: cell data updated
Cell->>Chart: render with new vegaSpec
Chart-->>User: updated visualization
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Possibly related PRs
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 3✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
packages/cells/src/components/VegaCellContent.tsx (1)
148-156:⚠️ Potential issue | 🟠 MajorAdd an accessible name to the icon-only settings button.
The button only renders an icon, so assistive tech has no descriptive label.
♿ Proposed fix
<Button size="xs" variant="secondary" className="h-6" + aria-label={isEditing ? 'Close chart settings' : 'Open chart settings'} onClick={() => setIsEditing((prev) => !prev)} disabled={!hasDataSource} > <Settings className="h-4 w-4" /> </Button>🤖 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 148 - 156, The Settings icon-only Button lacks an accessible name; update the Button in VegaCellContent (the Button that toggles setIsEditing and uses hasDataSource) to provide an accessible label for screen readers by adding an appropriate aria-label (e.g., aria-label="Settings" or a localized string) or include visually hidden text inside the Button; ensure the label clearly describes the action (like "Edit settings") so assistive tech can announce it.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@packages/cells/src/components/VegaCellContent.tsx`:
- Around line 131-140: handleSpecChange currently calls updateCell on every
keystroke/interaction; debounce the writes to prevent expensive update bursts by
wrapping the updateCell call in a debounced function (e.g., using
lodash.debounce or a custom setTimeout stored in a ref) inside the
VegaCellContent component. Create a debouncedUpdate (referencing updateCell and
id) and call that from handleSpecChange (keep handleSpecChange as a thin
wrapper), ensure the debounce is stable via useCallback/useRef and that you
cancel/flush the debounced timer on unmount (useEffect cleanup) so no stale
updates occur. Ensure dependencies include id and updateCell for correct
behavior.
---
Outside diff comments:
In `@packages/cells/src/components/VegaCellContent.tsx`:
- Around line 148-156: The Settings icon-only Button lacks an accessible name;
update the Button in VegaCellContent (the Button that toggles setIsEditing and
uses hasDataSource) to provide an accessible label for screen readers by adding
an appropriate aria-label (e.g., aria-label="Settings" or a localized string) or
include visually hidden text inside the Button; ensure the label clearly
describes the action (like "Edit settings") so assistive tech can announce it.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: e7875071-209b-4b2e-bc7f-a77e0631003d
📒 Files selected for processing (6)
packages/cells/src/components/ColorSelector.tsxpackages/cells/src/components/FieldSelector.tsxpackages/cells/src/components/FieldTypeIcon.tsxpackages/cells/src/components/VegaCellContent.tsxpackages/cells/src/components/VegaConfigPanel.tsxpackages/cells/src/vegaSpecBuilder.ts
Contributor
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@packages/cells/src/components/AggregationSelector.tsx`:
- Around line 25-27: The current fallback for selectedAggregation silently
coerces unknown persisted values to aggregationOptions[0]; instead, when
aggregationOptions.find((opt) => opt.value === value) returns undefined,
preserve the original persisted value by creating a transient option (e.g. {
value, label: String(value) }) or set selectedAggregation to undefined/empty so
the selector displays the actual stored value and does not save a different
default; update the AggregationSelector logic that computes selectedAggregation
and ensure the selector's change/save handlers only write when the user picks a
new known option.
In `@packages/cells/src/components/ChartTypeSelector.tsx`:
- Around line 25-31: The code currently coerces any unrecognized incoming value
to chartTypeOptions[0] via selectedChartType, which will silently overwrite
persisted/unsupported marks; instead locate the matching option with
chartTypeOptions.find((opt) => opt.value === value) but do not default to
chartTypeOptions[0] — pass the original value through to the Select (and render
a fallback IconComponent or null when no match is found) so unknown persisted
values are preserved; ensure onValueChange still accepts and emits the new
value. Reference: selectedChartType, chartTypeOptions, value, IconComponent,
onValueChange, Select.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 826e1323-e51a-4fc9-91e9-1a24a8e20ef6
📒 Files selected for processing (3)
packages/cells/src/components/AggregationSelector.tsxpackages/cells/src/components/ChartTypeSelector.tsxpackages/cells/src/components/VegaConfigPanel.tsx
🚧 Files skipped from review as they are similar to previous changes (1)
- packages/cells/src/components/VegaConfigPanel.tsx
Comment on lines
+25
to
+27
| const selectedAggregation = | ||
| aggregationOptions.find((opt) => opt.value === value) ?? | ||
| aggregationOptions[0]; |
Contributor
There was a problem hiding this comment.
Do not silently coerce unknown aggregation values to sum.
On Line 25, the fallback to aggregationOptions[0] masks persisted values not in this local list, so the selector can display/save a different aggregate than what is currently stored in the spec.
Suggested fix
const aggregationOptions = [
{value: 'sum', label: 'Sum'},
{value: 'mean', label: 'Mean'},
{value: 'count', label: 'Count'},
] as const;
+const defaultAggregation = aggregationOptions[0].value;
interface AggregationSelectorProps {
value?: string;
onValueChange: (value: string) => void;
}
export const AggregationSelector: React.FC<AggregationSelectorProps> = ({
value,
onValueChange,
}) => {
- const selectedAggregation =
- aggregationOptions.find((opt) => opt.value === value) ??
- aggregationOptions[0];
+ const options = React.useMemo(() => {
+ if (!value) return aggregationOptions;
+ return aggregationOptions.some((opt) => opt.value === value)
+ ? aggregationOptions
+ : [{value, label: value}, ...aggregationOptions];
+ }, [value]);
+
+ const selectedValue = value ?? defaultAggregation;
return (
- <Select value={selectedAggregation.value} onValueChange={onValueChange}>
+ <Select value={selectedValue} onValueChange={onValueChange}>
<SelectTrigger className="h-8 text-xs">
<SelectValue />
</SelectTrigger>
<SelectContent onCloseAutoFocus={(e) => e.preventDefault()}>
- {aggregationOptions.map((option) => (
+ {options.map((option) => (
<SelectItem key={option.value} value={option.value}>
{option.label}
</SelectItem>
))}
</SelectContent>
</Select>
);
};🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@packages/cells/src/components/AggregationSelector.tsx` around lines 25 - 27,
The current fallback for selectedAggregation silently coerces unknown persisted
values to aggregationOptions[0]; instead, when aggregationOptions.find((opt) =>
opt.value === value) returns undefined, preserve the original persisted value by
creating a transient option (e.g. { value, label: String(value) }) or set
selectedAggregation to undefined/empty so the selector displays the actual
stored value and does not save a different default; update the
AggregationSelector logic that computes selectedAggregation and ensure the
selector's change/save handlers only write when the user picks a new known
option.
Comment on lines
+25
to
+31
| const selectedChartType = | ||
| chartTypeOptions.find((opt) => opt.value === value) ?? chartTypeOptions[0]; | ||
|
|
||
| const IconComponent = selectedChartType.icon; | ||
|
|
||
| return ( | ||
| <Select value={selectedChartType.value} onValueChange={onValueChange}> |
Contributor
There was a problem hiding this comment.
Avoid coercing unknown persisted marks to bar in the controlled value.
Line 25 and Line 31 force any unrecognized value to bar. With the rebuild flow in packages/cells/src/components/VegaConfigPanel.tsx (Line 68–74), that can silently overwrite previously saved unsupported marks after subsequent config edits.
💡 Proposed fix
- const selectedChartType =
- chartTypeOptions.find((opt) => opt.value === value) ?? chartTypeOptions[0];
-
- const IconComponent = selectedChartType.icon;
+ const selectedChartType = chartTypeOptions.find((opt) => opt.value === value);
+ const selectValue =
+ value === undefined ? chartTypeOptions[0].value : selectedChartType?.value;
+ const IconComponent = selectedChartType?.icon;
return (
- <Select value={selectedChartType.value} onValueChange={onValueChange}>
+ <Select value={selectValue} onValueChange={onValueChange}>
<SelectTrigger className="h-8 text-xs">
<SelectValue placeholder="Select chart type">
- <div className="flex items-center gap-2">
- <IconComponent className="h-4 w-4" />
- <span>{selectedChartType.label}</span>
- </div>
+ {selectedChartType ? (
+ <div className="flex items-center gap-2">
+ <IconComponent className="h-4 w-4" />
+ <span>{selectedChartType.label}</span>
+ </div>
+ ) : null}
</SelectValue>
</SelectTrigger>🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@packages/cells/src/components/ChartTypeSelector.tsx` around lines 25 - 31,
The code currently coerces any unrecognized incoming value to
chartTypeOptions[0] via selectedChartType, which will silently overwrite
persisted/unsupported marks; instead locate the matching option with
chartTypeOptions.find((opt) => opt.value === value) but do not default to
chartTypeOptions[0] — pass the original value through to the Select (and render
a fallback IconComponent or null when no match is found) so unknown persisted
values are preserved; ensure onValueChange still accepts and emits the new
value. Reference: selectedChartType, chartTypeOptions, value, IconComponent,
onValueChange, Select.
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
packages/cells/src/components/VegaCellContent.tsx (1)
151-159:⚠️ Potential issue | 🟠 MajorAdd an accessible name to the icon-only settings button.
The button currently has no text/ARIA label, so assistive tech users won’t get its purpose.
♿ Suggested fix
<Button size="xs" variant="secondary" className="h-6" onClick={() => setIsEditing((prev) => !prev)} disabled={!hasDataSource} + aria-label={isEditing ? 'Close chart settings' : 'Open chart settings'} + title={isEditing ? 'Close chart settings' : 'Open chart settings'} > <Settings className="h-4 w-4" /> </Button>🤖 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 151 - 159, The settings Button (the icon-only Button that toggles setIsEditing and is disabled by hasDataSource) lacks an accessible name; add an aria-label (or a visually-hidden text child) to the Button so screen readers know its purpose (e.g., aria-label="Cell settings" or similar) and ensure the label accurately reflects the action toggled by setIsEditing; keep the Settings icon as-is and do not remove the disabled prop.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@packages/cells/src/components/VegaCellContent.tsx`:
- Around line 45-49: VegaCellContent currently reads vegaSpec from
cell.data.vegaSpec and calls updateCell via a debounced path which can cause
stale reads to overwrite recent rapid edits; fix by keeping a local live spec
state (e.g., useState called localSpec initialized from cell.data.vegaSpec or
default) and update localSpec synchronously on control changes, then call the
debounced updateCell only to persist localSpec to the store; update the handlers
that currently reference vegaSpec and call updateCell (symbols: vegaSpec,
cell.data.vegaSpec, updateCell, VegaCellContent) to use localSpec for immediate
UI changes and the debounced persistence for store updates, and apply this same
local-state + debounce-only-persist pattern to the other spots noted where
updateCell reads the store spec.
---
Outside diff comments:
In `@packages/cells/src/components/VegaCellContent.tsx`:
- Around line 151-159: The settings Button (the icon-only Button that toggles
setIsEditing and is disabled by hasDataSource) lacks an accessible name; add an
aria-label (or a visually-hidden text child) to the Button so screen readers
know its purpose (e.g., aria-label="Cell settings" or similar) and ensure the
label accurately reflects the action toggled by setIsEditing; keep the Settings
icon as-is and do not remove the disabled prop.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 0b96d816-2a22-40df-b0cc-09c7e3e3fbfb
📒 Files selected for processing (4)
packages/cells/src/components/VegaCellContent.tsxpackages/ui/src/hooks/useDebounce.tspackages/ui/src/hooks/useDebouncedCallback.tspackages/ui/src/index.ts
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (2)
packages/cells/src/components/VegaCellHeader.tsx (1)
29-48: Consider wrappinghandleDataSourceChangeinuseCallback.This handler is recreated on every render since it's not memoized. While functionally correct, this could cause unnecessary re-renders in
CellSourceSelectorif it's memoized or uses the callback in its own dependency arrays.♻️ Proposed fix
+import {useCallback} from 'react'; ... - const handleDataSourceChange = (value: string) => { + const handleDataSourceChange = useCallback((value: string) => { updateCell(cellId, (c) => produce(c, (draft) => { if (draft.type !== 'vega') { return; } const cellIdFromValue = fromDataSourceCell(value); const tableRef = fromDataSourceTable(value); if (cellIdFromValue) { draft.data.sqlId = cellIdFromValue; delete draft.data.tableRef; } else if (tableRef) { draft.data.tableRef = tableRef; delete draft.data.sqlId; } }), ); - }; + }, [cellId, updateCell]);🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/cells/src/components/VegaCellHeader.tsx` around lines 29 - 48, The handler handleDataSourceChange is recreated each render; wrap it in useCallback to stabilize its identity for downstream memoized components like CellSourceSelector. Import React's useCallback and change const handleDataSourceChange = (value: string) => { ... } to const handleDataSourceChange = useCallback((value: string) => { updateCell(cellId, (c) => produce(c, (draft) => { /* same body using fromDataSourceCell and fromDataSourceTable, update draft.data.sqlId/tableRef */ })); }, [updateCell, cellId]); ensuring the dependency array includes any props or functions used (e.g., updateCell and cellId).packages/ui/src/hooks/useDebouncedValue.ts (1)
33-36: Potential re-render loop ifexternalValueis a new object reference each render.The sync effect depends on
externalValuedirectly. If the consumer passes an inline object/array (e.g.,cell.data.vegaSpec ?? { ... }), every render creates a new reference, triggering the effect repeatedly even when values are semantically equal.Consumers should memoize
externalValueor this hook should optionally accept a custom equality check. The current usage inVegaCellContentappears safe sincecell.data.vegaSpeccomes from the store, but document this requirement to prevent misuse.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/ui/src/hooks/useDebouncedValue.ts` around lines 33 - 36, The effect in useDebouncedValue synchronizes localValue every time externalValue reference changes, causing potential re-render loops when callers pass inline objects/arrays; update useDebouncedValue to accept an optional equality comparator (e.g., isEqual?: (a,b)=>boolean) and change the sync effect to compare previousExternalRef.current to externalValue using that comparator before calling setLocalValue, storing the last external value in a ref (previousExternalRef) to avoid blind updates; provide a sensible default comparator (shallow/deep via JSON.stringify or import isEqual) and update the hook's TypeScript signature and JSDoc so callers (like VegaCellContent) either memoize externalValue or pass a comparator.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@packages/ui/src/hooks/useDebouncedValue.ts`:
- Around line 38-51: The debounced onPersist call from useDebouncedCallback
(used as debouncedPersist in useDebouncedValue) is cancelled on unmount which
can lose recent edits; modify useDebouncedValue to either (a) accept an option
like flushOnUnmount or expose the debounced flush function and call it in a
cleanup, or (b) return a third element flush() alongside [localValue, setValue]
that calls debouncedPersist.flush(); update the hook to reference
useDebouncedCallback, debouncedPersist, setValue and onPersist so callers can
flush pending persistence in their own cleanup or have the hook auto-flush on
unmount when the option is enabled.
---
Nitpick comments:
In `@packages/cells/src/components/VegaCellHeader.tsx`:
- Around line 29-48: The handler handleDataSourceChange is recreated each
render; wrap it in useCallback to stabilize its identity for downstream memoized
components like CellSourceSelector. Import React's useCallback and change const
handleDataSourceChange = (value: string) => { ... } to const
handleDataSourceChange = useCallback((value: string) => { updateCell(cellId, (c)
=> produce(c, (draft) => { /* same body using fromDataSourceCell and
fromDataSourceTable, update draft.data.sqlId/tableRef */ })); }, [updateCell,
cellId]); ensuring the dependency array includes any props or functions used
(e.g., updateCell and cellId).
In `@packages/ui/src/hooks/useDebouncedValue.ts`:
- Around line 33-36: The effect in useDebouncedValue synchronizes localValue
every time externalValue reference changes, causing potential re-render loops
when callers pass inline objects/arrays; update useDebouncedValue to accept an
optional equality comparator (e.g., isEqual?: (a,b)=>boolean) and change the
sync effect to compare previousExternalRef.current to externalValue using that
comparator before calling setLocalValue, storing the last external value in a
ref (previousExternalRef) to avoid blind updates; provide a sensible default
comparator (shallow/deep via JSON.stringify or import isEqual) and update the
hook's TypeScript signature and JSDoc so callers (like VegaCellContent) either
memoize externalValue or pass a comparator.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 0356a1a4-3435-4336-8e4e-5571d5377e98
📒 Files selected for processing (4)
packages/cells/src/components/VegaCellContent.tsxpackages/cells/src/components/VegaCellHeader.tsxpackages/ui/src/hooks/useDebouncedValue.tspackages/ui/src/index.ts
🚧 Files skipped from review as they are similar to previous changes (1)
- packages/ui/src/index.ts
Comment on lines
+38
to
+51
| // Debounced persistence | ||
| const debouncedPersist = useDebouncedCallback(onPersist, debounceMs); | ||
|
|
||
| // Combined setter: immediate local update + debounced persist | ||
| const setValue = useCallback( | ||
| (value: T) => { | ||
| setLocalValue(value); | ||
| debouncedPersist(value); | ||
| }, | ||
| [debouncedPersist], | ||
| ); | ||
|
|
||
| return [localValue, setValue]; | ||
| } |
Contributor
There was a problem hiding this comment.
Pending changes may be lost on unmount.
Per the cleanup logic in useDebouncedCallback (lines 58-65 in that file), when the component unmounts before the debounce delay expires, the pending onPersist call is cancelled rather than flushed. This means edits made within the final 300ms window before unmount will be permanently lost.
Consider adding an optional flush capability or documenting this limitation clearly. Alternatively, expose a flush() method that consumers can call in their own cleanup effects when data loss is unacceptable.
💡 Potential fix: Add flush-on-unmount option
export function useDebouncedValue<T>(
externalValue: T,
onPersist: (value: T) => void,
debounceMs = 300,
+ { flushOnUnmount = false }: { flushOnUnmount?: boolean } = {},
): [T, (value: T) => void] {
// Local state for immediate UI updates
const [localValue, setLocalValue] = useState<T>(externalValue);
+ const localValueRef = useRef(localValue);
+ const hasPendingRef = useRef(false);
// Sync local state when external value changes
useEffect(() => {
setLocalValue(externalValue);
}, [externalValue]);
// Debounced persistence
const debouncedPersist = useDebouncedCallback(onPersist, debounceMs);
// Combined setter: immediate local update + debounced persist
const setValue = useCallback(
(value: T) => {
setLocalValue(value);
+ localValueRef.current = value;
+ hasPendingRef.current = true;
debouncedPersist(value);
},
[debouncedPersist],
);
+ // Flush on unmount if enabled
+ useEffect(() => {
+ return () => {
+ if (flushOnUnmount && hasPendingRef.current) {
+ onPersist(localValueRef.current);
+ }
+ };
+ }, [flushOnUnmount, onPersist]);
return [localValue, setValue];
}🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@packages/ui/src/hooks/useDebouncedValue.ts` around lines 38 - 51, The
debounced onPersist call from useDebouncedCallback (used as debouncedPersist in
useDebouncedValue) is cancelled on unmount which can lose recent edits; modify
useDebouncedValue to either (a) accept an option like flushOnUnmount or expose
the debounced flush function and call it in a cleanup, or (b) return a third
element flush() alongside [localValue, setValue] that calls
debouncedPersist.flush(); update the hook to reference useDebouncedCallback,
debouncedPersist, setValue and onPersist so callers can flush pending
persistence in their own cleanup or have the hook auto-flush on unmount when the
option is enabled.
ilyabo
approved these changes
Mar 30, 2026
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Screen.Recording.2026-03-27.at.13.35.20.mov
Summary by CodeRabbit