feat: add D3.js charting library support via maidr/d3 binder

[jooyoungseo]

Add a JavaScript binder that extracts data from D3.js-rendered SVG charts
and generates the MAIDR JSON schema for accessible chart interaction.

Supported chart types:

• Bar charts (bindD3Bar)
• Line charts, single and multi-line (bindD3Line)
• Scatter plots (bindD3Scatter)
• Heatmaps (bindD3Heatmap)
• Box plots (bindD3Box)
• Histograms (bindD3Histogram)

The binder leverages D3's data property on DOM elements to extract
bound data, and generates CSS selectors for SVG highlighting. Available
as both ES module (maidr/d3) and UMD script tag (dist/d3.js).

Closes #537

https://claude.ai/code/session_01XrcqYMsCcxfUwhLDpzEwr3

[claude]

PR Review: D3.js Binder (#551)

This is a well-conceived feature — bridging D3.js charts to MAIDR's accessibility layer fills an important gap. The documentation and examples are excellent. Below is a structured review of the issues I found.

---

Critical Issues

1. resolveAccessor silently returns undefined for missing properties (util.ts:45)

// Returns T, but actually returns T | undefined when the property doesn't exist
return (datum as Record<string, T>)[accessor];

This propagates undefined into typed fields across all binders. For example in bindBar.ts:66-67:

return {
  x: resolveAccessor<string | number>(datum, xAccessor, index),  // could be undefined
  y: resolveAccessor<number | string>(datum, yAccessor, index),  // could be undefined
};

TypeScript doesn't flag this because the return type says T, not T | undefined. At runtime MAIDR receives data with undefined field values which will likely cause silent failures downstream. The function should either throw with a descriptive error when a string accessor finds no property, or return T | undefined and force callers to handle it.

2. bindLine with pointSelector queries the wrong scope (bindLine.ts:61)

for (const { element } of lineElements) {
  const parent = element.parentElement ?? svg;
  const points = queryD3Elements(parent, pointSelector);
  // ...
}

If multiple line paths share a parent <g>, queryD3Elements(parent, pointSelector) will find all point circles across all lines for each iteration. A chart with 3 lines and 10 points each will produce 3 line series each containing 30 points (10 × 3), tripling the data. The selector needs to be scoped to the specific line's sibling group, not its parent element.

Additionally, in bindLine.ts:88, the same selector string is pushed once per line path, producing a duplicate array like [".scope circle", ".scope circle", ".scope circle"].

3. Heatmap fills missing cells with 0 silently (bindHeatmap.ts:94)

row.push(cellMap.get(`${yLabel}::${xLabel}`) ?? 0);

If D3 renders a sparse heatmap or if the x/y accessors are misconfigured, missing cells become 0. This is indistinguishable from a genuine zero value. At minimum this should warn:

const value = cellMap.get(`${yLabel}::${xLabel}`);
if (value === undefined) {
  console.warn(`[MAIDR D3] Missing heatmap cell at (${xLabel}, ${yLabel}). Defaulting to 0.`);
}
row.push(value ?? 0);

---

High-Priority Issues

4. extractBarDataFromDOM fallback uses pixel dimensions as data values (util.ts:172-175)

if (orientation === 'vert') {
  return { x: x + width / 2, y: height };  // height in pixels ≠ data value
}

This fallback produces pixel measurements from SVG attributes rather than actual data values. A bar that is 200px tall and represents the value 5,000 will report y: 200, which is wrong for sonification/braille/text. This function should be removed, or the fallback should throw with a clear message that D3 data binding is required.

5. bindBox outlier try-catch swallows errors and redundant null-coalescing (bindBox.ts:83-105)

let lowerOutliers: number[];
try {
  lowerOutliers = resolveAccessor<number[]>(effectiveDatum, lowerOutliersAccessor, index);
} catch {
  lowerOutliers = [];  // silently fails
}
// ...
lowerOutliers: lowerOutliers ?? [],  // ?? [] is dead code — already set in catch

The ?? [] at the end is unreachable since the catch block always sets lowerOutliers = []. More importantly, swallowing the error without logging means users won't know why their outlier data is missing. The outlier accessors should be optional with an explicit undefined check instead of try-catch.

6. Inconsistent selectors format across binders

• bindBar → selectors: scopeSelector(svg, selector) (string)
• bindScatter → selectors: [scopeSelector(svg, selector)] (array)
• bindLine → selectors: selectors.length === 1 ? selectors[0] : selectors (string or array)

If MAIDR's highlight logic behaves differently for string vs string[], some charts will highlight incorrectly. This should be consistent. Check the expected type in MaidrLayer.selectors and ensure all binders match.

7. Heatmap composite key uses :: which may collide (bindHeatmap.ts:88)

cellMap.set(`${cell.y}::${cell.x}`, cell.value);

If a category label contains :: (e.g., "a::b"), this key will collide with other cells. Use a Map<string, Map<string, number>> structure instead.

---

Medium-Priority Issues

8. No test coverage

There are zero tests for any file in src/d3/. This is the highest-risk surface of the PR because:

• DOM querying behavior is environment-dependent
• resolveAccessor has the undefined-return bug that only manifests at runtime
• The bindLine pointSelector scope bug would be immediately caught by a test

At minimum, unit tests for util.ts functions and one integration test per binder (using jsdom + fixture SVGs) are needed before merging.

9. vite.d3.config.ts defines unused path aliases

resolve: {
  alias: {
    '@command': path.resolve(__dirname, 'src/command'),
    '@model': path.resolve(__dirname, 'src/model'),
    // ...
  }
}

None of the src/d3/ source files use these aliases. They should be removed to keep the config minimal and avoid confusion.

10. generateSelector may produce non-unique selectors (util.ts:108-117)

The nth-of-type index is calculated relative to the immediate parent's children. For deeply-nested SVG structures where multiple elements share the same tag name and class at the same nesting level, the generated selector may still match multiple elements, defeating the purpose of per-element highlighting.

---

Minor Issues

• util.ts:147: ID generation uses Date.now() + Math.random(). While collision probability is low in practice, two charts initialized in the same millisecond could get the same ID prefix. crypto.randomUUID() is available in modern browsers/Node and would be more robust.

• types.ts:57-59: x?: DataAccessor<string | number> and y?: DataAccessor<number | string> have reversed type union order. This is harmless but inconsistent.

• bindHistogram.ts:76: Number(yValue) can produce NaN silently if yValue is a non-numeric string. The result should be validated.

• index.ts formatting: The blank line between bindD3Histogram and bindD3Line exports is inconsistent with the rest of the file.

---

What's Working Well

• The overall architecture cleanly separates DOM querying (util.ts), type definitions (types.ts), and per-chart logic (bind*.ts)
• Error messages on missing D3 data are clear and actionable
• The JSDoc examples are thorough and cover the three main usage patterns (script tag, ES modules, React)
• The heatmap's label-ordering-preserving logic (using seenX/seenY Sets) is correct
• CSS.escape() usage in generateSelector is the right approach for sanitizing IDs/classes

[claude]

PR Review: feat: add D3.js charting library support via maidr/d3 binder

This is a substantial and well-documented addition. The architecture is clean — a separate entry point (maidr/d3), a dedicated Vite config, consistent utility abstraction in util.ts, and comprehensive JSDoc examples. The decision to leverage D3's __data__ property rather than trying to reverse-engineer SVG geometry is the right approach.

That said, there are several issues ranging from potential bugs to missing coverage that should be addressed before merging.

---

Bugs / Correctness

1. bindLine.ts: Multi-line processedPoints logic is likely broken

When multiple path.line elements share the same parent <g> (the most common D3 multi-line pattern), the second line path will find the same point elements as the first pass. All are already in processedPoints, so the second (and all subsequent) lines produce zero data points and are silently dropped from data. The comment says the Set prevents double-counting, but it actually prevents all lines except the first from being extracted.

A correct approach is to scope point queries to the specific line's own group element, or associate points with their line series by index/data key rather than by DOM identity.

---

2. generateSelector returns '' when element === container

The while (current && current !== container) loop never executes if element is the container itself, leaving parts empty and returning ''. An empty selector matches every element on the page. This edge case should be guarded.

---

3. bindD3Segmented: incompatible with D3's built-in d3.stack() output

d3.stack() produces data where each datum is [y0, y1] (a two-element array with baseline and top values), not a flat { x, y, fill } object. Users who build a stacked bar with standard D3 utilities will get a cryptic "Property 'y' not found on datum" error. The docs/examples should prominently warn about this, or the binder should add native support for the [y0, y1] tuple format.

---

Reliability

4. No guard for empty selector results

A typo in the selector silently produces MAIDR data with empty arrays, which causes confusing runtime failures later. An early guard surfaces the problem at the point of misconfiguration:

if (elements.length === 0) {
  throw new Error(
    `No elements matched selector "${selector}" within the provided SVG. ` +
    `Verify the selector matches the rendered chart elements.`
  );
}

---

5. generateId() can collide when called multiple times in the same millisecond

A dashboard that initialises several charts synchronously will get the same Date.now() prefix on every call. Prefer crypto.randomUUID() (available in all modern browsers and Node >= 14.17) or a module-level counter for guaranteed uniqueness.

---

6. CSS.escape is browser-only

generateSelector calls CSS.escape(), which is undefined in Node.js. Users doing SSR (Next.js, Gatsby, etc.) will get a ReferenceError. A lightweight polyfill or try/catch fallback in util.ts would make the binder portable.

---

Test Coverage

No tests were added for any of the 9 binder functions. This is a significant gap for ~2,600 lines of new code. Recommended coverage includes:

• Correct MAIDR schema shape for each binder type
• resolveAccessor with both a string key and a function accessor
• Error is thrown when __data__ is absent
• Multi-line extraction actually produces separate arrays per line (catches the processedPoints bug)
• Heatmap grid construction with non-square grids and a missing cell

---

Minor / Style

Axes spread pattern duplicated 9x: Extract axes ? { ...axes, ...(format ? { format } : {}) } : undefined into a buildAxes(axes, format) helper in util.ts to keep all binders DRY.

layerId variable is only used immediately in the same object literal — consider inlining it or documenting why it is a named variable.

Example files require a prior build (../dist/). A short note at the top of each example or a brief examples/README.md would prevent confusion for new contributors.

---

Summary

Area	Status
Architecture / API design	Good
Documentation and examples	Good
Multi-line chart extraction	Bug — fix before merge
Empty selector handling	Missing guard
D3 stack layout compatibility	Needs clarification or fix
ID generation	Fragile — prefer crypto.randomUUID()
SSR / Node.js portability	CSS.escape issue
Test coverage	None — add before merge
Repeated axes helper	Minor refactor opportunity

[claude]

PR Review: feat: add D3.js charting library support via maidr/d3 binder

This is a well-structured, well-documented feature that fills an important gap for D3.js users. The architecture is consistent across all binders, the JSDoc is excellent, and the error messages are helpful. Here is a detailed review with findings organized by severity.

---

Architecture & Design

Strengths:

• Clean module separation: types.ts, util.ts, per-chart binder files, and index.ts
• Consistent public API across all binders — same config shape, same return type
• Smart dual-pattern handling in bindD3Line (Pattern A: distinct parent per series, Pattern B: shared parent) is well-thought-out
• DataAccessor<T> = string | ((datum: unknown, index: number) => T) gives flexibility for both simple property access and computed values
• Returning { maidr, layer } from every binder enables composition (e.g., multi-layer subplots)

Concern:

• The binders live entirely outside the MVVC architecture (no Model/Service/ViewModel involved). This is appropriate since it is a standalone binder library, but it means no integration tests against the actual MAIDR runtime are included.

---

Bugs & Issues

Medium — Legend extraction in bindLine.ts (line 179)

const legend: string[] = [];
for (const lineData of data) {
  const fill = lineData[0]?.fill;  // only reads first point
  if (fill) {
    legend.push(fill);
  }
}

If the first point of a line has no fill (e.g., Pattern A where points do not carry a fill property), the line is silently omitted from the legend. With Pattern B (shared parent), fill is correctly set on all points, but Pattern A path does not guarantee fill on the first point.

Suggested fix:

for (const lineData of data) {
  const fill = lineData.find(p => p.fill)?.fill;
  if (fill) legend.push(fill);
}

Medium — Outlier type not validated at runtime in bindBox.ts (line 115)

const lowerOutliers = resolveAccessorOptional<number[]>(effectiveDatum, lowerOutliersAccessor, index) ?? [];

resolveAccessorOptional does a bare cast with no runtime check. If the accessor points to a non-array value (e.g., a string "5,8" from legacy data), this will pass TypeScript but fail at downstream array iteration. Suggested guard:

const rawLower = resolveAccessorOptional<unknown>(effectiveDatum, lowerOutliersAccessor, index) ?? [];
if (!Array.isArray(rawLower)) {
  throw new Error(`Expected array for lowerOutliers at index ${index}, got ${typeof rawLower}`);
}
const lowerOutliers = rawLower as number[];

Low — Number(yValue) fallback can silently produce NaN in bindHistogram.ts (line 90)

const yMax = yMaxAccessor
  ? resolveAccessor<number>(datum, yMaxAccessor, index)
  : Number(yValue);  // NaN if yValue is a non-numeric string

If yValue is something like "45 units", Number("45 units") is NaN, which silently breaks sonification. Consider: typeof yValue === 'number' ? yValue : parseFloat(String(yValue)).

Low — Group key type assumption in bindSegmented.ts (line 116)

const groupKey = (groupDatum as Record<string, unknown> | null)?.key as string | undefined;

In d3.stack() output, .key is typically a string, but there is no runtime validation. If someone uses numeric keys (d3.stack().keys([1, 2, 3])), groupKey would be a number silently treated as a string. Safer:

const rawKey = (groupDatum as Record<string, unknown> | null)?.key;
const groupKey = rawKey != null ? String(rawKey) : undefined;

Low — CSS escape fallback in util.ts (line 26)

return value.replace(/([^\w-])/g, '\\$1');

For some characters (like spaces), the correct CSS escape is a Unicode code point (e.g., \20), not a backslash-escaped character. Low risk since this only affects SSR/Node.js environments without CSS.escape, but worth noting if SSR support is ever needed.

---

Missing Test Coverage

No test files are included in this PR. For a new module with this many conditional code paths, tests are essential before merging. Critical cases to cover:

1. bindD3Line — Pattern A vs Pattern B detection; legend extraction when fill is missing from first point; the else-branch reading array data directly from path __data__
2. bindD3Box — missing __data__ on group element (fallback to first child); non-array outlier accessor return values
3. bindD3Heatmap — sparse grid (missing cells) should throw a clear error
4. bindD3Segmented — groupSelector path with d3.stack() key; flat structure grouping order preservation
5. util.ts — generateSelector for elements with no ID, no class, deeply nested

Smoke tests using jsdom (mock SVG with __data__ set on elements, asserting the MAIDR schema structure) would provide good baseline coverage.

---

Minor / Style

• vite.d3.config.ts: insertTypesEntry: false is unusual for a library build. Most library builds want true to register the package's type entry. If intentional, a brief comment explaining why would help.

• bindLine.ts doc comment: Says "each line path and its associated points must share the same parent group element" — this describes Pattern A only. The wording may confuse users of Pattern B charts.

• generateSelector comment in util.ts: Says nth-child is "more reliable than nth-of-type in deeply nested SVG structures." This is debatable — nth-of-type filters by element type and is often more stable in SVG when mixed element types are siblings. Consider revising the comment.

---

Summary

Category	Status
Code structure and API design	Good
Documentation and JSDoc	Excellent
Error messages	Good
Type safety	Minor gaps (outlier validation, group key coercion)
Edge case handling	Legend extraction, NaN yMax
Test coverage	Missing
Build configuration	Good (one minor question)

The implementation is solid and ready for integration with a few targeted fixes. The missing tests are the most pressing concern — the binders have enough conditional logic paths (line chart Pattern A/B, box plot child fallback, segmented flat vs grouped) that regressions could go undetected without them.