feat: add Victory charting library support

[jooyoungseo]

Add maidr/victory entry point with <MaidrVictory> React component
that wraps Victory chart components and automatically provides accessible
non-visual access through audio sonification, text descriptions, braille
output, and keyboard navigation.

Supports VictoryBar, VictoryLine, VictoryScatter, VictoryArea, and
VictoryPie chart types with automatic data extraction from component
props and SVG element tagging for highlight integration.

Closes #543

https://claude.ai/code/session_01EWLKfgUPQxmnWhB78rrkYg

[claude]

Code Review: feat: add Victory charting library support

Good addition of Victory charting library support. The architecture is sensible — a separate entry point with an adapter layer, keeping Victory concerns isolated from the core. Here are the issues I found, roughly ordered by severity.

---

Bugs

1. toMaidrLayer has an implicit undefined return path

adapter.ts:152 — The function declares return type MaidrLayer but the switch statement has no default case and no return after the block. TypeScript exhaustiveness checking may not catch this if strict mode is not fully configured, and at runtime if the union is somehow violated the function silently returns undefined, which will cause downstream null-reference errors in the model layer.

Add a default case with throw new Error(...) or a never-assertion after the switch to make TypeScript flag exhaustiveness failures at compile time.

---

2. VictoryPie mapped to TraceType.BAR

adapter.ts:159–167 — Pie charts are not bar charts. MAIDR's BarTrace generates descriptions like "Bar 1 of 4, Q1: 120". Mapping pie slices here means a 4-slice pie chart announces as "Bar 1 of 4", which is semantically wrong and defeats the accessibility goal.

If there is no TraceType.PIE yet, this should at minimum be documented clearly (in the JSDoc or the index.ts exports) and ideally tracked as a follow-up issue rather than silently mislabeling the data.

---

Type Safety

3. VictoryLayerInfo.data typed as unknown

types.ts:39 — data: unknown is then cast in three places as BarPoint[], LinePoint[], ScatterPoint[] without runtime validation. This nullifies TypeScript's safety guarantees; if the Victory accessor returns unexpected shapes, the error surfaces deep inside the model layer with an unhelpful message.

The actual shape produced by extractLayerFromElement is Array<{ x: unknown; y: unknown }>. Typing data as that would catch shape mismatches at call sites.

---

Fragility / Correctness Concerns

4. DOM selector strategy relies on Victory internal structure

selectors.ts — The entire highlighting strategy depends on Victory rendering elements with role="presentation", specific SVG tag names per chart type, and a particular DOM ordering. These are undocumented implementation details that Victory can change in a patch release. At minimum, add a comment pinning the Victory major version tested against (e.g., ^37) and listing what to re-verify on upgrades.

5. Line/area path heuristic is fragile

selectors.ts:131–139 — The heuristic counts SVG path commands to identify the line path. This can mis-match on clip paths, defs, or mask paths that Victory adds to the SVG. Victory's default cubic bezier interpolation also means each data segment uses multiple C commands, affecting the count. The fallback to candidates[0] (line 143) then silently uses an arbitrary path. At minimum, log a warning when the fallback is triggered so developers can detect mis-matches during integration.

6. VictoryArea may match the wrong path

selectors.ts:114 — Victory's area chart renders two paths: the filled area and the line on top. If the fill path has more SVG commands than the line (common since it closes the shape), the heuristic may claim the fill path rather than the line, causing incorrect highlighting. This should be verified against actual Victory SVG output.

7. Children introspection breaks with HOCs and wrappers

adapter.ts:13–25 — Component detection works via displayName. If a user wraps a Victory component (e.g., with React.memo, React.forwardRef, or their own HOC), the check fails silently and the layer is skipped with no error. This should be documented as unsupported.

---

Performance

8. children as useLayoutEffect dependency triggers on every parent re-render

MaidrVictory.tsx:82 — children is a new object reference on every render even if chart data is unchanged. This causes the DOM tagging logic (which calls querySelectorAll and setAttribute on potentially many elements) to run on every parent re-render. Consider extracting the Victory layer data from children and memoizing it as the dep rather than using the raw children reference.

---

Unsupported Patterns (Documentation Gap)

9. VictoryGroup, VictoryStack, nested compositions not handled

adapter.ts:112–143 — The extraction only handles one level of VictoryChart children. Common compositions like VictoryGroup (grouped bars) and VictoryStack (stacked charts) are silently ignored — extractVictoryLayers returns an empty array and MAIDR initializes with no data, with no warning to the user. index.ts should document supported vs. unsupported compositions.

---

Tests

10. No tests included

There are no tests for the new Victory adapter. Given the adapter relies on React children introspection, DOM manipulation, and multi-step heuristics, it is both the most testable and most breakage-prone part of this PR. Unit tests for extractVictoryLayers and toMaidrLayer are straightforward (no DOM required), and a jsdom-based test for tagLayerElements would catch selector regressions.

---

Minor

• adapter.ts:134: trailing space inside { } is inconsistent with surrounding style.

---

Overall this is a useful feature addition with clean separation of concerns. The main actionable items before merge are the implicit undefined return bug (#1), the pie/bar semantic mismatch (#2), and at least minimal tests for the adapter logic (#10).

[claude]

PR Review: feat: add Victory charting library support

Great feature addition! The architecture is well thought-out — using a separate entry point, an adapter layer, and graceful degradation when selectors can't be found are all good patterns. Here are some issues worth addressing before merge.

---

Bugs

1. validateRawData includes null (adapter.ts:326)

typeof null === 'object' in JavaScript, so the current check passes for arrays containing null values:

function validateRawData(rawData: unknown): rawData is Record<string, unknown>[] {
  return Array.isArray(rawData) && rawData.length > 0 && typeof rawData[0] === 'object';
  //                                                      ^^^^ null passes this check
}

The guard should be:

typeof rawData[0] === 'object' && rawData[0] \!== null

2. Tagged DOM attributes accumulate on re-renders (selectors.ts:86-88)

When useLayoutEffect runs again (Victory data changes), new data-maidr-victory-N attributes are added to elements but old ones are never removed. After two renders a bar element could carry both data-maidr-victory-0 and a new stale attribute. The claimed set is recreated fresh each run but doesn't clean up the previous run's attributes.

3. Empty chart loses title/subtitle/caption (MaidrVictory.tsx:170)

if (\!container || victoryLayers.length === 0) return;  // early return

If Victory renders a chart with no supported layer types, the maidrData state stays as emptyMaidrData(id) which contains none of the title, subtitle, or caption props. Consider updating the schema even when layers are empty.

4. Stack overflow risk with large histograms (adapter.ts:473-474)

const min = Math.min(...values);
const max = Math.max(...values);

Math.min/max with spread will throw a RangeError: Maximum call stack size exceeded for datasets with ~100k+ points. Prefer values.reduce((a, b) => Math.min(a, b)) or Array.prototype.reduce.

---

Design/Robustness Concerns

5. Fragile DOM selector strategy (selectors.ts)

The entire highlighting mechanism relies on Victory always rendering data elements with role="presentation". This is an undocumented implementation detail of Victory's internals. A Victory upgrade or theme customization could silently break all highlighting without any console error. Consider documenting the Victory version range this has been tested against, and adding a console warning when matched.length \!== layer.dataCount (selector degradation is currently silent).

6. Fragile line/area path heuristic (selectors.ts:115-123)

The path selection logic counts SVG path commands and compares to dataCount. This will fail for:

• Smooth (Bezier) curves where each data point generates multiple commands (C, Q)
• Area charts where the d attribute includes both a "fill" path and a "stroke" path

The fallback exists but means the wrong path element gets tagged silently.

7. VictoryPie → BAR semantic mismatch (adapter.ts:521-523, index.ts comment)

Mapping VictoryPie to TraceType.BAR is documented in the JSDoc as "bar chart (categorical data)" but a screen reader user navigating a "pie chart" and hearing bar-chart-style descriptions ("Bar 1 of 5") will be confused. Consider either:

• Using a dedicated TraceType.PIE if it exists, or
• Making the text description explicitly say "Slice 1 of 5" instead

8. children as useMemo dependency is unstable (MaidrVictory.tsx:163)

const victoryLayers = useMemo(() => extractVictoryLayers(children), [children]);

React creates a new children reference on every parent render, so this useMemo will recompute on every parent re-render regardless of whether Victory data actually changed. This makes useLayoutEffect re-run and re-tag the DOM repeatedly. Consider whether there's a more stable dependency to use, or document that this is intentional.

---

Missing Coverage

9. No tests

None of the new modules have test coverage. The following are particularly high-value to test since they're pure functions:

• extractVictoryLayers — data extraction for each chart type
• toMaidrLayer — schema conversion for each trace type
• validateRawData — edge cases (null, empty array, non-object items)
• extractHistogramData — bin calculation edge cases (all-same values, single value)

---

Minor

10. VictoryPie selector uses path tag (selectors.ts:56-58)

Pie slices in Victory are rendered as path elements, so this is correct — but it's not obvious why VictoryPie is grouped with scatter (also path) rather than VictoryBar (rect). A brief comment would help.

11. legend only captures first multi-series layer (MaidrVictory.tsx:188)

legend: victoryLayers.find(l => l.legend)?.legend,

If a chart has multiple VictoryGroup/VictoryStack layers, only the first legend is used. Likely a real-world edge case, but worth noting.

---

Overall this is a solid implementation with good separation of concerns. The main blockers are the null validation bug (#1) and the attribute accumulation issue (#2). The fragile selector strategy (#5, #6) is an inherent challenge with this approach and is acceptable if documented and tested.

[claude]

PR Review: feat: add Victory charting library support

Great addition — making MAIDR work with Victory enables a much wider audience of React developers to build accessible charts. The architecture (separate entry point, optional peer dep, fingerprint-based skip) is well-thought-out. Issues found are ordered by severity.

---

Bug: Metadata props silently fail to update

File: src/victory/MaidrVictory.tsx, layerFingerprint + useLayoutEffect

The fingerprint only hashes layer data (victoryType, data, dataCount), not id, title, subtitle, or caption. If a parent re-renders with a changed title but the same chart data, the early-return fires and setMaidrData is never called, so the stale title persists indefinitely.

Fix: Include metadata in the fingerprint string, or add a separate useEffect([id, title, subtitle, caption]) that calls setMaidrData when only metadata changes.

---

clearTaggedElements silently breaks at 10+ layers

File: src/victory/selectors.ts, line ~916

The selector query is hardcoded to attributes 0–9. A chart with 10+ layers will leave ghost data-maidr-victory-10 (and above) attributes on re-render, breaking selector uniqueness for those layers.

Fix: Walk all elements and filter by attribute prefix rather than enumerating indices:

for (const el of svg.querySelectorAll('*')) {
  for (const attr of Array.from(el.attributes)) {
    if (attr.name.startsWith(ATTR_PREFIX)) el.removeAttribute(attr.name);
  }
}

---

Histogram bin count can mismatch Victory's rendered bar count

File: src/victory/adapter.ts, extractHistogramData (~line 540)

When bins is an array of thresholds (a valid Victory prop), the adapter falls back to the sqrt formula, computing a different bin count than Victory renders. tagDiscreteElements uses dataCount to slice DOM elements by count, so it can tag the wrong <rect> elements or silently return undefined.

Fix:

const binCount = typeof props.bins === 'number'
  ? props.bins
  : Array.isArray(props.bins)
    ? (props.bins as number[]).length - 1   // N thresholds = N-1 bins
    : Math.ceil(Math.sqrt(values.length));

---

process.env: {} stub can break runtime env checks

File: vite.victory.config.ts, define block

Replacing process.env with {} makes process.env.NODE_ENV evaluate to undefined, which can trigger development-mode code paths or guard failures in React internals. The existing vite.react.config.ts does not use this override. Remove it, or scope it to:

define: {
  'process.env.NODE_ENV': JSON.stringify('production'),
},

---

HOC traversal in getVictoryDisplayName has no cycle guard

File: src/victory/adapter.ts, ~line 286

A circular HOC reference (obj.type === obj) would cause infinite recursion. Adding a visited set prevents this:

function getVictoryDisplayName(type: unknown, visited = new Set<unknown>()): string | null {
  if (!type || visited.has(type)) return null;
  visited.add(type);
  // pass `visited` through all recursive calls
}

---

Minor: unsafe type casts without runtime validation

File: src/victory/adapter.ts

Accessor results are cast directly without narrowing (getX(d) as string | number, lowerOutliers as number[]). Custom Victory accessors returning Date or null would silently produce broken data. A narrow-before-cast helper would be safer.

---

Minor: intentional useLayoutEffect dep array omission should be explicit

File: src/victory/MaidrVictory.tsx, line ~243

The missing dependency array looks like an accidental omission and will trigger lint warnings. The existing comment explains the reasoning, but a brief note at the closing brace makes it explicit for reviewers:

  // No dep array intentional: children is always a new reference per render;
  // the fingerprint check inside handles de-duplication.
});

---

Minor: no debug logging when selector tagging fails

File: src/victory/selectors.ts, tagDiscreteElements

Per the project's debugging guide, a console.warn when element count mismatches would help developers diagnose highlighting failures without guessing.

---

Test coverage

No tests are included for the new module. Unit tests for the pure functions (extractVictoryLayers, toMaidrLayer, layerFingerprint) would prevent regressions when Victory's rendering changes. Not a merge blocker, but worth a follow-up issue.

---

Summary

Severity	Issue
Critical	Metadata props (id, title, subtitle, caption) silently fail to update when chart data is unchanged
Critical	clearTaggedElements hardcoded to 10 layers; silent failure for larger charts
Major	Histogram bin count mismatch when bins is an array
Major	process.env: {} stub can cause runtime issues
Major	HOC traversal has no cycle guard (potential infinite recursion)
Minor	Unsafe type casts; missing dep-array comment; missing warn log for failed selectors

The overall design is solid and the graceful degradation story for highlighting is correct. The two critical bugs should be fixed before merge.