Skip to content

Commit 20d8a46

Browse files
committed
refactor: address code review must-fix items for v7-cohesion-pt2
- Restore examples/src/animation.stories.tsx annotation block that was accidentally committed in 3b59d20 (the file had unstaged cleanup work predating this refactor). - Wrap marks render in try/finally so resetMarkRenderState fires even if any downstream renderer throws. Previously the reset was pure theater without exception safety. - Split the compile.ts ORCHESTRATION INVARIANTS block comment into three inline callouts co-located with the lines they guard, so a reader reordering the second half of compileChart sees the invariant at the point of mutation instead of 70 lines above.
1 parent 965ef89 commit 20d8a46

3 files changed

Lines changed: 59 additions & 58 deletions

File tree

examples/src/animation.stories.tsx

Lines changed: 8 additions & 8 deletions
Original file line numberDiff line numberDiff line change
@@ -498,14 +498,14 @@ const annotationDelaySpec: ChartSpec = {
498498
},
499499
},
500500
annotations: [
501-
// {
502-
// type: 'text',
503-
// text: 'Product relaunch',
504-
// x: '2024-04',
505-
// y: 53,
506-
// offset: { dx: -80, dy: -18 },
507-
// connector: true,
508-
// },
501+
{
502+
type: 'text',
503+
text: 'Product relaunch',
504+
x: '2024-04',
505+
y: 53,
506+
offset: { dx: -80, dy: -18 },
507+
connector: true,
508+
},
509509
{
510510
type: 'refline',
511511
y: 70,

packages/engine/src/compile.ts

Lines changed: 7 additions & 8 deletions
Original file line numberDiff line numberDiff line change
@@ -309,11 +309,8 @@ export function compileChart(spec: unknown, options: CompileOptions): ChartLayou
309309
theme = adaptTheme(theme);
310310
}
311311

312-
// ORCHESTRATION INVARIANTS — do not reorder without care:
313-
// 1. Legend is computed twice (preliminary + refined) to break a dims/legend dependency cycle.
314-
// 2. computeGridlines mutates `axes` in place.
315-
// 3. scales.defaultColor is set post-computeScales because the resolution needs theme context.
316-
312+
// INVARIANT 1 — double legend pass: preliminaryArea → computeDimensions → legendArea → final
313+
// legend. Breaks a dims/legend dependency cycle. Do not collapse into one call.
317314
// Compute legend first (needs to reserve space)
318315
const preliminaryArea: Rect = {
319316
x: 0,
@@ -376,8 +373,9 @@ export function compileChart(spec: unknown, options: CompileOptions): ChartLayou
376373
// Update color scale to use theme palette (only when user hasn't provided an explicit range)
377374
applyColorScaleRange(scales, renderSpec.encoding, theme);
378375

379-
// Set default color for single-series charts. If the user set a fill on the mark def
380-
// (string or gradient), that takes priority over the theme's first categorical color.
376+
// INVARIANT 3 — post-hoc defaultColor: must run AFTER computeScales since resolution needs
377+
// theme context. Do not move into computeScales (would require threading theme through).
378+
// If the user set a fill on the mark def, it takes priority over the theme's first categorical.
381379
scales.defaultColor = chartSpec.markDef.fill ?? theme.colors.categorical[0];
382380

383381
// Arc charts (pie/donut) don't use axes or gridlines
@@ -388,7 +386,8 @@ export function compileChart(spec: unknown, options: CompileOptions): ChartLayou
388386
? { x: undefined, y: undefined }
389387
: computeAxes(scales, chartArea, strategy, theme, options.measureText);
390388

391-
// Compute gridlines (stored in axes, used by adapters via axes.y.gridlines)
389+
// INVARIANT 2 — computeGridlines mutates `axes` in place. Downstream consumers read
390+
// axes.y.gridlines off the same object. Do not introduce a copy-on-write.
392391
if (!isRadial) {
393392
computeGridlines(axes, chartArea);
394393
}

packages/vanilla/src/svg-renderer.ts

Lines changed: 44 additions & 42 deletions
Original file line numberDiff line numberDiff line change
@@ -131,55 +131,57 @@ export function renderChartSVG(
131131
svg.appendChild(defs);
132132

133133
// Prime mark-renderer module-level state so mark sub-renderers can resolve
134-
// animation + gradient fills without signature changes.
134+
// animation + gradient fills without signature changes. try/finally guarantees
135+
// the reset fires even if any downstream renderer throws, so the next render
136+
// starts with a clean slate.
135137
setMarkRenderState({ animation, gradientMap });
138+
try {
139+
// Render layers in order (back to front)
140+
// Axes render outside clip (labels extend beyond chart area)
141+
renderAxes(svg, layout);
142+
143+
// Marks are clipped to chart area so area fills don't cover chrome
144+
const clippedGroup = createSVGElement('g');
145+
clippedGroup.setAttribute('clip-path', `url(#${clipId})`);
146+
renderMarks(clippedGroup, layout);
147+
148+
// Add transparent overlay rect for line/area charts to enable voronoi tooltip lookup.
149+
// Only added when there are line or area marks with dataPoints, and no explicit
150+
// PointMark objects (which use per-element event handling instead).
151+
const hasLineOrAreaWithDataPoints = layout.marks.some(
152+
(m) => (m.type === 'line' || m.type === 'area') && m.dataPoints && m.dataPoints.length > 0,
153+
);
154+
const hasPointMarks = layout.marks.some((m) => m.type === 'point');
155+
if (hasLineOrAreaWithDataPoints && !hasPointMarks) {
156+
const overlay = createSVGElement('rect');
157+
setAttrs(overlay, {
158+
x: layout.area.x,
159+
y: layout.area.y,
160+
width: layout.area.width,
161+
height: layout.area.height,
162+
fill: 'transparent',
163+
});
164+
overlay.setAttribute('class', 'oc-voronoi-overlay');
165+
overlay.setAttribute('data-voronoi-overlay', 'true');
166+
clippedGroup.appendChild(overlay);
167+
}
136168

137-
// Render layers in order (back to front)
138-
// Axes render outside clip (labels extend beyond chart area)
139-
renderAxes(svg, layout);
140-
141-
// Marks are clipped to chart area so area fills don't cover chrome
142-
const clippedGroup = createSVGElement('g');
143-
clippedGroup.setAttribute('clip-path', `url(#${clipId})`);
144-
renderMarks(clippedGroup, layout);
145-
146-
// Add transparent overlay rect for line/area charts to enable voronoi tooltip lookup.
147-
// Only added when there are line or area marks with dataPoints, and no explicit
148-
// PointMark objects (which use per-element event handling instead).
149-
const hasLineOrAreaWithDataPoints = layout.marks.some(
150-
(m) => (m.type === 'line' || m.type === 'area') && m.dataPoints && m.dataPoints.length > 0,
151-
);
152-
const hasPointMarks = layout.marks.some((m) => m.type === 'point');
153-
if (hasLineOrAreaWithDataPoints && !hasPointMarks) {
154-
const overlay = createSVGElement('rect');
155-
setAttrs(overlay, {
156-
x: layout.area.x,
157-
y: layout.area.y,
158-
width: layout.area.width,
159-
height: layout.area.height,
160-
fill: 'transparent',
161-
});
162-
overlay.setAttribute('class', 'oc-voronoi-overlay');
163-
overlay.setAttribute('data-voronoi-overlay', 'true');
164-
clippedGroup.appendChild(overlay);
165-
}
166-
167-
svg.appendChild(clippedGroup);
169+
svg.appendChild(clippedGroup);
168170

169-
renderAnnotations(svg, layout);
170-
renderLegend(svg, layout.legend);
171+
renderAnnotations(svg, layout);
172+
renderLegend(svg, layout.legend);
171173

172-
// Chrome renders on top so titles are never obscured by chart elements
173-
renderChrome(svg, layout);
174+
// Chrome renders on top so titles are never obscured by chart elements
175+
renderChrome(svg, layout);
174176

175-
// Brand renders as a footer item, right-aligned on the source/footer row
176-
if (layout.watermark) {
177-
renderBrand(svg, layout);
177+
// Brand renders as a footer item, right-aligned on the source/footer row
178+
if (layout.watermark) {
179+
renderBrand(svg, layout);
180+
}
181+
} finally {
182+
resetMarkRenderState();
178183
}
179184

180-
// Reset module-level state after rendering
181-
resetMarkRenderState();
182-
183185
container.appendChild(svg);
184186
return svg;
185187
}

0 commit comments

Comments
 (0)