Repository navigation
feat: Charts actions: only show on hover - #336
Conversation
Signed-off-by: Ilya Boyandin <[email protected]>
|
Warning Rate limit exceeded
⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. 📝 WalkthroughWalkthroughUpdated Vega chart UI and editor input handling: action container styling now hides/disables actions until hover/focus; VegaLiteArrowChart render tree gains an extra wrapper; editor APIs accept spec as string or object and use a normalization helper to initialize and detect spec changes. Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes 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 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}
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In `@packages/vega/src/VegaChartActions.tsx`:
- Line 39: The action container currently hides controls by default using
"opacity-0 pointer-events-none" and only reveals them on hover; update the class
string in VegaChartActions (the container rendering the actions) to also reveal
on keyboard focus and touch by adding the Tailwind variants
"group-focus-within:pointer-events-auto group-focus-within:opacity-100" and a
touch fallback such as "pointer-coarse:pointer-events-auto
pointer-coarse:opacity-100" (or the equivalent non-hover/touch-safe variants
your Tailwind version supports) so actions become visible and interactive for
keyboard and touch users.
Signed-off-by: Ilya Boyandin <[email protected]>
Signed-off-by: Ilya Boyandin <[email protected]>
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In `@packages/vega/src/editor/useVegaChartEditor.ts`:
- Around line 33-58: The current normalizeInitialSpec treats any truthy result
from safeJsonParse as a valid VisualizationSpec; change normalizeInitialSpec to
validate that parsed is a non-null plain object (typeof parsed === 'object' &&
parsed !== null && !Array.isArray(parsed)) before returning parseOk: true and
using parsed; if the parsed value is not an object (array or primitive), treat
it as a failed parse (parsed: null, parseOk: false) and return
normalized/formatted as the original spec string so onSpecChange and Vega embed
only receive actual object specs.
| const normalizeInitialSpec = (spec: VisualizationSpec | string) => { | ||
| if (typeof spec === 'string') { | ||
| const parsed = safeJsonParse(spec) as VisualizationSpec | null; | ||
| if (parsed) { | ||
| return { | ||
| parsed, | ||
| normalized: JSON.stringify(parsed), | ||
| formatted: JSON.stringify(parsed, null, 2), | ||
| parseOk: true, | ||
| }; | ||
| } | ||
| return { | ||
| parsed: null, | ||
| normalized: spec, | ||
| formatted: spec, | ||
| parseOk: false, | ||
| }; | ||
| } | ||
|
|
||
| return { | ||
| parsed: spec, | ||
| normalized: JSON.stringify(spec), | ||
| formatted: JSON.stringify(spec, null, 2), | ||
| parseOk: true, | ||
| }; | ||
| }; |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
# First, find the file and understand its structure
find . -name "useVegaChartEditor.ts" -type fRepository: sqlrooms/sqlrooms
Length of output: 109
🏁 Script executed:
# Locate VisualizationSpec type definition
rg -t ts "VisualizationSpec" --max-count 20Repository: sqlrooms/sqlrooms
Length of output: 2584
🏁 Script executed:
# Find safeJsonParse implementation
rg -t ts "safeJsonParse" -B 2 -A 10Repository: sqlrooms/sqlrooms
Length of output: 9927
🌐 Web query:
vega-embed VisualizationSpec type definition documentation
💡 Result:
VisualizationSpec in vega-embed is a TypeScript type alias:
type VisualizationSpec = VlSpec | VgSpec(vega.github.io)
Meaning: wherever vega-embed accepts a VisualizationSpec, you can pass either:
- a Vega-Lite spec (
VlSpec) — see Vega-Lite spec docs (JSON structure) (vega.github.io) and the Vega-Lite TypeScript typing entrypoint (TopLevelSpec) (vega.github.io) - a Vega spec (
VgSpec) — see Vega spec docs (JSON structure) (vega.github.io)
Where to find the VisualizationSpec definition docs:
- Vega-Embed TypeDoc page: “VisualizationSpec” (vega.github.io)
Citations:
- 1: https://vega.github.io/vega-embed/types/VisualizationSpec.html
- 2: https://vega.github.io/vega-lite/docs/spec.html?utm_source=openai
- 3: https://vega.github.io/vega-lite/usage/typescript.html?utm_source=openai
- 4: https://vega.github.io/vega/docs/specification/?utm_source=openai
- 5: https://vega.github.io/vega-embed/types/VisualizationSpec.html
🏁 Script executed:
# Check the full context of useVegaChartEditor.ts to see how parsedSpec is used
cat packages/vega/src/editor/useVegaChartEditor.tsRepository: sqlrooms/sqlrooms
Length of output: 8403
Validate parsed string specs are objects before treating them as VisualizationSpec.
safeJsonParse can return arrays or primitives (e.g., "[]" or "\"foo\""), which are truthy and currently treated as valid specs. That can flow into onSpecChange and Vega embed with an invalid shape. Consider an explicit object check.
🔧 Suggested guard
- const normalizeInitialSpec = (spec: VisualizationSpec | string) => {
+ const normalizeInitialSpec = (spec: VisualizationSpec | string) => {
+ const isSpecObject = (value: unknown): value is VisualizationSpec =>
+ typeof value === 'object' && value !== null && !Array.isArray(value);
if (typeof spec === 'string') {
- const parsed = safeJsonParse(spec) as VisualizationSpec | null;
- if (parsed) {
+ const parsed = safeJsonParse(spec);
+ if (isSpecObject(parsed)) {
return {
- parsed,
+ parsed,
normalized: JSON.stringify(parsed),
formatted: JSON.stringify(parsed, null, 2),
parseOk: true,
};
}
return {
- parsed: null,
+ parsed: null,
normalized: spec,
formatted: spec,
parseOk: false,
};
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| const normalizeInitialSpec = (spec: VisualizationSpec | string) => { | |
| if (typeof spec === 'string') { | |
| const parsed = safeJsonParse(spec) as VisualizationSpec | null; | |
| if (parsed) { | |
| return { | |
| parsed, | |
| normalized: JSON.stringify(parsed), | |
| formatted: JSON.stringify(parsed, null, 2), | |
| parseOk: true, | |
| }; | |
| } | |
| return { | |
| parsed: null, | |
| normalized: spec, | |
| formatted: spec, | |
| parseOk: false, | |
| }; | |
| } | |
| return { | |
| parsed: spec, | |
| normalized: JSON.stringify(spec), | |
| formatted: JSON.stringify(spec, null, 2), | |
| parseOk: true, | |
| }; | |
| }; | |
| const normalizeInitialSpec = (spec: VisualizationSpec | string) => { | |
| const isSpecObject = (value: unknown): value is VisualizationSpec => | |
| typeof value === 'object' && value !== null && !Array.isArray(value); | |
| if (typeof spec === 'string') { | |
| const parsed = safeJsonParse(spec); | |
| if (isSpecObject(parsed)) { | |
| return { | |
| parsed, | |
| normalized: JSON.stringify(parsed), | |
| formatted: JSON.stringify(parsed, null, 2), | |
| parseOk: true, | |
| }; | |
| } | |
| return { | |
| parsed: null, | |
| normalized: spec, | |
| formatted: spec, | |
| parseOk: false, | |
| }; | |
| } | |
| return { | |
| parsed: spec, | |
| normalized: JSON.stringify(spec), | |
| formatted: JSON.stringify(spec, null, 2), | |
| parseOk: true, | |
| }; | |
| }; |
🤖 Prompt for AI Agents
In `@packages/vega/src/editor/useVegaChartEditor.ts` around lines 33 - 58, The
current normalizeInitialSpec treats any truthy result from safeJsonParse as a
valid VisualizationSpec; change normalizeInitialSpec to validate that parsed is
a non-null plain object (typeof parsed === 'object' && parsed !== null &&
!Array.isArray(parsed)) before returning parseOk: true and using parsed; if the
parsed value is not an object (array or primitive), treat it as a failed parse
(parsed: null, parseOk: false) and return normalized/formatted as the original
spec string so onSpecChange and Vega embed only receive actual object specs.
There was a problem hiding this comment.
Actionable comments posted: 0
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
packages/vega/src/editor/useVegaChartEditor.ts (1)
223-249:⚠️ Potential issue | 🟡 MinorReset should still restore invalid original spec strings.
Now that
initialSpeccan be a string,resetToOriginalno-ops when the original JSON is invalid, so users can’t revert to the original raw spec. Consider falling back to the raw string and original SQL in the catch branch.🛠️ Proposed fallback for invalid original spec
} catch { - return prev; + return { + ...prev, + editedSpecString: prev.originalSpecString, + editedSql: prev.originalSql, + appliedSpecString: prev.originalSpecString, + appliedSql: prev.originalSql, + prevInitialSpecString: prev.originalSpecString, + prevInitialSql: prev.originalSql, + }; }
Kapture.2026-02-03.at.18.13.44.mp4
Summary by CodeRabbit
Bug Fixes
Improvements