feat: add Chart.js charting library support (#538)

[jooyoungseo]

Add a Chart.js plugin that automatically makes canvas-based charts
accessible through MAIDR's audio sonification, text descriptions,
braille output, and keyboard navigation.

Key changes:

• New maidr/chartjs entry point with maidrPlugin and extractMaidrData
• Data extraction for bar, line, scatter, pie, and doughnut chart types
• Canvas highlighting via Chart.js setActiveElements API bridge
• onNavigate callback on Maidr type for canvas-based highlight integration
• Navigate callback observer registration in Controller
• Vite build config for the Chart.js entry (vite.chartjs.config.ts)
• chart.js v4+ as optional peer dependency

https://claude.ai/code/session_01JWjCS4jngMUYvbcqRq7Qbn

[claude]

PR Review: feat: add Chart.js charting library support (#538)

This is a well-structured addition that opens MAIDR accessibility to a large ecosystem of canvas-based charts. The architecture (plugin → extractor → controller bridge) is clean and the documentation comments are thorough. Below are issues that should be addressed before merge.

---

Bug: Wrong row/col values in registerNavigateCallback

File: src/controller.ts:392-396

callback({
  layerId: state.layerId,
  row: state.audio.panning.y,   // semantic mismatch
  col: state.audio.panning.x,   // semantic mismatch
});

panning.x and panning.y are audio positioning values, not dataset/point indices. For horizontal bar charts, they are deliberately swapped:

// src/model/bar.ts
panning: {
  x: isVertical ? this.col : this.row,  // x = row index when horizontal
  y: isVertical ? this.row : this.col,  // y = col index when horizontal
}

So for a horizontal bar chart, row = panning.y = this.col and col = panning.x = this.row, which passes the wrong values to Chart.js setActiveElements({ datasetIndex: row, index: col }).

The fix should expose explicit navigation indices from TraceState, or use state.braille.row/state.braille.col which consistently track position regardless of chart orientation.

---

Bug: Silent data loss for object-typed dataset values in bar extraction

File: src/chartjs/extractor.ts:124

y: typeof value === 'number' ? value : (value as null) === null ? 0 : 0,

Both branches of the inner ternary return 0, making this equivalent to typeof value === 'number' ? value : 0. If value is { x: number, y: number } (valid in ChartJsDataset.data), the y-coordinate is silently dropped. Should be:

y: typeof value === 'number'
  ? value
  : value \!== null && typeof value === 'object' && 'y' in value
    ? (value as { x: number; y: number }).y
    : 0,

---

Architecture concern: onNavigate mutates the data object

File: src/chartjs/plugin.tsx:156, src/type/grammar.ts:131

maidrData.onNavigate = createHighlightCallback(chart);

The Maidr interface is a JSON schema/data type, but onNavigate is an imperative callback attached after construction. This mixes data and behavior concerns. Consider passing the callback separately to the Maidr React component as a prop, or accepting it in a dedicated factory separate from the data schema.

The current approach also means onNavigate appears on the public Maidr type that consumers see when constructing <Maidr data={...}> objects, which is confusing since it has no meaningful use outside the Chart.js plugin context.

---

Multi-dataset bar charts lose highlight consistency

File: src/chartjs/extractor.ts:95-99

Multi-dataset bar charts become multiple independent MAIDR layers. When the user navigates between layers, the previously highlighted bar in Chart.js is not cleared. An explicit chart.setActiveElements([]) call before setting the new element, or clearing on layer switch, would prevent stale highlights.

---

No live data update support

File: src/chartjs/plugin.tsx:182

extractMaidrData is called once in afterInit. If the chart data changes later (a very common Chart.js pattern), MAIDR will describe stale data. At minimum, document this limitation prominently. Implementing the afterUpdate plugin hook to re-extract data when the chart updates would make the plugin significantly more useful for interactive charts.

---

Potential duplicate React roots

File: src/chartjs/plugin.tsx:140

initMaidrForChart does not guard against being called twice for the same chart instance. Add an early return if the binding already exists:

function initMaidrForChart(chart: ChartJsChart): void {
  if (chartBindings.has(chart)) return;
  const pluginOptions = ...;
  ...
}

---

Unsafe cast for plugin options

File: src/chartjs/plugin.tsx:143

const pluginOptions = (chart.options.plugins?.maidr ?? {}) as MaidrPluginOptions;

chart.options.plugins.maidr is unknown. The cast is unsafe and could cause silent runtime errors with invalid options. A type guard or validation step would be safer.

---

Minor: DomNodeAdapter is a fragile DOM manipulation pattern

File: src/chartjs/plugin.tsx:56-67

Moving the canvas into a React tree via an imperative ref callback can behave unexpectedly in concurrent React rendering. Using ReactDOM.createPortal would be cleaner and more predictable for adopting an existing DOM node.

---

Minor: ID generation not guaranteed unique

File: src/chartjs/extractor.ts:28

Date.now() can collide for charts initialized synchronously in the same millisecond. Consider a monotonic counter or crypto.randomUUID() as the fallback.

---

No tests

There are no unit tests for extractor.ts or integration tests for the plugin lifecycle. The data extraction logic has multiple code paths (bar, line, scatter, pie, multi-dataset). Even a small suite of unit tests for extractMaidrData would significantly improve confidence and catch regressions.

---

Summary

Severity	Issue
Bug	Wrong row/col values from panning in navigate callback
Bug	Object-typed dataset values silently become 0 in bar extraction
Architecture	onNavigate mutates data schema object; pollutes public type
Correctness	Multi-dataset bar chart highlight not cleared on layer switch
UX	No support for chart data updates after initialization
Safety	No guard against duplicate React roots in initMaidrForChart
Safety	Unsafe plugin options cast from unknown
Minor	Fragile canvas DOM adoption via ref callback
Minor	Non-unique Date.now() fallback for chart ID
Missing	Tests for extraction and plugin lifecycle

[claude]

Code Review

Great addition. Chart.js integration fills an important gap for canvas-based accessibility. The overall architecture is clean and using Chart.js native plugin lifecycle hooks is the right approach. Below are findings organized by severity.

High Priority

1. controller.ts — Navigate callback uses braille state for position

registerNavigateCallback reads state.braille.row/state.braille.col to drive navigation position:

if (!state.empty && !state.braille.empty) {
  callback({
    layerId: state.layerId,
    row: state.braille.row,
    col: state.braille.col,
  });
}

Two problems here:

• The guard !state.braille.empty means the callback never fires when braille is disabled, silently breaking canvas highlighting for users who do not use braille output.
• Braille state is a display/output concern. The canonical navigation position should come from the trace's own position state, not from a display-layer artifact.

The callback should read row/col from the trace's navigation state directly, not from state.braille.

---

2. plugin.tsx — DomNodeAdapter cleanup removes canvas from DOM

} else {
  node.parentNode?.removeChild(node);
}

When the React subtree unmounts, this removes the canvas element entirely from the DOM. Chart.js still holds a reference to chart.canvas and may try to render into it, or the canvas simply disappears before Chart.js's own cleanup runs. The canvas should be moved back to its original parent position on cleanup. Capture the original parent and next sibling at mount time and restore them in the cleanup branch.

---

3. plugin.tsx — Throwing from a Chart.js plugin hook can break the chart

if (!parent) {
  throw new Error('MAIDR Chart.js plugin: canvas must be in the DOM');
}

Throwing from afterInit propagates up through Chart.js initialization and can prevent the chart from rendering entirely for all users. Use console.error + early return so MAIDR degrades gracefully without crashing the chart.

---

Medium Priority

4. extractor.ts — ID collision when canvases share the same canvas.id

nextId only increments when canvas.id is falsy. Two charts sharing the same non-empty canvas.id receive identical MAIDR IDs causing DOM ID conflicts. Consider always appending the counter, or use crypto.randomUUID().

---

5. extractor.ts — Silent fallback for unknown chart types

The default branch silently produces bar chart data for unknown types. Add a console.warn before the fallback so developers know an unsupported type was encountered.

---

6. extractor.ts — Multi-series scatter plots lose dataset distinction

All scatter datasets are merged into one flat array. A chart with two scatter series loses its series structure entirely. Consider mapping each dataset to a separate MAIDR layer, or document this limitation explicitly.

---

7. plugin.tsx — createHighlightCallback row/col mapping assumption

chart.setActiveElements([{ datasetIndex: row, index: col }]);

The mapping row → datasetIndex, col → element index holds for simple charts but breaks for segmented/stacked bars, which are a single MAIDR layer with a 2D data array but correspond to multiple Chart.js datasets. The highlight bridge needs chart-type-aware mapping, not a generic passthrough.

---

8. extractor.ts — Hardcoded volume: 0 for candlestick

Volume is silently dropped and hardcoded to 0. If CandlestickPoint surfaces volume in text descriptions, users will always hear "volume: 0". Either extract it from the data or add a comment noting it is intentionally omitted.

---

9. extractor.ts — Math.max with empty spread produces -Infinity

const numCategories = Math.max(labels.length, ...data.datasets.map(ds => ds.data.length));

If data.datasets is ever empty when this runs, Math.max() returns -Infinity. Worth adding a guard even if the current call path prevents it today.

---

Low Priority / Suggestions

10. extractor.ts — Global mutable nextId module state

Module-level mutable counters complicate unit testing (they do not reset between tests) and behave unexpectedly with HMR. crypto.randomUUID() or a counter scoped per invocation would be cleaner.

---

11. extractor.ts — isPointValue uses brittle negative predicates

return v != null && typeof v === 'object' && 'x' in v && 'y' in v && !('o' in v) && !('v' in v) && !('median' in v);

This relies on the absence of other properties to distinguish types. A future plugin data type with x and y but without o, v, or median would match incorrectly. Checking typeof v.x === 'number' && typeof v.y === 'number' as positive assertions is more robust.

---

12. extractor.ts — Heatmap composite key collision

valueMap.set(`${x}|${y}`, point.v);

If a label contains |, keys can collide (e.g. label "a|b" with "c" collides with "a" and "b|c"). A nested Map<string, Map<string, number>> or JSON.stringify([x, y]) as the key is safe.

---

13. grammar.ts — Non-serializable function field on a schema type

onNavigate?: NavigateCallback on Maidr mixes the JSON data schema with runtime behavior. Anyone who calls JSON.stringify on their MAIDR config will silently lose this field. A more explicit separation such as a separate React prop or context would avoid the surprise.

---

14. Missing tests

This PR adds around 800 lines of new logic. The extractor functions are pure transformations that are straightforward to unit test. High-value cases:

• extractBarLayers for single/multi-dataset, stacked vs dodged
• extractScatterLayers with multi-series input
• extractHeatmapLayers with labels containing |
• isBoxplotValue / isPointValue guards with edge-case inputs
• Navigate callback firing (or not) based on state conditions, especially after fixing the braille state dependency

---

Overall this is a solid, well-structured implementation. The three high-priority items (braille state dependency, canvas DOM cleanup, and throwing from a plugin hook) should be addressed before merge. The test coverage gap is also worth filling given the complexity of the extraction logic.

[claude]

Code Review: Chart.js Integration

This is a solid feature that meaningfully expands MAIDR's reach to canvas-based charts. The plugin/extractor separation is clean, the type definitions are minimal and well-scoped, and the use of Chart.js's native plugin lifecycle hooks is the right approach. A few issues need attention before merge.

---

Bug: Canvas disappears from DOM after Chart.js chart destruction

File: src/chartjs/plugin.tsx — destroyMaidrForChart + DomNodeAdapter

These two cleanup paths race against each other:

// destroyMaidrForChart does these steps in order:
parent.insertBefore(canvas, binding.container);  // (1) move canvas back
binding.root.unmount();                           // (2) triggers React cleanup →
//   DomNodeAdapter ref cleanup fires:
//   node.parentNode?.removeChild(node);          // (3) removes canvas again!
binding.container.remove();                       // (4) too late

After destroyMaidrForChart, the canvas is gone from the DOM because DomNodeAdapter's cleanup runs after the canvas is moved back to its original parent, and it calls removeChild on it.

Fix: Either call root.unmount() before moving the canvas back (so DomNodeAdapter cleanup fires while the canvas is still inside the container), or guard the cleanup branch in DomNodeAdapter to only remove if the parent is the managed container.

---

Bug: Canvas highlighting silently breaks when braille is disabled

File: src/controller.ts:392–403

if (!state.empty && !state.braille.empty) {
  callback({
    layerId: state.layerId,
    row: state.braille.row,
    col: state.braille.col,
  });
}

Two problems:

1. The guard !state.braille.empty means the navigate callback never fires when braille output is disabled, silently breaking canvas highlighting for users who have braille turned off.

2. state.braille.row / state.braille.col are display-layer output values, not canonical navigation indices. The authoritative position should come from the trace's own navigation state, not from a rendering artifact of a specific output modality.

The callback should use the trace's position directly and guard only on !state.empty.

---

Bug: isBoxplotValue type guard is too permissive

File: src/chartjs/extractor.ts:226

function isBoxplotValue(v: ChartJsDataValue): v is { min: number; q1: number; median: number; q3: number; max: number; outliers?: number[] } {
  return v != null && typeof v === 'object' && 'median' in v;
}

This only checks for the presence of 'median', so any object with a median property—including future plugin types—will be treated as a boxplot. At minimum, also check for 'q1' in v && 'q3' in v to require the full boxplot structure.

---

Design: onNavigate on the data schema type

File: src/type/grammar.ts:131, src/react-entry.ts

Adding a function callback to Maidr (which is otherwise a JSON-serializable data schema) creates problems:

• JSON.stringify silently drops onNavigate, which can surprise users who serialize their MAIDR config.
• NavigateCallback is re-exported from react-entry.ts as a public API, but it's meaningless outside the Chart.js plugin context—it will appear in the public type docs with no obvious use case.
• The Maidr data object is supposed to be a declarative description of a chart; adding a callback turns it into a mix of data and behavior.

Consider accepting the callback separately—as an additional prop on <MaidrComponent>, or as a second parameter to the internal controller—rather than embedding it in the data schema.

---

No live data update support

File: src/chartjs/plugin.tsx:199

extractMaidrData is called once in afterInit. Updating chart data (a common Chart.js pattern for interactive dashboards) produces stale MAIDR audio/text descriptions with no error. The afterUpdate plugin hook could re-extract and re-render, or at minimum this limitation should be documented prominently in the JSDoc.

---

Minor: isPointValue relies on negative property predicates

File: src/chartjs/extractor.ts:222

return v != null && typeof v === 'object' && 'x' in v && 'y' in v && !('o' in v) && !('v' in v) && !('median' in v);

Distinguishing types by the absence of properties is fragile. A future plugin data shape with x/y and some other property would silently mis-classify. Positive assertions on typeof v.x === 'number' && typeof v.y === 'number' are more robust.

---

Minor: Hardcoded volume: 0 for candlestick silently drops data

File: src/chartjs/extractor.ts:312

volume: 0,  // Chart.js financial plugin does not include volume data; default to 0

If CandlestickPoint is surfaced in text descriptions, users navigating a candlestick chart will always hear "volume: 0". Worth noting the omission in the comment and checking whether the MAIDR text output for candlestick uses the volume field.

---

Minor: Module-level nextId complicates testing

File: src/chartjs/extractor.ts:150

let nextId = 0;

Module-level mutable state does not reset between test runs, meaning test ordering affects ID values. crypto.randomUUID() or scoping the counter per call would be cleaner.

---

Missing tests

The extractor functions are pure transformations and are straightforward to unit test. Given ~850 lines of new logic with multiple code paths, test coverage would significantly improve confidence. High-value cases:

• extractBarLayers — single dataset, multi-dataset stacked vs. dodged
• extractScatterLayers — multi-series preserves separate layers
• extractHeatmapLayers — label ordering and value mapping
• isBoxplotValue / isPointValue — edge-case inputs
• Navigate callback firing when state.braille.empty is true (after the braille guard fix)

---

Summary

Priority	Issue
Bug	Canvas removed from DOM after chart destruction (cleanup race)
Bug	Canvas highlighting breaks when braille is disabled
Bug	isBoxplotValue too permissive — only checks 'median'
Design	onNavigate adds runtime behavior to a data schema type
Correctness	No afterUpdate support; stale data after chart updates
Minor	isPointValue fragile negative predicates
Minor	volume: 0 silently drops candlestick volume
Minor	Module-level nextId complicates testing
Missing	Unit tests for extraction logic

The canvas cleanup race and braille guard are the most impactful bugs to fix before merge. The rest can be addressed iteratively.