feat: add realtime/streaming data support via setData API (Phase 1)

[jooyoungseo]

Implements Phase 1 of issue #536 by adding a setData API that allows
chart data to be replaced at runtime without unmounting the component.

Changes:

• Convert <Maidr> component to forwardRef and expose MaidrRef
  handle with setData() method via useImperativeHandle
• Support prop-driven data updates: changing the data prop while the
  chart is focused transparently rebuilds the Controller
• Add useMaidrController effect to detect data changes and recreate
  the Controller while preserving the Redux store
• Expose window.maidrPlots global registry for script-tag consumers
  to call setData() by plot ID
• Export MaidrRef type from maidr/react public API

https://claude.ai/code/session_011ooVhoME1PLoimCg71q9rW

[claude]

PR Review: feat: add realtime/streaming data support via setData API (Phase 1)

The overall design is sound — using forwardRef + useImperativeHandle is the idiomatic React approach for imperative handles, and keeping the Redux store alive while rebuilding the Controller is the right call. Below are issues worth addressing before merge, ordered by severity.

---

Bugs / Correctness

1. No screen-reader announcement when data updates mid-focus

This is the most important issue for an accessibility-focused library. When setData is called while the chart is focused, the Controller is silently rebuilt. The hasAnnouncedRef.current = false reset only takes effect on the next onFocusIn call — but focus never leaves, so showInitialInstructionInText is never called again. Screen reader users get no feedback that the data changed.

Consider firing a dedicated announcement (e.g., "Chart data updated") via TextService or NotificationService after recreating the Controller in the data-change effect.

2. prevDataRef guard in useMaidrController is redundant

// useMaidrController.ts
useEffect(() => {
  if (prevDataRef.current === data)  // always false when this effect runs
    return;
  prevDataRef.current = data;
  ...
}, [data, disposeController]);

React only runs a useEffect when a listed dependency changes by reference. Because data is in the dep array, prevDataRef.current === data will always be false when the effect fires. The prevDataRef ref is unnecessary — the dep array already provides this guard. Remove it to reduce cognitive overhead.

3. hasAnnouncedRef.current = false reset is redundant in the new effect

disposeController() (line 74 in the hook) already resets hasAnnouncedRef.current = false. The explicit reset on line 174 is dead code.

---

Performance

4. useState + useEffect for syncing dataProp causes an extra render

// maidr-component.tsx
const [data, setData] = useState<MaidrData>(dataProp);

useEffect(() => {
  setData(dataProp);
}, [dataProp]);

This is the derived state from props anti-pattern. When dataProp changes, the component renders once with the stale data state, then the effect queues a state update causing a second render. During the first render useMaidrController receives the old data, meaning there is a brief frame where the hook's data dep and the prop are out of sync.

React's recommendation is to update state during render when the prop changes:

const [data, setData] = useState<MaidrData>(dataProp);
const [prevDataProp, setPrevDataProp] = useState<MaidrData>(dataProp);

if (prevDataProp !== dataProp) {
  setPrevDataProp(dataProp);
  setData(dataProp);  // prop wins over any previous imperative setData call
}

This collapses two renders into one and eliminates the intermediate stale frame.

---

Design / Maintainability

5. maidrPlots registry has no cleanup mechanism

// index.tsx
maidrPlots.set(maidr.id, handle);  // never removed

For the script-tag entry point, plots are typically static so this is likely acceptable for Phase 1. But document this limitation so it is not forgotten for Phase 2. There is also no guard against duplicate IDs — calling initMaidr twice with the same maidr.id silently overwrites the existing entry.

6. window.maidrPlots assignment is at module top-level

window.maidrPlots = maidrPlots;  // runs on import

src/index.tsx is the script-tag entry point so this is mostly fine. However, if this module is ever imported in an SSR or Node.js test context it will throw (window is not defined). A typeof window !== 'undefined' guard would be defensive.

---

Test Coverage

7. No tests for the new behavior

The new code path (data-change effect, proxy handle, registry) is complex enough to warrant at least:

• A unit/integration test that calls setData while a controller is active and verifies the controller is recreated with the new data.
• A test that verifies maidrPlots.get(id) returns a handle that delegates to the React ref.

The existing E2E specs in e2e_tests/specs/ would be the natural place for an integration test covering the realtime update flow.

---

Minor / Nits

• createRef in initMaidr — correct for non-component code. A brief comment explaining why createRef rather than a plain object with a mutable .current would help future readers.
• useImperativeHandle deps [] — technically correct since the setData state setter is stable, but [setData] would be more explicit.
• Version bump to 3.51.0 — verify this follows the project's release workflow and should not happen at release time instead.

---

Summary

Category	Issue
A11y (blocking)	No SR announcement when data updates mid-focus
Correctness	Redundant prevDataRef guard and hasAnnouncedRef reset
Performance	Extra render from useState + useEffect anti-pattern
Design	No cleanup for maidrPlots registry; missing duplicate-ID guard
Tests	No coverage for new behavior

The setData API surface (MaidrRef, useImperativeHandle, proxy handle) is well-designed. The main blocking concern is the missing accessibility announcement on mid-focus data update — a silent data swap with no feedback is a significant gap for a library whose core mission is accessible data interaction.

[claude]

Code Review: feat: add realtime/streaming data support via setData API (Phase 1)

Overall, this is a well-structured implementation with clear separation of concerns. The forwardRef conversion, proxy handle pattern, and useMaidrController data-change effect are all solid. Below are my findings ordered by severity.

Must Fix

1. Memory leak in window.maidrPlots (src/index.tsx)

Entries are added to maidrPlots when plots initialise but are never removed when they unmount. Over time this leaks both the MaidrRef handle and any React ref closures it holds.

The initMaidr function should register a cleanup that calls maidrPlots.delete(maidr.id) on focusout/unmount. Or better: expose a destroy() method on MaidrRef and call it from the unmount path so the caller can also clean up.

2. No tests for new functionality

The PR adds ~2,700 lines of new production code but zero tests. Given this is an accessibility library where regressions have direct user impact, the following should be covered:

• Maidr component: setData ref updates the internal state
• Maidr component: changing the data prop is reflected in rendered output
• useMaidrController: data change triggers controller rebuild only when focused
• D3 binders: happy-path unit tests for resolveAccessor, generateSelector, and one chart-type binder

Should Fix

3. Unsafe cast in resolveAccessor (src/d3/util.ts)

The line return datum[accessor as keyof typeof datum] as T; is problematic because datum is typed as unknown, so this cast silently produces undefined at runtime when the key is absent. Downstream binders see undefined where they expect a number/string, causing silent data corruption. Prefer an explicit existence check and throw a clear error when the key is missing.

4. Equal-distribution assumption in bindLine.ts fallback

When pointSelector is not provided, the fallback uses Math.floor(index / linesCount) to assign a line. This only works when every line has exactly the same number of points. Real-world D3 multi-line charts (missing data, sparse series) will silently misassign points. At minimum, emit a warning; ideally, require the caller to pass a pointSelector.

5. generateSelector may not produce a unique selector (src/d3/util.ts)

The function walks the DOM using tag names and :nth-child indices. In D3 grouped bar charts with many structural siblings, the generated path can match multiple elements. Consider always anchoring the path with the container's id.

Suggestions / Consider

6. Derived state double-render in maidr-component.tsx

The pattern useState(dataProp) + useEffect(() => setData(dataProp), [dataProp]) causes two renders on every prop change (once with stale state, then after the effect). For high-frequency streaming this adds up. The React-recommended alternative is to not store a duplicate of the prop in state at all - keep only the imperative override in state, and compute const data = imperativeOverride ?? dataProp during render.

7. Rapid data updates in useMaidrController.ts

If setData is called in quick succession (e.g., every 100 ms), each call disposes and recreates the Controller synchronously with no debounce. Consider documenting that callers are responsible for throttling, or add an optional debounce parameter.

8. Document data prop vs setData precedence

The JSDoc should state explicitly that a prop change clears any imperative override (prop wins). This is the current behaviour but is not documented.

9. generateId collision risk (src/d3/util.ts)

Date.now() has millisecond precision; two charts initialising in the same millisecond are only separated by Math.random(). crypto.randomUUID() is available in all target environments and is a zero-cost improvement.

What is Done Well

• Proxy handle pattern in src/index.tsx correctly solves the async React commit timing issue.
• structuredClone(data) in createController prevents external mutations from corrupting the model.
• Reference equality (===) for data change detection avoids deep-equal overhead and prevents spurious controller rebuilds.
• emptyOutDir: false in vite.d3.config.ts correctly preserves the existing bundles.
• JSDoc across MaidrRef and all D3 binder configs is thorough and includes examples for all three consumer patterns (script tag, ESM, React).
• D3 binders cover all existing MAIDR chart types, maintaining consistent accessibility support across the board.

---

Summary: The core React integration (forwardRef, useImperativeHandle, useMaidrController data-change effect) is sound. Priority items before merging: memory leak in the global registry (#1), adding tests (#2), and fixing the unsafe resolveAccessor cast (#3). The remaining points are lower-priority improvements.

[claude]

PR Review: feat: add realtime/streaming data support via setData API (Phase 1)

This is a well-structured PR that adds two independent capabilities: (1) an imperative setData API for real-time chart updates and (2) a new D3.js binder package. The implementation is generally clean and thoughtful. Here are my findings:

---

Architecture & Design

Strengths:

• The forwardRef + useImperativeHandle pattern is the correct React idiom for imperative handles
• Keeping the D3 binder as a separate build entry (src/d3/) with its own Vite config is a clean separation of concerns
• The proxy handle in initMaidr() that delegates to maidrRef.current is a good way to handle the timing gap before React commits
• The D3 binder's data accessor pattern (DataAccessor<T> = string | ((datum, index) => T)) is flexible and idiomatic

---

Bugs / Issues

1. Memory leak in maidrPlots registry (src/index.tsx)

Plots are registered in maidrPlots but never removed. In long-lived SPAs where plots are dynamically mounted/unmounted, this will accumulate entries indefinitely. A cleanup callback (e.g., hooking into the React root's unmount or the focusout/dispose lifecycle) is needed.

No removal of the entry when the plot is destroyed -- maidrPlots.delete(maidr.id) is never called.

2. Selector bug in multi-line path-based extraction (src/d3/bindLine.ts, bindSmooth.ts)

In the path-based extraction branch (no pointSelector), the selectors.push(...) call is outside the loop, so only one selector is added regardless of how many lines exist. This means all lines share one selector, which will break per-line highlighting in MAIDR.

// bindLine.ts - selectors.push is called once, outside the for-loop
for (const { datum } of lineElements) {
  if (lineData.length > 0) {
    data.push(lineData);
    // Missing: selectors.push(scopeSelector(svg, selector)) here inside the loop
  }
}
selectors.push(scopeSelector(svg, selector)); // only one entry, but data may have N entries

The same issue exists in bindSmooth.ts at the path-based branch.

3. useImperativeHandle missing dependency (src/maidr-component.tsx)

The useImperativeHandle hook has an empty dependency array [] but closes over setData from useState. While the state setter is stable, the react-hooks/exhaustive-deps lint rule will warn on this. Recommend [setData].

4. No debouncing for rapid data updates

The useEffect in useMaidrController.ts that recreates the Controller on data change has no rate-limiting. For a streaming/realtime use case (the stated goal of this PR), a caller updating data at high frequency (even 10 fps) will cause continuous Controller teardown and recreation, each of which creates a full Figure/Context/Service graph. This would severely degrade performance and produce audio glitches.

Even a simple leading-edge debounce (e.g., 16ms) or a requestAnimationFrame guard would help significantly.

---

Code Quality

5. resolveAccessor unsafe cast (src/d3/util.ts)

const record = datum as Record<string, unknown>;
return record[accessor] as T | undefined;

If datum is a primitive (number, string, boolean) -- which can happen if D3 binds raw values -- this cast is technically unsafe. A guard like if (datum == null || typeof datum !== 'object') return undefined; would prevent confusing runtime behavior.

6. Module-level window access (src/index.tsx)

window.maidrPlots = maidrPlots; runs at module load time. This will throw ReferenceError: window is not defined in SSR environments (Next.js, Gatsby). Since the file already touches document.readyState at module level, this is consistent with the existing pattern -- but a typeof window !== 'undefined' guard is worth adding for hybrid environments.

7. Fallback line partitioning assumes equal point counts (src/d3/bindLine.ts)

const pointsPerLine = Math.floor(allPoints.length / lineElements.length);

This fallback silently produces incorrect data if lines have different numbers of points (very common with real datasets). A console.warn here -- or rejecting this case entirely -- would be safer than silent data corruption.

---

Missing Tests

This PR adds 2,807 lines of new logic with zero new tests. Key areas that need coverage:

• Unit tests for each bindD3* function (happy path + missing __data__ error path)
• Unit tests for resolveAccessor with string and function accessors
• Unit tests for generateId, scopeSelector, buildAxes
• Integration/hook test for the data-change effect in useMaidrController
• A test verifying maidrPlots registry is populated after initMaidr

---

Minor Observations

• The useEffect(() => setData(dataProp), [dataProp]) in maidr-component.tsx fires once on mount setting the same initial value again. Harmless due to React's bailout, but redundant.
• idCounter is module-level state in util.ts and won't reset between Jest test runs without module re-import.
• The D3 binder docs and examples are clear and well-written -- great developer experience.

---

Summary

Category	Status
Architecture compliance	✅ Good
Core setData API design	✅ Good
D3 binder design	✅ Good
Multi-line selector bug	❌ Needs fix
Memory leak in registry	❌ Needs fix
No debouncing for streaming	⚠️ Should address before production use
Test coverage	❌ Missing
SSR guard	⚠️ Minor

The core idea is sound and the API surface is well-designed. I'd recommend addressing the selector bug and the missing tests before merging, and ideally adding a debouncing strategy given that streaming is the primary motivation for this PR.