Repository navigation
Conversation
…omponents to improve word breaking
…event layout issues with map legend
…eplerMapContainer
…pane support in CustomMapLegendPanel
… for interactive editing
…d improving canvas rendering
…pec reference and improve code readability
…ith spec prop for improved functionality
…d update ChartToolRenderer to support interactive editing
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughImplements interactive Vega chart editing with title and label modifications, updates SVG/PNG export to inline external styles, integrates the new interactive editor into chart components, and exposes these features through public APIs with documentation and example usage. ChangesInteractive Vega Chart Editing and Chart Export
Estimated code review effort🎯 4 (Complex) | ⏱️ ~50 minutes Possibly related PRs
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 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 |
|
@ilyabo A candidate feature for vega-lite chart tool |
…d streamline resolution format
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 7
🤖 Prompt for all review comments with 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.
Inline comments:
In `@packages/vega/README.md`:
- Line 163: The docs for the prop editorMode are incorrect: replace the listed
enum value 'spec' with the actual API value 'vega' so examples and the README
match the package contract; update the description of editorMode (used in the
popover for spec/SQL editors) and any examples showing editorMode to use 'vega'
(referencing the editorMode prop) to avoid mismatch with the rest of the README
and package API.
In `@packages/vega/src/InteractiveVegaChartToolResult.tsx`:
- Around line 119-121: The currentSpec state is only initialized once from
editedSpecCache or vegaLiteSpec (useState in InteractiveVegaChartToolResult) and
never updates when output?.vegaLiteSpec becomes available later; add a useEffect
that watches output?.vegaLiteSpec, toolCallId, and editedSpecCache and calls
setCurrentSpec(...) to set the spec to editedSpecCache.get(toolCallId) ??
output.vegaLiteSpec ?? vegaLiteSpec so the chart re-renders when the tool output
transitions to available.
In `@packages/vega/src/VegaExportAction.tsx`:
- Around line 49-57: VegaExportAction currently inlines every stylesheet rule by
iterating document.styleSheets (the cssText build starting at the loop over
sheets), which bloats exports; change the logic to only include CSS rules that
actually apply to the cloned SVG subtree (or alternatively compute and inline
per-element computed styles for nodes Vega doesn't serialize). Concretely, when
building cssText for the cloned SVG used by export, collect all elements in the
cloned SVG, then for each stylesheet rule (use rule.selectorText) test whether
rule.selectorText matches any element in that cloned SVG (e.g., via
clonedSvg.querySelectorAll) and only append those matching rule.cssText; for
properties that are not preserved by Vega serialization, compute
window.getComputedStyle for the specific affected nodes and inline those
declarations on the elements instead. Update VegaExportAction (and any helper
used by InteractiveVegaChartToolResult/VegaLiteArrowChart exports) to use this
filtered/more-targeted approach.
- Around line 95-115: The temporary blob URL created with
URL.createObjectURL(blob) isn’t revoked on image load failure; wrap the image
load/rasterization logic in a try/finally so URL.revokeObjectURL(url) always
runs even if img.onerror rejects. Specifically, in the code around
img.onload/img.onerror (the Promise that awaits image load) and subsequent
canvas draw (ctx.drawImage), move the revoke call into a finally block that
executes before returning the canvas or propagating the error so the blob URL is
released on both success and failure.
In `@packages/vega/src/VegaInteractiveEdit.tsx`:
- Around line 393-400: The click (non-drag) branch currently calls
element.removeAttribute('transform') and thus can erase Vega's original
placement when no persisted offset exists; change the logic in the click handler
that uses draggingRef so that if draggingRef.current.origTransformX/Y (or any
saved original transform) exists you restore/apply that original transform, and
if no persisted offset is present you do nothing (leave the element's transform
untouched) instead of removeAttribute('transform'); update references around
draggingRef and any ariaKey-based persistence checks to only clear the stored
offset state, not the SVG transform attribute itself.
- Around line 408-414: The effect that syncs offsets from spec into
offsetsRef.current (useEffect with dependency spec) merges new entries but never
removes keys removed from spec, leaving stale transforms; modify the sync so it
first resets/clears offsetsRef.current (e.g., clear the Map) before iterating
the result of extractOffsetsFromSpec(spec) and setting entries, and ensure the
same reset occurs when spec is undefined/null so offsetsRef.current does not
retain old values (refer to offsetsRef, useEffect, extractOffsetsFromSpec, and
spec.usermeta.__labelOffsets).
In `@packages/vega/src/vegaInteractiveEditHelpers.ts`:
- Around line 391-394: The loop that restores persisted offsets (the for (const
entry of entries) block) currently accepts entry.dx/entry.dy without validation;
update it to validate/coerce these values before calling
result.set(entry.ariaKey, {dx: entry.dx, dy: entry.dy}) — e.g., ensure typeof
entry.dx/entry.dy === 'number' && isFinite(entry.dx) (or coerce with Number(...)
and check isFinite) and if invalid either skip the entry or substitute a safe
default (0); apply this check around the existing entry.ariaKey condition so
only valid numeric dx/dy are restored.
🪄 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: 82fc31c9-2d7b-451d-b634-234db5529626
⛔ Files ignored due to path filters (1)
pnpm-lock.yamlis excluded by!**/pnpm-lock.yaml
📒 Files selected for processing (9)
examples/ai/src/store.tspackages/ai-core/src/components/FlatAgentRenderer.tsxpackages/vega/README.mdpackages/vega/src/InteractiveVegaChartToolResult.tsxpackages/vega/src/VegaExportAction.tsxpackages/vega/src/VegaInteractiveEdit.tsxpackages/vega/src/VegaLiteArrowChart.tsxpackages/vega/src/index.tspackages/vega/src/vegaInteractiveEditHelpers.ts
…artToolResult to handle initial rendering of vegaLiteSpec; improve error handling in svgToCanvas function; add origTransformAttr to DragLabelsMode for better drag handling
…InteractiveVegaChartToolResult to prevent memory leaks
|
@codex review |
| useHoistedRenderers, | ||
| } from './components/HoistedRenderersContext'; | ||
| export { | ||
| findChatSearchMatches, |
There was a problem hiding this comment.
looks like this is from another PR
|
|
||
| ## Chat search | ||
|
|
||
| `Chat` renders a `ChatSearchProvider` and exposes `Chat.Search`, an in-conversation |
There was a problem hiding this comment.
looks like this is from another PR
| } from '@sqlrooms/ai-core'; | ||
| export {Chat} from '@sqlrooms/ai-core'; | ||
| export {ContextSelector, CHAT_CONTEXT_SELECTOR_SLOT} from '@sqlrooms/ai-core'; | ||
| export { |
There was a problem hiding this comment.
looks like this is from another PR
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 425b47bca4
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| if (persistTimeoutRef.current) { | ||
| clearTimeout(persistTimeoutRef.current); | ||
| persistTimeoutRef.current = null; |
There was a problem hiding this comment.
If a user edits a chart and then switches sessions, closes the chat view, or reloads the page within persistDebounceMs (500 ms by default), this cleanup cancels the only scheduled call to setSessionUiMessages. The in-memory editedSpecCache can mask the loss during the same JS session, but the edit is never written to the AI session and is lost on reload despite having been applied visually; the cleanup should flush the pending persist or persist synchronously before clearing it.
Useful? React with 👍 / 👎.
|
@lixun910 This looks cool, but wouldn't it be more valuable, instead of making the charts UI more complex, to add the functionality within individual charts to ask AI to make adjustments in them? e.g. something like this (Databricks): 
also we are trying to invest more in mosaic charts now, than vega, because they have more powerful cross-filtering capabilities. AI review (with my input): Here’s the combined version with that added: I’m supportive of the goal here: letting users polish generated charts is meaningful, especially for presentation/reporting workflows. But I think this PR may be pushing too much complexity into the Vega chart UI itself, and I’m not sure this is where we should invest deeply right now. My bigger product concern is that the most valuable version of this might be “ask AI to adjust this chart” rather than a growing WYSIWYG editing toolbar. For SQLRooms, that seems more aligned with the architecture: the AI can operate on the same chart primitives users already have, propose spec/query changes, preview them, and persist normal chart state. That would work for both AI-generated and manually-created charts. There’s also a strategic consideration: we’re increasingly investing in Mosaic charts rather than Vega, because Mosaic gives us stronger cross-filtering and interactive analytical capabilities. So I’d be cautious about building a complex Vega-specific editing surface unless we’re confident it will become a reusable pattern across chart systems. Right now the PR feels mostly aimed at AI-generated chat charts. A few implementation concerns before merge:
My recommendation would be to narrow this PR: keep any generally useful export improvements and maybe the reusable primitive if we’re confident in it, but avoid making the AI chat renderer the main abstraction. Longer term, I’d rather see a chart-local AI action like “Adjust chart…” that receives the current chart spec/config, SQL/query, schema/sample, and user instruction, then returns a previewable change. Ideally that pattern should work for Mosaic first, and Vega only where it remains useful. Findings: [P1] Dragged labels persist as absolute SVG positions, not offsets. In VegaInteractiveEdit.tsx, finalDx/finalDy are the final translate(...) coordinates and later reapplied verbatim at line 776. That means resize, data changes, or different chart layout can pin a label to an old absolute pixel location instead of new Vega position + user delta. This is probably acceptable only for static post-generation polish, not durable/non-AI chart editing. [P2] Pending AI chart edits can be lost on unmount/session switch. InteractiveVegaChartToolResult.tsx clears the debounce timer without flushing it, while the only session write happens later at line 185. A user can edit, switch sessions/reload within 500ms, see the local cache mask it briefly, then lose the edit. Flush on cleanup or persist synchronously when unmounting. [P2] The PR has unrelated public API/docs changes. The chat search exports/docs in packages/ai-core/README.md, packages/ai-core/src/index.ts, and packages/ai/src/index.ts look like another PR. I’d split them out. [P2] Lockfile churn is far broader than the dependency addition. Adding immer should not rewrite unrelated peer resolutions like @loaders.gl/parquet/ws at pnpm-lock.yaml#L20834. Regenerate with the repo’s pinned pnpm and keep only the real @sqlrooms/vega/immer changes. |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: b486f9b3db
ℹ️ About Codex in GitHub
Codex has been enabled to automatically review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
| if (liveOutputRef.current) { | ||
| liveOutputRef.current.vegaLiteSpec = newSpec; |
There was a problem hiding this comment.
When a user edits a chart and later sends another prompt in the same mounted session, this only mutates the store-backed output object rendered by AnalysisResult; it does not update the useChat internal messages snapshot, because setSessionUiMessages structured-clones messages and useSessionChat only reinitializes on messagesRevision. The next useChat update therefore uses/writes the stale chart output and can drop the edited spec from both model history and persisted uiMessages; bump/reset the chat messages or update that state as part of the edit persistence.
Useful? React with 👍 / 👎.
| if (/^-?\d+(\.\d+)?$/.test(normalizedValue)) { | ||
| return `(datum['${escapedField}'] == ${normalizedValue} || ${stringComparison})`; |
There was a problem hiding this comment.
For labels whose Vega aria value is formatted, such as a text mark using format: ',.0f' that reports 1,234, this falls through to the string comparison against toString(datum[field]), which is 1234, so the generated delete filter matches no rows. In that common numeric-label case the click only hides the SVG node until the chart re-renders and the label comes back; normalize formatted numeric strings or avoid emitting a filter that cannot match the raw datum.
Useful? React with 👍 / 👎.
WYSIWYG editing on top of Vega-Lite's rendered SVG
other:
Summary by CodeRabbit
New Features
Documentation
Style