mo.chart_builder plan.md#9676
Draft
Light2Dark wants to merge 1 commit into
Draft
Conversation
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
![cubic-dev-ai[bot]](https://avatars.githubusercontent.com/in/1082092?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
5 issues found across 2 files
Prompt for AI agents (unresolved issues)
Check if these issues are valid — if so, understand the root cause of each and fix them. If appropriate, use sub-agents to investigate and fix each issue separately.
<file name="PLAN.md">
<violation number="1" location="PLAN.md:165">
P1: Inconsistent PR assignment for boxplot and candlestick. §3.1 marks list and §7 future blocks table assign them to PR 7, but the roadmap §21 assigns them to PR 8 (PR 7 is explicitly for multi-y/dual-axis). Implementers following the wrong section will ship blocks in the wrong PR.</violation>
<violation number="2" location="PLAN.md:350">
P2: `ControlSection` and `BlockRenderProps<TLens>` are used in the `Block` interface but never defined. This leaves implementers without a clear contract for what props a block renderer receives or what a ControlSection is.</violation>
<violation number="3" location="PLAN.md:355">
P1: `BuildContext` is referenced by the Block interface and extensively used in normalize functions (with properties like `opSource`, `channel`, `opChannel`, `fields`, `chartType`) but is never formally defined anywhere in the plan. Without a defined interface, the normalize contract is ambiguous and implementers have no single source of truth for what the context provides.</violation>
<violation number="4" location="PLAN.md:1051">
P1: Importing from the private `altair.vegalite.v6.schema._typing` module is fragile. Altair treats `_`-prefixed modules as private API (confirmed by their public API definition work in PR #3482), and has established a public `altair.typing` module for type exports. This dependency could break on any Altair minor release. Use the public `altair.typing` module or access types via `alt.Chart(...).to_dict()` round-tripping instead.</violation>
<violation number="5" location="PLAN.md:1380">
P2: Case inconsistency between the folder structure showing `rules.md` (lowercase) and all prose references using `RULES.md` (uppercase). The folder listing should match the name used consistently everywhere else to avoid naming conflicts.</violation>
</file>
Architecture diagram
sequenceDiagram
participant User
participant Python as Python Runtime (marimo)
participant MoChartAdapter as mo.chart_builder Plugin
participant Panel as ChartPanel (UI)
participant Core as chart-builder/core
participant Registry as Chart-Type Registry
participant Blocks as Block Library
participant Subset as UI Subset Allowlist
participant Normalize as normalize()
participant Validate as validate()
participant Codegen as Codegen
participant Backend as Vega Backend
participant MarimoCell as Marimo Cell Renderer
participant AI as chart-builder/ai
Note over User,AI: Chart Builder Architecture Overview
User->>Python: Execute cell with mo.chart_builder(df, spec={...})
Python->>MoChartAdapter: Create UIElement with spec
MoChartAdapter->>Panel: Render ChartPanel with spec
Panel->>Core: useChartBuilder(spec)
Core->>Registry: Lookup chart type from usermeta
Registry-->>Core: chart type definition
Core->>Blocks: Load blocks for chart type
Blocks->>Subset: Verify block paths are in allowlist
Subset-->>Blocks: Allowlist paths
Blocks-->>Core: Block definitions
Panel->>Normalize: normalize(spec, fields)
Normalize->>Normalize: Apply default aggregates, time units
Normalize->>Normalize: Add usermeta namespace
Normalize-->>Panel: Normalized spec
Panel->>Validate: validate(normalizedSpec)
Validate->>Validate: Vega-Lite validity + custom rules
alt Validation passes
Validate-->>Panel: Valid spec
else Validation fails
Validate-->>Panel: Errors with auto-fix suggestions
Panel->>Normalize: Apply fixes
Normalize-->>Panel: Fixed spec
end
Panel->>Backend: Render chart with VegaEmbed
Backend-->>Panel: Rendered chart
User->>Panel: Edit encoding (e.g., drag field to x-axis)
Panel->>Core: useChartSelection(update)
Core->>Normalize: Apply mutation to spec path
Normalize->>Validate: Validate updated spec
Validate-->>Normalize: OK
Normalize-->>Core: Updated spec
Core-->>Panel: New spec
Panel->>Backend: Re-render with updated spec
alt Persistence mode (mo.chart_builder)
Panel->>MoChartAdapter: Send updated spec to cell source
MoChartAdapter->>MarimoCell: Update cell source via marimo channel
MarimoCell->>Python: Re-run cell with new spec
Python->>MoChartAdapter: Recreate chart with new spec
MoChartAdapter->>Panel: Re-render
end
Note over User,AI: AI interaction path
AI->>Normalize: edit_chart(userRequest, currentSpec)
Normalize->>Normalize: Apply intent-based edits
Normalize->>Validate: Validate AI-modified spec
Validate-->>Normalize: OK / errors with fixes
Normalize-->>AI: Updated spec
AI-->>Panel: Apply spec update
Panel->>Backend: Re-render
Note over User,MarimoCell: Python parity path
User->>Python: Direct Python API call
Python->>MoChartAdapter: normalize(spec, fields)
MoChartAdapter->>Core: Call TS normalize (via WASM or port)
Core-->>MoChartAdapter: Result
MoChartAdapter-->>Python: Dict spec
Reply with feedback, questions, or to request a fix.
Re-trigger cubic
| Altair downloads the Vega-Lite JSON Schema and regenerates Python `TypedDict`s on every Vega-Lite version bump. We import them directly: | ||
|
|
||
| ```python | ||
| from altair.vegalite.v6.schema._typing import TopLevelSpec, Mark |
Contributor
There was a problem hiding this comment.
P1: Importing from the private altair.vegalite.v6.schema._typing module is fragile. Altair treats _-prefixed modules as private API (confirmed by their public API definition work in PR #3482), and has established a public altair.typing module for type exports. This dependency could break on any Altair minor release. Use the public altair.typing module or access types via alt.Chart(...).to_dict() round-tripping instead.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At PLAN.md, line 1051:
<comment>Importing from the private `altair.vegalite.v6.schema._typing` module is fragile. Altair treats `_`-prefixed modules as private API (confirmed by their public API definition work in PR #3482), and has established a public `altair.typing` module for type exports. This dependency could break on any Altair minor release. Use the public `altair.typing` module or access types via `alt.Chart(...).to_dict()` round-tripping instead.</comment>
<file context>
@@ -0,0 +1,1703 @@
+Altair downloads the Vega-Lite JSON Schema and regenerates Python `TypedDict`s on every Vega-Lite version bump. We import them directly:
+
+```python
+from altair.vegalite.v6.schema._typing import TopLevelSpec, Mark
+from altair.vegalite.v6.schema.channels import X, Y, Color
+import altair as alt
</file context>
| visible?: (spec: VegaLiteSpec) => boolean; | ||
|
|
||
| // Optional protocols used by other blocks and the orchestrator. | ||
| normalize?: (lens: TLens, spec: VegaLiteSpec, ctx: BuildContext) => TLens; |
Contributor
There was a problem hiding this comment.
P1: BuildContext is referenced by the Block interface and extensively used in normalize functions (with properties like opSource, channel, opChannel, fields, chartType) but is never formally defined anywhere in the plan. Without a defined interface, the normalize contract is ambiguous and implementers have no single source of truth for what the context provides.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At PLAN.md, line 355:
<comment>`BuildContext` is referenced by the Block interface and extensively used in normalize functions (with properties like `opSource`, `channel`, `opChannel`, `fields`, `chartType`) but is never formally defined anywhere in the plan. Without a defined interface, the normalize contract is ambiguous and implementers have no single source of truth for what the context provides.</comment>
<file context>
@@ -0,0 +1,1703 @@
+ visible?: (spec: VegaLiteSpec) => boolean;
+
+ // Optional protocols used by other blocks and the orchestrator.
+ normalize?: (lens: TLens, spec: VegaLiteSpec, ctx: BuildContext) => TLens;
+ validate?: (lens: TLens, ctx: BuildContext) => Issue[];
+ describeFields?: (lens: TLens) => FieldDescription[];
</file context>
| ```text | ||
| Marks (chart types): | ||
| bar, line, area, point (scatter), rect (heatmap), arc (pie/donut) | ||
| (PR 7) boxplot, tick, rule, layered for candlestick |
Contributor
There was a problem hiding this comment.
P1: Inconsistent PR assignment for boxplot and candlestick. §3.1 marks list and §7 future blocks table assign them to PR 7, but the roadmap §21 assigns them to PR 8 (PR 7 is explicitly for multi-y/dual-axis). Implementers following the wrong section will ship blocks in the wrong PR.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At PLAN.md, line 165:
<comment>Inconsistent PR assignment for boxplot and candlestick. §3.1 marks list and §7 future blocks table assign them to PR 7, but the roadmap §21 assigns them to PR 8 (PR 7 is explicitly for multi-y/dual-axis). Implementers following the wrong section will ship blocks in the wrong PR.</comment>
<file context>
@@ -0,0 +1,1703 @@
+```text
+Marks (chart types):
+ bar, line, area, point (scatter), rect (heatmap), arc (pie/donut)
+ (PR 7) boxplot, tick, rule, layered for candlestick
+
+Channels:
</file context>
| ai/ | ||
| index.ts # re-exports of public API agents call from code-mode | ||
| tools.ts # edit_chart frontend tool (FRONTEND_TOOL_REGISTRY) | ||
| rules.md # hand-written authoring rules |
Contributor
There was a problem hiding this comment.
P2: Case inconsistency between the folder structure showing rules.md (lowercase) and all prose references using RULES.md (uppercase). The folder listing should match the name used consistently everywhere else to avoid naming conflicts.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At PLAN.md, line 1380:
<comment>Case inconsistency between the folder structure showing `rules.md` (lowercase) and all prose references using `RULES.md` (uppercase). The folder listing should match the name used consistently everywhere else to avoid naming conflicts.</comment>
<file context>
@@ -0,0 +1,1703 @@
+ ai/
+ index.ts # re-exports of public API agents call from code-mode
+ tools.ts # edit_chart frontend tool (FRONTEND_TOOL_REGISTRY)
+ rules.md # hand-written authoring rules
+ vega-lite-subset-export.ts # generates ai/vega-lite-subset.json at build time
+ intent-namespace-export.ts # generates ai/intent-namespace.json at build time
</file context>
Suggested change
| rules.md # hand-written authoring rules | |
| RULES.md # hand-written authoring rules |
| defaults: TLens; | ||
|
|
||
| // Render the UI. | ||
| render: ControlSection | React.FC<BlockRenderProps<TLens>>; |
Contributor
There was a problem hiding this comment.
P2: ControlSection and BlockRenderProps<TLens> are used in the Block interface but never defined. This leaves implementers without a clear contract for what props a block renderer receives or what a ControlSection is.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At PLAN.md, line 350:
<comment>`ControlSection` and `BlockRenderProps<TLens>` are used in the `Block` interface but never defined. This leaves implementers without a clear contract for what props a block renderer receives or what a ControlSection is.</comment>
<file context>
@@ -0,0 +1,1703 @@
+ defaults: TLens;
+
+ // Render the UI.
+ render: ControlSection | React.FC<BlockRenderProps<TLens>>;
+
+ visible?: (spec: VegaLiteSpec) => boolean;
</file context>
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
1 participant
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.
📝 Summary
Closes #
📋 Pre-Review Checklist
✅ Merge Checklist