Skip to content

Commit 809e231

Browse files
committed
fix(engine): dual-axis lines no longer break where layers' x-values interleave
compileLayerIndependent gave both dual-axis layers a shared x-axis by calling ensureXDomainCoverage, which injected the OTHER layer's x-values into each leaf as placeholder rows with no y-field. In line/area compute those became null-y points, which are treated as line breaks -- so two series with interleaved x-values (e.g. two temporal series on different dates) drew as disconnected segments. This is the "line rendered with gaps" bug. The earlier guard (1e105ff) only added a regression test that asserts point x-positions, not path continuity, and used disjoint 2-point layers where the injected nulls land at the ends rather than mid-line -- so it never caught this. Replace the row-injection with an explicit union x-domain pinned on each leaf's x-encoding (temporal -> [minDate, maxDate], quantitative -> [min, max], discrete -> ordered category union). The scale builders already honour scale.domain, so both layers share one x-scale without ever touching mark data. An author-pinned x domain still wins per leaf. The discrete union is pinned to both leaves so the bar-remap path still enumerates every band centre. Add regression tests that assert PATH continuity (exactly one M command per series) for interleaved temporal series, and that bar+line remap covers the full category union. Claude-Session: https://claude.ai/code/session_016rS7hi2g4bSzRBfnD1p2ji
1 parent 6753720 commit 809e231

2 files changed

Lines changed: 210 additions & 21 deletions

File tree

packages/engine/src/__tests__/compile-layer.test.ts

Lines changed: 108 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -1022,4 +1022,112 @@ describe('compileLayer', () => {
10221022
expect(Math.min(...layerBXs)).toBeGreaterThan(areaMidX);
10231023
expect(Math.max(...layerAXs)).toBeLessThan(Math.min(...layerBXs));
10241024
});
1025+
1026+
it('keeps dual-axis lines continuous when the two layers have interleaved x-values', () => {
1027+
// Regression guard for the "line drawn with gaps" bug. The shared x-domain
1028+
// used to be achieved by injecting the OTHER layer's x-values as placeholder
1029+
// rows with no y-field. Those became null-y points, which line compute treats
1030+
// as line breaks — so two temporal series with interleaved dates each drew as
1031+
// disconnected segments. The fix pins an explicit union domain instead of
1032+
// touching mark data, so each series stays a single unbroken path.
1033+
//
1034+
// Layer A: Jan, Mar, May. Layer B: Feb, Apr, Jun. Sorted together the dates
1035+
// interleave, which is exactly the arrangement that produced mid-line breaks.
1036+
const spec: LayerSpec = {
1037+
resolve: { scale: { y: 'independent' } },
1038+
layer: [
1039+
{
1040+
mark: 'line' as const,
1041+
data: [
1042+
{ d: '2024-01-01', a: 10 },
1043+
{ d: '2024-03-01', a: 20 },
1044+
{ d: '2024-05-01', a: 15 },
1045+
],
1046+
encoding: {
1047+
x: { field: 'd', type: 'temporal' as const },
1048+
y: { field: 'a', type: 'quantitative' as const },
1049+
},
1050+
},
1051+
{
1052+
mark: 'line' as const,
1053+
data: [
1054+
{ d: '2024-02-01', b: 5 },
1055+
{ d: '2024-04-01', b: 8 },
1056+
{ d: '2024-06-01', b: 6 },
1057+
],
1058+
encoding: {
1059+
x: { field: 'd', type: 'temporal' as const },
1060+
y: { field: 'b', type: 'quantitative' as const },
1061+
},
1062+
},
1063+
],
1064+
};
1065+
1066+
const layout = compileLayer(spec, compileOpts);
1067+
const lines = layout.marks.filter((m) => m.type === 'line') as unknown as {
1068+
path: string;
1069+
points: { x: number }[];
1070+
data: unknown[];
1071+
}[];
1072+
expect(lines).toHaveLength(2);
1073+
1074+
for (const line of lines) {
1075+
// Exactly one `M` command ⇒ one unbroken subpath (no injected null breaks).
1076+
expect((line.path.match(/M/g) ?? []).length).toBe(1);
1077+
// No placeholder rows leaked into the mark: 3 real points per series.
1078+
expect(line.points).toHaveLength(3);
1079+
expect(line.data).toHaveLength(3);
1080+
}
1081+
});
1082+
1083+
it('remaps a dual-axis line onto all bar band centres across a category union', () => {
1084+
// The bar+line dual-axis path reads band centres for every category from the
1085+
// bar layer's axis ticks. With the union pinned as an explicit discrete
1086+
// domain on both leaves, the band scale must still enumerate categories from
1087+
// BOTH layers so the line's points remap onto real centres.
1088+
const spec: LayerSpec = {
1089+
resolve: { scale: { y: 'independent' } },
1090+
layer: [
1091+
{
1092+
mark: 'bar' as const,
1093+
data: [
1094+
{ yr: '2024', bars: 100 },
1095+
{ yr: '2025', bars: 120 },
1096+
],
1097+
encoding: {
1098+
x: { field: 'yr', type: 'ordinal' as const },
1099+
y: { field: 'bars', type: 'quantitative' as const },
1100+
},
1101+
},
1102+
{
1103+
mark: 'line' as const,
1104+
data: [
1105+
{ yr: '2025', ln: 3 },
1106+
{ yr: '2026', ln: 5 },
1107+
],
1108+
encoding: {
1109+
x: { field: 'yr', type: 'ordinal' as const },
1110+
y: { field: 'ln', type: 'quantitative' as const },
1111+
},
1112+
},
1113+
],
1114+
};
1115+
1116+
const layout = compileLayer(spec, compileOpts);
1117+
1118+
// The x-axis enumerates the ordered union of both layers' categories.
1119+
const tickLabels = (layout.axes.x?.ticks ?? []).map((t) => String(t.label));
1120+
expect(tickLabels).toEqual(['2024', '2025', '2026']);
1121+
1122+
const line = layout.marks.find((m) => m.type === 'line') as unknown as {
1123+
points: { x: number }[];
1124+
};
1125+
expect(line).toBeDefined();
1126+
// Every line point remapped to a finite band centre inside the plot area.
1127+
for (const p of line.points) {
1128+
expect(Number.isFinite(p.x)).toBe(true);
1129+
expect(p.x).toBeGreaterThanOrEqual(layout.area.x);
1130+
expect(p.x).toBeLessThanOrEqual(layout.area.x + layout.area.width);
1131+
}
1132+
});
10251133
});

packages/engine/src/compile/layer.ts

Lines changed: 102 additions & 21 deletions
Original file line numberDiff line numberDiff line change
@@ -225,12 +225,10 @@ function compileLayerIndependent(
225225

226226
const xField0 = leaf0.encoding?.x?.field;
227227
const xField1 = leaf1.encoding?.x?.field;
228-
const unionXValues = new Set<unknown>();
229-
if (xField0) for (const row of leaf0.data) unionXValues.add(row[xField0]);
230-
if (xField1) for (const row of leaf1.data) unionXValues.add(row[xField1]);
231228

232-
let leaf0WithUnionX = ensureXDomainCoverage(leaf0, xField0, unionXValues);
233-
let leaf1WithUnionX = ensureXDomainCoverage(leaf1, xField1, unionXValues);
229+
const union = withUnionXDomain(leaf0, leaf1);
230+
let leaf0WithUnionX = union.leaf0;
231+
let leaf1WithUnionX = union.leaf1;
234232

235233
const aligned = alignYDomains(leaf0WithUnionX, leaf1WithUnionX);
236234
if (aligned) {
@@ -414,28 +412,111 @@ function compileLayerIndependent(
414412
};
415413
}
416414

417-
function ensureXDomainCoverage(
418-
leaf: ChartSpec,
419-
xField: string | undefined,
420-
allXValues: Set<unknown>,
421-
): ChartSpec {
422-
if (!xField || allXValues.size === 0) return leaf;
423-
424-
const existingXValues = new Set<unknown>();
425-
for (const row of leaf.data) existingXValues.add(row[xField]);
415+
/** Merge an x-scale `domain` onto a leaf's x-encoding, preserving other scale config. */
416+
function withXDomain(leaf: ChartSpec, domain: number[] | string[]): ChartSpec {
417+
if (!leaf.encoding?.x) return leaf;
418+
return {
419+
...leaf,
420+
encoding: {
421+
...leaf.encoding,
422+
x: {
423+
...leaf.encoding.x,
424+
scale: {
425+
...leaf.encoding.x.scale,
426+
domain,
427+
},
428+
},
429+
},
430+
} as ChartSpec;
431+
}
426432

427-
const missingRows: DataRow[] = [];
428-
for (const xVal of allXValues) {
429-
if (!existingXValues.has(xVal)) {
430-
missingRows.push({ [xField]: xVal });
433+
/**
434+
* Make both dual-axis layers render against ONE shared x-scale by pinning an
435+
* explicit union x-domain on each, rather than injecting placeholder data rows.
436+
*
437+
* Injecting rows (the old approach) put x-only rows with no y-field into each
438+
* leaf's data. In line/area compute those become null-y points, which are
439+
* treated as line breaks -- so two series with interleaved x-values drew as
440+
* disconnected segments. Pinning the domain achieves the shared axis without
441+
* ever touching the mark data.
442+
*
443+
* Kept separate from computeSharedDomains, which only unions *quantitative*
444+
* channels and folds zero in (correct for a magnitude y-axis, wrong for an x
445+
* position axis that must also cover temporal/ordinal and must not fold zero).
446+
*
447+
* An author-pinned `scale.domain` on a leaf's x wins for that leaf; the union
448+
* is still applied to the other leaf. Note that pinning a domain suppresses
449+
* d3's `.nice()`, so x-axis endpoints become the exact union min/max -- the
450+
* same behaviour the shared-scale path already has.
451+
*/
452+
function withUnionXDomain(
453+
leaf0: ChartSpec,
454+
leaf1: ChartSpec,
455+
): { leaf0: ChartSpec; leaf1: ChartSpec } {
456+
const xEnc0 = leaf0.encoding?.x;
457+
const xEnc1 = leaf1.encoding?.x;
458+
const xField0 = xEnc0?.field;
459+
const xField1 = xEnc1?.field;
460+
if (!xField0 || !xField1) return { leaf0, leaf1 };
461+
462+
// The x-type guard upstream already ensures both encodings agree; take either.
463+
const xType = xEnc0?.type ?? xEnc1?.type;
464+
465+
const pinned0 = xEnc0?.scale?.domain !== undefined;
466+
const pinned1 = xEnc1?.scale?.domain !== undefined;
467+
if (pinned0 && pinned1) return { leaf0, leaf1 };
468+
469+
const values0 = leaf0.data.map((row) => row[xField0]);
470+
const values1 = leaf1.data.map((row) => row[xField1]);
471+
const allValues = [...values0, ...values1].filter((v) => v != null);
472+
if (allValues.length === 0) return { leaf0, leaf1 };
473+
474+
let domain: number[] | string[] | undefined;
475+
476+
if (xType === 'temporal') {
477+
let minMs = Number.POSITIVE_INFINITY;
478+
let maxMs = Number.NEGATIVE_INFINITY;
479+
for (const v of allValues) {
480+
const ms = (v instanceof Date ? v : new Date(String(v))).getTime();
481+
if (Number.isNaN(ms)) continue;
482+
if (ms < minMs) minMs = ms;
483+
if (ms > maxMs) maxMs = ms;
431484
}
485+
if (minMs <= maxMs) {
486+
domain = [new Date(minMs).toISOString(), new Date(maxMs).toISOString()];
487+
}
488+
} else if (xType === 'quantitative') {
489+
let min = Number.POSITIVE_INFINITY;
490+
let max = Number.NEGATIVE_INFINITY;
491+
for (const v of allValues) {
492+
const n = typeof v === 'number' ? v : Number(v);
493+
if (!Number.isFinite(n)) continue;
494+
if (n < min) min = n;
495+
if (n > max) max = n;
496+
}
497+
if (min <= max) domain = [min, max];
498+
} else {
499+
// Discrete x (nominal/ordinal): ordered union of category labels. leaf0's
500+
// categories in data order, then leaf1's not-yet-seen values appended. Pinned
501+
// to BOTH leaves so each band/point scale enumerates the full set -- the bar
502+
// remap in the caller reads band centres for every category from the axis ticks.
503+
const seen = new Set<string>();
504+
const categories: string[] = [];
505+
for (const v of allValues) {
506+
const s = String(v);
507+
if (!seen.has(s)) {
508+
seen.add(s);
509+
categories.push(s);
510+
}
511+
}
512+
if (categories.length > 0) domain = categories;
432513
}
433514

434-
if (missingRows.length === 0) return leaf;
515+
if (domain === undefined) return { leaf0, leaf1 };
435516

436517
return {
437-
...leaf,
438-
data: [...leaf.data, ...missingRows],
518+
leaf0: pinned0 ? leaf0 : withXDomain(leaf0, domain),
519+
leaf1: pinned1 ? leaf1 : withXDomain(leaf1, domain),
439520
};
440521
}
441522

0 commit comments

Comments
 (0)