Review 4411
#4411 feat(vega): theme Vega charts from the charts package by AKnassa (bucket: contributor)
b846f95f4a57855c7684c33538003ce702e3ee9d — current main merged mechanically into the branch before review.
LOOP VERSION: 1.6.0 AUDIT RUBRIC: 1.13
LANE: full WHY: contributor PR with package dependency changes, public exports, build-test setup, and rendered Vega theming behavior.
WHY 1: Vega-Lite charts in @astryxdesign/vega did not use the shared Astryx chart palette or theme config.
WHY 2: Builders combining declarative Vega charts with compositional Astryx charts could get mismatched colors and chrome unless they manually carried a parallel config.
WHY 3: The chart system exists to make data visualization visually consistent across authoring models and themes.
USER-FACING PROBLEM: a builder can place two Astryx chart paths in one dashboard and get a spec-based chart that does not follow the system palette.
PROBLEM SEVERITY: missing capability — the declarative chart path lacked the shared theming seam.
VERDICT: clear
Vega-Lite charts now read the active Astryx theme, build a default Vega-Lite config from resolved tokens, and pass that config underneath the caller's own compile options. The categorical palette comes from the charts package so both chart paths use one ordered list. The package test setup also builds charts before the node-project Vega tests import it.
SOLUTION (3 decisions · ~126 runtime/config lines of 399 total)
- Export
CATEGORICAL_TOKENSfrom charts and consume it from vega — stated problem: one shared palette. - Compile Vega-Lite specs with
withAstryxConfig()and the active theme — stated problem: themed by default. - Generalize the test global setup to build charts as well as core — stated problem: node tests resolve workspace package
dist.
BURDEN: medium — new package dependency/peer graph, one new public helper export, one theme-driven memo, and shared test setup. BURDEN MATCH: proportionate for the stated goal, but two implementation details are wrong.
VERDICT: BLOCKS — package peer graph is unsatisfiable and native Vega specs reinitialize on theme changes.
OWNER: @astryxdesign/vega owns Vega-Lite compilation; @astryxdesign/charts owns the shared chart palette list.
TIER 1: theme token resolution through useTheme; chart palette from getChartColors/CATEGORICAL_TOKENS.
TIER 2: none.
SEAMS: Vega-Lite vs native Vega specs, caller compileOptions.config, inline spec config, light/dark/custom theme changes, and node-project package resolution.
BEHAVIOR UNIT: pure utility (withAstryxConfig) plus component Effect; merge behavior is testable without DOM, theme reinitialization needs browser evidence.
| seam | driven result |
|---|---|
| Vega-Lite default config | rendered SVG bars used token-derived categorical fills (#0171E3, #EB6E00) and labels used --color-text-secondary
|
| caller config | rendered SVG kept theme label color while caller axis.labelFontSize: 99 and background #ffeecc won |
| inline spec config | rendered SVG label color #00ff00 and background #112233 won over caller config |
| custom theme/mode | rendered SVG bars changed to #abcdef/#fedcba in dark and #123456/#654321 in light |
| native Vega | rendered, but onReady fired 1 → 4 across theme/mode changes although docs say native specs are not themed |
| cold node-project test setup | after deleting packages/core/dist and packages/charts/dist, the Vega config Vitest rebuilt both and passed |
VERDICT: BLOCKS — the native Vega seam now depends on theme updates even though it does not consume the themed config.
Vega-Lite consumers get default Astryx palette/chrome and still keep caller/spec override precedence. Canary package consumers also get a broken peer graph: @astryxdesign/vega asks for core 0.3.0, while the new @astryxdesign/charts@0.1.9 dependency asks for core 0.5.0. Native Vega consumers get a lifecycle reset on any theme/mode change despite no visual theming.
VERDICT: BLOCKS — one install-path bug and one native-spec behavior regression.
New/changed surface:
| change | public? | class | doc'd? | verdict | |
|---|---|---|---|---|---|
+ |
@astryxdesign/charts exports CATEGORICAL_TOKENS
|
yes — charts barrel | chart palette source of truth | README mentions through vega docs | ok, but maintainer-owned surface |
+ |
@astryxdesign/vega exports withAstryxConfig(token, compileOptions?)
|
yes — vega barrel | helper for custom compile pipeline | README | ok after blockers |
+ |
@astryxdesign/vega depends on @astryxdesign/charts@0.1.9 and peers on @astryxdesign/core@0.3.0
|
package install contract | charts/core peer must align | README | finding |
OSSIFICATION: The issue already records vega depending on charts for shared theming, so the concept is not invented here. The peer versions are the package contract a canary consumer installs, and the current pair cannot be satisfied by one core version.
VERDICT: BLOCKS — peer dependency mismatch.
No theme target was added. The config consumes resolved semantic/data tokens rather than raw component colors. Browser evidence shows token-derived bars and labels update across light/dark and a custom theme, while caller and inline config override precedence still works.
VERDICT: clear
BEHAVIOR: yes — native Vega specs are documented as unthemed, but theme changes now recreate their View and refire onReady.
API: yes — vega now has an impossible peer graph with charts/core versions.
VISUAL: intended Vega-Lite visual change; rendered evidence matched token/config precedence.
THEME: existing tokens and overrides still resolve; no target removed.
VERDICT: BLOCKS — behavior and install contract both changed incorrectly.
EFFECTS: existing useEffect in VegaChart now depends on a theme-derived object.
| Effect + deps | external system | why render/handler cannot do it | measured render cost | lifetime + cleanup | focused test |
|---|---|---|---|---|---|
useEffect [spec, data, themedCompileOptions, parseConfig, parseOptions, viewOptions] VegaChart.tsx:120
|
Vega View lifecycle | needed to create/finalize Vega runtime | browser probe: native onReady 1 → 4 over light/dark/custom theme toggles |
cleanup finalizes the old View | missing |
RENDER: Vega-Lite charts intentionally rebuild on token changes; native Vega also rebuilds even though it is unthemed.
LISTENERS/OBSERVERS: no new listeners/observers in the wrapper; Vega runtime owns its own View and is finalized on cleanup.
LAYOUT: no layout read/write in React; rendering is delegated to Vega.
BUNDLE: new direct dependency on @astryxdesign/charts, externalized in tsup.
VERDICT: BLOCKS — native View teardown is new work with user-visible lifecycle effects.
VISUAL CHECK: manual frames required WHY: the PR's claim is rendered theme application and precedence in Vega-Lite output.
| Default light | Probe theme dark | Probe theme light |
|---|---|---|
 |
 |
 |
SENSOR RECEIPT: head b846f95f4a57855c7684c33538003ce702e3ee9d; Chromium; viewport 1100×900; four SVG chart cases rendered (default, caller-config, inline-config, native); themes/modes captured: default light, default dark, probe dark, probe light; page errors: 0; harness used stable spec/options objects to isolate theme-driven rerenders.
Observed values: default light bars #0171E3/#EB6E00, labels #4E606F; default dark labels #AAAFB5; probe dark bars #abcdef/#fedcba, labels #dddddd; probe light bars #123456/#654321, labels #222222; caller config preserved theme labels and applied font-size: 99px + background #ffeecc; inline spec config applied label #00ff00 + background #112233.
VERDICT: clear
REMEDY SEARCH: not triggered — no proven visual defect
No user-visible strings or ARIA/focus behavior changed in the wrapper. The rendered SVG had Vega's default graphics roles and no page errors. The native lifecycle reset is recorded under BREAKING/PERFORMANCE rather than A11Y.
VERDICT: clear
| slot | verdict |
|---|---|
| PROBLEM | clear |
| SOLUTION | BLOCKS — package peer graph and native seam |
| ARCHITECTURE | BLOCKS — native seam depends on theme |
| IMPACT | BLOCKS — install and native lifecycle regressions |
| API | BLOCKS — peer dependency mismatch |
| THEMING | clear |
| BREAKING | BLOCKS — behavior and install contract |
| PERFORMANCE | BLOCKS — native View teardown |
| VISUAL | clear |
| A11Y & I18N | clear |
GOAL: partly met — rendered Vega-Lite theming and precedence work, and focused tests pass; package install and native Vega lifecycle are not ready. DISPOSITION: core peer mismatch → blocks now; native Vega rerender → blocks now. ADVICE: bounded outcome criteria — align vega's core peer with the charts/core version it now requires, and keep theme-derived compile options from changing native Vega View lifecycle. AUTHOR CAN PROCEED: yes — both fixes are concrete and verifiable. WORST OUTCOME: “Canary package consumers also get a broken peer graph” and “native Vega consumers get a lifecycle reset on any theme/mode change” → request changes.
JUDGEMENT NEEDED: none for the blockers; the contributor/public API gate remains for the maintainer once blockers are fixed.
request changes
-
@astryxdesign/vegapeers on@astryxdesign/core@0.3.0while depending on@astryxdesign/charts@0.1.9, whose peer is core0.5.0. → a canary consumer cannot install one core version that satisfies both packages. ·packages/vega/package.json:53 - Native Vega specs are unthemed but still depend on the themed compile-options object.
→ changing theme/mode tears down the native View and refires
onReady; local SVG probe measured native ready1 → 4. ·packages/vega/src/VegaChart.tsx:190
Thanks, this gets the Vega-Lite theming path onto the shared chart palette, but I’d fix two things before it lands. The package graph currently has no satisfying core version: vega asks for 0.3.0 while its new charts dependency asks for 0.5.0, so a canary consumer gets peer-conflict noise whichever core they install. Native Vega specs are also documented as unthemed, but a Theme mode/theme change still tears their View down and calls onReady again; my local SVG probe saw native ready 1→4. Could you keep the theme dependency on the Vega-Lite branch only and align the core peer? If you’d rather talk it through with someone, we’re in Discord.
[Reviewed by Robohands]
-
packages/vega/package.json:53— This needs to match the charts/core peer; no core version satisfies both today. -
packages/vega/src/VegaChart.tsx:190— This also reruns native Vega specs on theme changes, even though they stay unthemed.
- No prior reviews or inline comments existed before this pass.
- The only merge conflict was adjacent README sections; both sections were kept and the merge commit was pushed without force.
- Exact-head CI: GitHub Actions passed,
review-requiredpending for community/API review, Vercel failed.
TIME total 35m setup 10m rules, safety guard, dependency diff, one mechanical main merge, fast install; warm main reused: yes reading 8m PR body, issue, instructions, diff, package metadata measuring 10m focused Vitest, cold package-build setup, Chromium Vega render probe writing 7m draft, two critic passes, wiki record waste 4m first install timeout, one wrong Vitest reporter flag, and one harness alias correction
- I did not inspect the Vercel failure logs; GitHub Actions were green and Vercel remained red.
Pass 1 failed: the draft over-weighted the package peer issue and did not anchor the native Vega lifecycle regression cleanly. Pass 2 passed: both blockers had independent evidence, author-can-proceed was yes, no speculative remedy or private delivery state remained in the public text.
Not posted to the GitHub PR; draft returned as requested.