feat: add amCharts 5 binder for MAIDR (#544)

[jooyoungseo]

Add a new maidr/amcharts entry point that provides adapter functions
to convert amCharts 5 chart instances into MAIDR-compatible JSON data.
This enables audio sonification, text descriptions, braille output, and
keyboard navigation for amCharts 5 visualizations.

Supported chart types: bar/column, line, scatter, and candlestick.

New files:

• src/binder/amcharts/ - Binder module (types, extractors, selectors, adapter)
• src/amcharts-entry.ts - Package entry point for maidr/amcharts
• vite.amcharts.config.ts - Build configuration
• examples/amcharts-bar.html - Usage example

https://claude.ai/code/session_01TB5aCm8C8y3aBP3UgTCGjV

[claude]

PR Review: amCharts 5 Binder for MAIDR

Overall, this is a well-structured addition with good documentation and clean separation of concerns (adapter, extractors, selectors, types). The duck-typed approach correctly avoids a hard dependency on amCharts. Here are my findings:

---

Critical Issues

1. Module-level mutable counter (adapter.ts:290)

let _counter = 0;
function uid(): string {
  return String(++_counter);
}

This is a module-level mutable state problem. In tests or server-side rendering, repeated calls across chart instances will produce different IDs for the same logical data structure, making output non-deterministic. Consider using crypto.randomUUID() or a deterministic ID derived from the chart contents.

2. No tests

The PR introduces ~1,000 lines of new logic (extraction, classification, selector building) but no unit tests. The extraction functions in extractors.ts are pure functions that would be straightforward to test in isolation. Tests for classifySeriesKind, extractBarPoints, extractLinePoints, etc. would significantly improve confidence in the binder's correctness across different chart configurations.

3. Example doesn't exercise the binder (examples/amcharts-bar.html)

The example manually hardcodes the MAIDR JSON instead of calling fromAmCharts(root). This means the example provides no coverage of the actual binder code path and could mislead users. Either use the ES module build in the example or add a comment explaining clearly why the binder can't be used in a plain <script> tag and point to a separate module-based example.

---

Significant Issues

4. toNumber silently converts invalid values to 0 (extractors.ts:248)

function toNumber(value: unknown): number {
  const n = Number(value);
  return Number.isFinite(n) ? n : 0;  // 0 is a valid data value
}

When value is null, undefined, or "N/A", this returns 0 — a potentially valid data point. This can cause silent data corruption. Returning NaN (or filtering at the call site) would be safer and make the failure visible.

5. Series name used as fill in line points (extractors.ts:142)

const point: LinePoint = { x: toStringOrNumber(x), y: toNumber(y) };
if (seriesName) point.fill = seriesName;

fill on a LinePoint semantically represents a visual fill/color category. Using a series name here is a semantic mismatch. MAIDR's line trace uses fill to group points by category (for stacked/segmented lines). If the intent is to label which series a point belongs to, this mapping should be verified against how LineTrace actually uses the fill field to avoid incorrect rendering.

6. lineLayerSelectors mapping may be incorrect (adapter.ts:115-125)

Line series selectors are collected one-per-series into a flat array, but all series are merged into a single multi-line MAIDR layer. This means selectors[0] would map to the first series, but MAIDR's line trace structure (LinePoint[][]) has the series as the first dimension. It's unclear if MaidrLayer.selectors: string[] for a line trace maps index-by-index to each sub-series — if not, the selectors will point to wrong elements during navigation highlighting.

7. Unsafe type assertions in readChartTitle (adapter.ts:245-272)

const children = (chart as unknown as Record<string, unknown>).children;

The function chains multiple type assertions and null checks that can silently fail at any step, resulting in undefined being returned rather than a visible error. At minimum, add a comment that this is intentionally best-effort and document what amCharts version/API this targets.

---

Minor Issues

8. CandlestickPoint.volume defaults to 0 (extractors.ts:196)

volume: volume != null ? toNumber(volume) : 0,

0 is a meaningful volume value. Using it as a sentinel for "not available" could cause incorrect sonification. Consider volume: volume != null ? toNumber(volume) : undefined if the type allows it, or document that volume is read from the X axis and may be absent.

9. buildLineLayer has no title field (adapter.ts:214)

All other layer builders (buildBarLayer, buildScatterLayer, buildCandlestickLayer) set title: seriesName(series). Since line series are merged into one layer, there's no single series name — but a fallback (e.g., "Lines" or the first series name) would be consistent.

10. CurvedColumnSeries classified as bar — correct? (extractors.ts:212)

const COLUMN_CLASSES = new Set(['ColumnSeries', 'CurvedColumnSeries']);

CurvedColumnSeries may render differently from a standard bar chart. Whether MAIDR's bar trace correctly represents it is worth validating and documenting.

11. CSS selector fragility with canvas renderer (selectors.ts)

The module comment acknowledges amCharts 5 uses Canvas by default. The strategies tried (series.columns[0].dom, [data-am-id="..."] rect) may return nothing for most users. The fallback behaviour (returning undefined and skipping highlighting) is correct, but it would be worth noting in the README / docs that visual highlighting is best-effort for amCharts.

12. vite.amcharts.config.ts — missing external for MAIDR's own packages

If consumers import from maidr/amcharts, the amcharts bundle should not re-bundle MAIDR's core packages (@type/grammar, etc.). The rollupOptions has no external list, which means the grammar types will be bundled twice if both maidr and maidr/amcharts are used together.

---

Positive Highlights

• Clean architecture: the binder is self-contained in src/binder/amcharts/ and doesn't pollute existing code.
• The duck-typed interface approach in types.ts is the right call for an optional peer dependency.
• Error thrown by fromAmCharts when no chart is found is appropriate and has a helpful message.
• Good JSDoc coverage throughout.
• The separate vite.amcharts.config.ts keeps the build isolated.

---

Summary

Category	Count
Critical (correctness/reliability)	3
Significant	4
Minor	5

The binder's structure is sound. The two most important blockers before merging are: (a) adding at least basic unit tests for the extraction functions, and (b) verifying the fill field semantics for line traces and the lineLayerSelectors index-to-series mapping for correct highlighting. The _counter module state should also be replaced with a stateless ID strategy.

[claude]

Code Review: feat: add amCharts 5 binder for MAIDR (#544)

This is a well-structured addition that keeps the binder cleanly isolated under src/binder/amcharts/ with a dedicated entry point and build config. The duck-typed interfaces in types.ts are a smart choice for avoiding a hard amCharts dependency. Below are issues ranging from critical to minor.

---

Critical

1. toNumber() silently converts invalid data to 0 (extractors.ts)

function toNumber(value: unknown): number {
  const n = Number(value);
  return Number.isFinite(n) ? n : 0;  // ← silent corruption
}

If amCharts returns null, undefined, NaN, or Infinity for a data point value, it will silently become 0 in the MAIDR data. This means bad data passes validation and produces incorrect audio/text output with no warning. Consider returning null and filtering those points out, or at minimum logging a warning.

2. No tests

There are no unit tests for any of the new code. The extraction logic — especially classifySeriesKind, extractHeatmapData, extractCandlestickPoints, and the segmented bar logic — are non-trivial and could easily have edge-case bugs (see point 4 below). Tests with mock AmXYSeries/AmDataItem objects would be straightforward to write and would prevent regressions.

3. CSS.escape is browser-only (selectors.ts)

return `#${CSS.escape(parentId)} > ${tag}`;

CSS.escape is not available in Node.js (jsdom does not polyfill it by default). This will throw during Jest/Vitest test runs. Either add a polyfill, inline a simple escaper, or guard with typeof CSS !== 'undefined'.

---

Significant

4. Volume field in candlestick extraction is incorrect (extractors.ts)

const volume = item.get('valueX'); // volume sometimes on X

In amCharts 5 candlestick charts, volume is a separate data field (volumeField), not the X-axis value. Reading valueX here will almost always produce 0 or a wrong number. The volume field in CandlestickPoint should probably be optional, or read from a configurable field name, rather than guessing valueX.

5. Global mutable counter for ID generation (adapter.ts)

let _counter = 0;
function uid(): string {
  return String(++_counter);
}

This module-level state persists across calls within the same session. In tests, IDs will differ between runs depending on call order. More importantly, if two charts call fromAmCharts concurrently (e.g., multiple charts on a page), the IDs will be unpredictable. Consider using crypto.randomUUID() (available in modern browsers and Node 14.17+) or Math.random().toString(36).slice(2).

6. Layer ordering does not match series ordering (adapter.ts)

Bar series are collected in a side-list and appended last (after line layers), regardless of their original position in chart.series.values. This means if a chart has [line, bar, scatter] series, the output layers will be [scatter, bar, line]. The series order in amCharts determines render layering, so MAIDR's layer order should match.

7. Selectors are mostly no-ops for Canvas renderer (selectors.ts)

The file's own comment notes that amCharts 5 uses Canvas by default, meaning series.columns.values[0].dom will almost never exist. Both selector strategies in buildColumnSelector will return undefined for the vast majority of users. This is acceptable as a progressive enhancement, but the example and documentation should make it very clear that visual highlighting requires SVG mode. Currently the example HTML gives no indication of this limitation.

---

Minor

8. subtitle: undefined may pollute output (adapter.ts)

const subtitle = options?.subtitle;
// ...
return { id, title, subtitle, subplots: [[subplot]] };

When options?.subtitle is undefined, the key subtitle is still present in the returned object with value undefined. Depending on how MAIDR's grammar validation works, this may cause issues. Prefer omitting the key entirely: ...(subtitle != null ? { subtitle } : {}).

9. readChartTitle is fragile (adapter.ts)

The function traverses chart.children.values looking for className === 'Label' or className === 'Title'. This relies on amCharts internal class naming which could change between patch versions. Since the title is already available via options?.title, consider either removing readChartTitle or making it a best-effort attempt with clear documentation about its fragility.

10. Dead aliases in vite.amcharts.config.ts

The config includes @command, @model, @state, @service, @ui, @util aliases that are not used by the amcharts binder. These add noise and could mask import errors. Only @type is needed.

11. Example does not demonstrate the binder (examples/amcharts-bar.html)

The example manually constructs the MAIDR JSON instead of calling fromAmCharts. The comment explains this is a script-tag limitation, but it means the example doesn't actually test or demonstrate the binder. Consider adding an ES module-based example (e.g., using Vite/Parcel for the demo) so users can see the actual API in action.

12. buildCommonSelector generates overly broad selectors (selectors.ts)

return `#${CSS.escape(parentId)} > ${tag}`;

This matches all direct child <rect>/<path> elements inside the parent, not just the ones for this series. If amCharts renders multiple series inside the same SVG group, the selector will match elements from other series too, producing incorrect highlighting.

13. Horizontal bar detection uses only first series (adapter.ts)

const isHorizontal = typeof barSeriesList[0].get('categoryYField') === 'string';

For segmented charts with mixed orientations (edge case), this could be wrong. Should be consistent, but worth a comment explaining the assumption.

---

Summary

The architecture is clean and well-separated. The main blockers before merging are: adding tests, fixing the silent toNumber corruption, and the CSS.escape Node compatibility issue. The candlestick volume field is also a correctness bug worth fixing. The rest are polish items.

[claude]

Code Review: amCharts 5 Binder

Overall this is a well-structured addition — clean module boundaries, solid JSDoc, and a sensible duck-typed interface layer that avoids requiring amCharts as a hard dependency. The following notes are meant to help harden it before merge.

---

Bugs / Correctness Issues

1. Unguarded axis array access (adapter.ts)

If a chart has no X or Y axis (unusual but possible with partially-initialized charts), chart.xAxes.values[0] will be undefined. While readAxisLabel handles undefined gracefully, using .at(0) makes the intent explicit and is easier to read.

2. Candlestick volume reads from valueX with a sometimes comment (extractors.ts)

valueX is the primary numeric X field in amCharts; using it as a proxy for volume will silently produce wrong output for non-volume charts. Consider defaulting volume to 0 with a console.warn when a dedicated volume field is absent, or adding a volumeField option to AmChartsBinderOptions.

3. Segmented-bar selector is a union, not per-series (adapter.ts)

A union CSS selector will match all bars from all series at once. SegmentedTrace likely needs selectors scoped per series to highlight individual segments. The combined selector could inadvertently highlight the entire chart on any navigation step.

4. classifySeriesKind silently defaults unknown types to bar (extractors.ts)

The final return 'bar' will silently run extractBarPoints on completely unrelated series types that happen to have a categoryXField. Returning 'unknown' and emitting a console.warn would surface the issue rather than producing corrupted data.

---

Design / Architecture Concerns

5. Global mutable counter causes brittle tests (adapter.ts)

This module-level counter persists across fromAmCharts calls within the same JS process. Two consecutive test runs (or snapshot tests) will see different IDs, making tests non-deterministic. uid() is also identical to counter() — the separation adds no value. Using crypto.randomUUID() (or a simpler per-invocation approach) would be more robust.

6. readChartTitle relies on amCharts internal class names (adapter.ts)

These are internal amCharts class names, not guaranteed stable across patch versions. Any amCharts update that renames these will silently drop the chart title. At minimum, add a comment documenting which amCharts 5 version this was verified against.

7. CSS selector fallback matches all elements with a shared class (selectors.ts)

This produces a selector that matches every element on the page with that class, not just the ones belonging to this series. MAIDR would then attempt to highlight the wrong elements. When a reliable per-element selector cannot be built, returning undefined is the correct behavior — outer callers already handle undefined correctly.

---

Example Code Issue

8. setTimeout in the example is an anti-pattern (examples/amcharts-bar.html)

The comment says In production, listen to chart.events.on('datavalidated', ...) — but the example demonstrates the wrong approach. A fixed 1000 ms delay will fail on slow machines and unnecessarily delays fast ones. Since this is likely the first code a new user copies, please use the event-based approach instead.

---

Missing Tests

9. No tests for the binder module

The data extraction logic — especially classifySeriesKind, extractBarPoints, extractHistogramPoints, and extractHeatmapData — is complex enough to warrant unit tests with mock amCharts-like objects (no real amCharts dependency needed). Without tests, regressions in the extraction logic will only be caught at runtime. Even a small suite covering happy-path and null/missing-field cases for each extractor would greatly improve confidence.

---

Minor Nits

10. Unused path aliases in vite.amcharts.config.ts

The binder module imports nothing from @command, @model, @State, @service, or @ui. Those aliases appear copy-pasted from the main Vite config. Removing the unused ones keeps the build config lean.

11. src/binder/amcharts/index.ts duplicates src/amcharts-entry.ts

Both files re-export the same symbols from adapter.ts. If index.ts is for internal use only, a short comment to that effect would clarify the intent. Otherwise, consider collapsing the two files.

---

Summary

Area	Status
Module boundaries	Clean
Duck-typed types (no amCharts peer dep)	Good approach
Axis label / title reading	Fragile reflection
Candlestick volume field	Weak assumption
Segmented selector (union vs per-series)	Potential highlight bug
Global mutable counter	Non-deterministic IDs across tests
CSS selector fallback	Matches wrong elements
Example uses setTimeout	Anti-pattern in example
Test coverage	Missing

The core data extraction approach is sound. The highest-priority items before merge are the selector correctness issues (#3, #7), the example anti-pattern (#8), and adding minimal tests (#9).