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).

[Copilot]

Copilot encountered an error and was unable to review this pull request. You can try again by re-requesting a review.

[claude]

Code Review: amCharts 5 Binder (#544)

Overview

This PR adds a complete amCharts 5 adapter that converts amCharts 5 XYChart instances into MAIDR-compatible JSON, with optional canvas highlight overlay support. The implementation is well-structured and closely mirrors the existing Chart.js adapter pattern. The core design decisions are sound — duck-typed interfaces avoid requiring amCharts as a hard dependency, the overlay approach is appropriate for a canvas-rendered chart library, and the two-path API (fromAmCharts vs bindAmCharts) gives integrators the right level of flexibility.

---

Bug / Correctness Issues

1. findXYChart is duplicated between adapter.ts and binder.tsx. Both implement the identical duck-type check. Either re-export it from adapter.ts or extract it to a shared internal helper to avoid the two implementations diverging.

2. classifySeriesKind fall-through returns 'bar' for any unrecognized series class:

// Default to bar for category-based series.
return 'bar';

A series that is neither COLUMN_CLASSES nor LINE_CLASSES (e.g. a candlestick or custom series) will be classified as 'bar', attempted extraction via extractBarPoints, and silently produce an empty layer. This is safe but confusing during debugging. Returning 'unknown' (which the SeriesKind type already includes) and skipping it explicitly in the adapter switch would be clearer.

3. Series grouping is duplicated between adapter.ts and binder.tsx. fromXYChart builds layers in one pass; groupSeries in binder.tsx classifies the same series a second time to feed buildNavigationMap. If the two ever diverge (e.g. a new chart type is added to the adapter but not groupSeries), the navigation map will silently misalign. Refactoring to share one grouping pass would eliminate this class of bug.

4. buildNavigationMap relies on insertion order matching between layers and series groups. The counters histIdx and heatIdx assume multiple histogram/heatmap layers are encountered in the same order as histogramSeries / heatmapSeries. This is true today because both walk chart.series.values in order, but this invariant is implicit and fragile. A comment explaining the dependency (or better, a Map keyed on series uid) would make it safer.

5. Module-level mutable _counter in adapter.ts persists across multiple fromAmCharts calls on the same page. IDs like heatmap-1, line-2 are documented as ephemeral, but they will change between calls on a page reload that doesn't fully unmount (e.g. React hot reload). If anything caches these IDs (Redux state, a11y trees), unexpected stale-layer effects could occur. Consider resetting per-call or using a closure-scoped counter.

---

Code Quality

6. readChartTitle relies on undocumented amCharts internals. Walking chart.children.values and matching on className === 'Label' || className === 'Title' has no public API guarantee. The option docs already suggest providing title explicitly; consider removing this heuristic and just returning undefined, letting users supply titles through options.title. If the heuristic must stay, a code comment explaining why it's needed and its fragility would help.

7. color-mix CSS has no fallback. In overlay.ts:

const fill = `color-mix(in srgb, ${color} 22%, transparent)`;

color-mix() is available from Chrome 111 / Firefox 113 / Safari 16.2 (May 2023). Older browser support will silently produce no background fill on the highlight box. Worth checking whether MAIDR's supported browser range covers this, or whether a hex-with-alpha fallback is needed.

8. Naming inconsistency: AmChartsBinderOptions vs AmChartsBindOptions. AmChartsBinderOptions (in types.ts) is for fromAmCharts; AmChartsBindOptions (in binder.tsx) extends it for bindAmCharts. The one-letter difference (Binder vs Bind) is easy to confuse in documentation and type hover. Consider AmChartsAdapterOptions / AmChartsBindOptions or similar to make the distinction more obvious.

9. Long duplicated branch in bindXYChart. The highlight-enabled and highlight-disabled paths both call renderMaidr() and return an object with the same shape. The shared teardown logic can be extracted to reduce the function length and the two near-identical renderMaidr calls.

---

Performance

10. getHighlightColor() instantiates new LocalStorageService() on every navigation event. Reading from localStorage on every keypress is unlikely to be a bottleneck, but aligning with how other adapters manage this (stable reference rather than per-draw construction) would be cleaner.

11. syncToRoot() reads layout properties inside show() on every navigation. offsetLeft, offsetTop, clientWidth, clientHeight can trigger layout reflow. If the chart hasn't resized (the common case), they're wasted reads. Caching the last-known geometry and only invalidating via the existing ResizeObserver callback would avoid the reflow on every arrow-key press.

---

Test Coverage

12. No tests are included. The PR adds ~2,600 lines of new functionality. The pure extraction logic in extractor.ts (extractBarPoints, classifySeriesKind, extractHeatmapData, etc.) and layer builders in adapter.ts are straightforward to unit-test against mock amCharts objects — the duck-typed interface design makes mocks easy to construct. At minimum, these should be covered before merging.

---

Minor / Nits

• Example uses setTimeout(bindAll, 1500) — even with the caveat in comments, a 1.5-second hardcoded delay is fragile on slow machines and in CI. The datavalidated event pattern shown in the Quick Start is better and should be the primary approach in the example.
• window.__amBindings global in the example: a module-scoped array or a direct callback at chart creation time would be cleaner.
• PR description mentions src/binder/amcharts/ but the actual path is src/adapters/amcharts/. Minor doc inconsistency.
• _container parameter in buildCommonSelector and selectorForElement is unused. If reserved for future use, a comment helps; otherwise removing it cleans up the signature.
• scripts/build.js exclusion list for docs files is now a very long single-line string condition. A Set lookup or array .includes() would be more readable and less error-prone when adding future adapters.

---

Summary

The feature is well-designed and architecturally consistent with the rest of the adapter layer. The main risks to address before merging are:

1. The duplicated findXYChart and series-grouping logic (correctness risk if they diverge)
2. The classifySeriesKind default returning 'bar' silently for unknown series types
3. Missing tests for the extraction/adapter logic
4. The color-mix CSS compatibility gap

Everything else is low severity or polish-level. Great work overall on a complex integration.

[xabilitylab]

🎉 This PR is included in version 3.69.0 🎉

The release is available on GitHub release

Your semantic-release bot 📦🚀