feat: add realtime/streaming data support

[jooyoungseo]

Summary

Implements realtime/streaming data support as proposed in #536, covering all three suggested phases plus user documentation.

Closes #536

What's included

Phase 1 — setData (full data replacement)

• Script tag: window.maidrLive.setData(maidrJson)
• npm/React: setMaidrData() from maidr/react, or simply pass a new data prop on charts configured with live: true
• Core mechanism: Context.replaceFigure() swaps the Figure/Subplot/Trace model in place. The old figure is disposed first (removing stale SVG highlight clones before the new model re-queries selectors), the navigation stack is rebuilt at the same depth, and the user's position is silently restored with clamping (including across layer-type changes, which reset navigation via the shape signature). Services, view models, and keybindings are untouched, so updates are cheap enough for streaming.

Phase 2 — appendData (streaming)

• window.maidrLive.appendData(point, { id?, layerId?, layerIndex?, groupIndex?, subplotRow?, subplotCol? }) / appendMaidrData()
• New LiveDataManager registry routes updates to mounted chart instances and merges points immutably — flat layers (bar, scatter, histogram, candlestick, box) and nested group layers (line, smooth, segmented) are both supported, including initially empty layers and creating a new series at groupIndex === count; heatmaps are rejected with a warning
• New Maidr.maxWidth option implements the sliding window: oldest points are trimmed on append, and the cursor follows the same data point as the window slides (section-aware for candlesticks)
• Axis ranges, navigation graphs, braille, and text recompute naturally since the model is rebuilt from the merged data
• Multi-layer charts (e.g. py-maidr's candlestick + volume + moving-average) stream per layer via layerId, preserving the user's active layer and x-position across ticks

Phase 3 — Monitor mode

• New Maidr.live: true opt-in flag
• Pressing M (TRACE, BRAILLE, and SUBPLOT scopes) toggles MonitorService, announcing "Monitoring on/off" (or explaining monitoring is only for live charts)
• While on, each appended point is auto-sonified (AudioService) and announced (TextService) without moving the user's cursor, mirroring chart2music's behavior — via AbstractTrace.getStateAt(row, col), which computes state at a position without notifying observers (now structurally enforced: notifyStateUpdate throws during computation). Candlesticks announce the new candle's close price (with trend), overriding getStateAt to map their internal section/point state
• Wired through the standard Command pattern (ToggleMonitorCommand, CommandContext, keymaps)

AI chat freshness

ChatService.updateData() stores the latest data and lazily re-serializes on the first LLM request after a change (cached via getDataJson()), so AI answers reflect the data currently on screen with zero serialization cost on streaming appends.

Documentation & examples

• New user manual: docs/LIVE_DATA.md (Live & Streaming Data) with full API reference, script-tag and React examples, a candlestick/stock-ticker section, a real-data-feed integration guide, monitor mode, sliding window, performance guidance, and behavior details; registered in the docs site build
• Cross-links and sections added to README.md, docs/SCHEMA.md (live/maxWidth properties), docs/CONTROLS.md (Monitor Mode key), docs/react.md (Live & Streaming Data section), and docs/llms.txt
• Runnable streaming demos: examples/live-line.html (simulated sensor stream), examples/live-candlestick.html (simulated multi-layer stock ticker: candlestick + volume + moving average), and examples/live-coinbase.html (real live market data — BTC-USD trades from Coinbase's keyless public WebSocket feed, aggregated into 10-second candles; requires network)

Design rationale

Rather than making all 15 trace classes mutable with cache invalidation (the high-risk path flagged in the issue's barrier table), updates rebuild the model layer through the existing, tested constructors and restore navigation state at the Context level. This works uniformly for every chart type, reuses the Observer wiring as suggested in the issue, and leaves static charts completely unaffected (live features are opt-in).

Testing

• TDD: 60+ new unit tests (written red-first) — test/service/liveData.test.ts (data merging incl. candlestick/empty-layer/new-series edges, sliding window, clone semantics, manager registry), test/service/monitor.test.ts, test/model/contextReplaceFigure.test.ts (position preservation/clamping/shape & type changes/multi-layer mixed types/figure-level depth-1/observer cleanup), test/model/traceStateAt.test.ts + test/model/candlestickStateAt.test.ts (cursor-free state computation incl. throw-path restore and the structural no-notify guard), test/service/chat.test.ts (lazy serialization), test/command/toggleMonitor.test.ts
• Full Jest suite: 262 passing; the 3 failures in display.test.ts/braille-escape.test.ts pre-exist on main
• End-to-end (Playwright, real browser, production bundle): 20/20 checks — API exposure, navigation, append + reachability, monitor on/off announcements, auto-announcement of appended points, cursor preservation across appends, setData with position clamping, the maxWidth sliding window, new-series creation, and the full multi-layer candlestick ticker flow (layer switching with x-sync, per-layer appends, monitor announcing the new candle's close price). The Coinbase real-data demo was additionally verified live in a browser against the actual feed.
• tsc --noEmit, repo-wide ESLint, full 12-bundle production build, and the docs site build all pass

https://claude.ai/code/session_01KuiTyhDAEWtQX9oTKGatzN

[claude]

Code Review — feat: add realtime/streaming data support

This is a well-scoped, ambitious feature. The opt-in live: true design is exactly right — static charts are completely unaffected, and the rebuild-from-existing-constructors strategy avoids making all 15 trace classes mutable. The test coverage (36 new unit tests + 11 Playwright checks) is thorough. Below are issues from high-severity to low.

---

High Severity

1. Type safety bug: AppendedPointInfo.layerId typed as string but layer.id is optional

src/service/liveData.ts — appendPointToMaidr:

appended: {
  layerId: layer.id,  // layer.id?: string | undefined, but layerId: string
  ...
}

MaidrLayer.id is optional. When a layer has no id, layerId will be undefined at runtime even though the type says string. Downstream code in resolveActiveColShift and potentially user-facing callbacks will receive an incorrect value. Fix: type layerId as string | undefined, or guard with layer.id ?? ''.

---

2. announceAppendedPoint temporarily mutates trace cursor state

src/controller.ts:

trace.isInitialEntry = false;
trace.row = appended.row;
trace.col = appended.col;
const state = trace.state;          // ← could trigger side effects
trace.row = previous.row;
trace.col = previous.col;
trace.isInitialEntry = previous.isInitialEntry;

This is a pattern to compute the state at an arbitrary position without moving the user. The try/catch means the restore still runs on throw, but any side effects triggered by trace.state (e.g., getters with lazy computation or observer notification) could produce unexpected behavior. If trace.state triggers notifyStateUpdate() internally, observers would fire before the restore.

Preferred approach: add a getStateAt(row: number, col: number): TraceState method on AbstractTrace that computes state from parameters without touching this.row/this.col. This keeps the temporary-mutation hack contained to the one method that needs it, and makes announceAppendedPoint safer.

---

Medium Severity

3. resolveActiveColShift — trace-layer index mapping assumption

src/controller.ts:

const onAppendedTrace = onAppendedSubplot && activeSubplot.row === appended.layerIndex;

activeSubplot.row is the subplot's active trace row (i.e., which trace in subplot.traces[row]). appended.layerIndex is the index into subplot.layers. For single-layer subplots these are both 0, but for multi-layer subplots they may diverge. Consider adding a comment explaining this assumption, or verifying the mapping is always 1:1.

---

4. maxWidth does not apply via the declarative React prop path

src/state/hook/useMaidrController.ts:

if (data.live) {
  liveDataManager.setData(data);  // calls setData, not appendData
}

setData passes data verbatim — the maxWidth sliding window is applied only by appendPointToMaidr (called from appendData). The docs/react.md example accumulates data indefinitely:

data: [[
  ...(prev.subplots[0][0].layers[0].data as { x: number; y: number }[][])[0],
  { x: Date.now(), y: 30 + Math.random() * 40 },
]],

This will grow without bound. Either apply the sliding window inside setData as well, or document explicitly that maxWidth only applies to appendData, and show the user how to trim the array themselves in the React prop example.

---

5. latestDataRef/previousDataRef interaction is subtle

src/state/hook/useMaidrController.ts has two effects that both write latestDataRef.current:

• The data.id effect: sets it on id change
• The data effect: sets it for non-live charts

For live charts, the data change effect routes through liveDataManager.setData(data), which eventually sets latestDataRef.current = event.maidr in the listener. There's a brief window between liveDataManager.setData(data) firing synchronously and the listener updating latestDataRef.current where createController (if called) might see stale data. This is unlikely to matter in practice (focus-in is user-driven), but a comment explaining the data ownership chain would reduce future confusion here.

---

Low Severity / Code Quality

6. figure and other fields losing readonly

// controller.ts (was readonly)
private figure: Figure;

// context.ts (was readonly)
private figure: Figure;

// highContrast.ts (was readonly)
private figure: Figure;

This is architecturally necessary for live updates, but the PR silently drops the readonly modifier on fields that were intentionally immutable after construction. Worth a comment (e.g., // mutable: updated on live data refresh) to signal that this is intentional.

7. liveDataManager module singleton — test isolation

export const liveDataManager = new LiveDataManager();

The unit tests correctly create fresh LiveDataManager instances. However, the singleton is imported and used in useMaidrController.ts and src/index.tsx — any integration or E2E test that renders a chart will register into the global instance. This is fine for the current test structure, but worth noting if tests are added that import the hook directly.

8. Minor: resolveActiveColShift silently swallows all errors

try {
  // ...
} catch {
  return 0;  // no logging
}

When figuring out if a trace is the active one fails (e.g., unexpected model state), this silently returns 0 (no shift). This is safe behavior, but a console.warn in the catch block would help diagnose issues in development.

---

Positive Highlights

• The appendPointToMaidr function is correctly immutable — input is never mutated, and the sharing of untouched subtrees is an intentional and correct optimization.
• The LiveDataManager.register disposable correctly guards against stale registrations: if (this.instances.get(id)?.listener === listener).
• Disposing the old figure before creating the new one (this.figure.dispose() then createFigure()) so stale highlight clones are removed before the new model queries selectors — this is a subtle but important correctness detail.
• MonitorService is clean and correctly decoupled through Observer<PlotState> rather than holding direct service references.
• cloneMaidrData correctly handles the non-cloneable onNavigate function reference.

---

Summary

The two must-fix issues before merge are the layerId type safety bug (#1) and the announceAppendedPoint mutation pattern (#2). The maxWidth/prop-path inconsistency (#4) should at minimum be documented clearly if not fixed. The rest are suggestions.

[Copilot]

Pull request overview

Adds opt-in live/realtime chart updates to MAIDR, enabling full data replacement and streaming point appends while preserving navigation state and supporting “monitor mode” auto-announcements.

Changes:

• Introduces a live data registry + APIs (window.maidrLive, setMaidrData, appendMaidrData) and a model-swap mechanism (Context.replaceFigure + Controller.updateData) to rebuild the model in place.
• Adds Monitor Mode (key M) via a new MonitorService and ToggleMonitorCommand, plus keymap/docs updates.
• Updates ChatService to refresh LLM chart context on live updates, and adds docs, examples, and Jest tests for the new behavior.

Reviewed changes

Copilot reviewed 28 out of 28 changed files in this pull request and generated 4 comments.

Show a summary per file

File	Description
test/service/monitor.test.ts	Adds unit tests for MonitorService toggling and appended-point handling.
test/service/liveData.test.ts	Adds unit tests for append/set behavior, sliding window, and LiveDataManager registry.
test/service/chat.test.ts	Adds unit test ensuring chat model JSON refreshes on update.
test/model/contextReplaceFigure.test.ts	Adds unit tests for navigation-state preservation/clamping during figure replacement.
src/type/grammar.ts	Extends MAIDR schema with live?: boolean and maxWidth?: number.
src/state/hook/useMaidrController.ts	Registers charts with LiveDataManager; wires React prop changes into live updates.
src/service/monitor.ts	Implements MonitorService (toggle + auto announce/sonify for appended points).
src/service/liveData.ts	Implements LiveDataManager, cloning helper, and append merge logic + sliding window.
src/service/keybinding.ts	Adds M binding (TOGGLE_MONITOR) for TRACE/BRAILLE scopes.
src/service/highContrast.ts	Makes figure replaceable and reapplies high-contrast after live updates.
src/service/formatter.ts	Adds formatter refresh hook after live data replacement.
src/service/chat.ts	Adds ChatService.updateData() and LLM model data refresh plumbing.
src/react-entry.ts	Re-exports live data helpers/types for React consumers.
src/model/plot.ts	Ensures subplot highlight clones are removed on dispose (avoids DOM accumulation on live updates).
src/model/context.ts	Adds replaceFigure() with navigation snapshot/restore and optional sliding-window shift.
src/index.tsx	Exposes window.maidrLive script-tag API for set/append live updates.
src/controller.ts	Adds updateData() model swap + observer rewire; wires monitor mode + appended-point announcement.
src/command/toggle.ts	Adds ToggleMonitorCommand.
src/command/factory.ts	Wires TOGGLE_MONITOR to ToggleMonitorCommand.
src/command/command.ts	Adds monitorService to CommandContext.
scripts/build-site.js	Registers “Live & Streaming Data” page in docs site build.
README.md	Adds Live & Streaming Data section and TOC entry.
examples/live-line.html	Adds runnable streaming demo using window.maidrLive.appendData.
docs/SCHEMA.md	Documents live/maxWidth schema properties and links to live guide.
docs/react.md	Documents React live updates via prop replacement and imperative helpers.
docs/llms.txt	Adds link to live/streaming documentation.
docs/LIVE_DATA.md	Adds full user guide for setData/appendData/monitor mode/sliding window.
docs/CONTROLS.md	Documents Monitor Mode keybinding and behavior.

[claude]

Code Review — PR #620: feat: add realtime/streaming data support

Overview

This is a substantial, well-conceived feature (2 171 additions, 32 deletions, 29 files) that adds realtime/streaming data support across three tiers: full setData replacement, incremental appendData streaming, and a monitor mode ('M' key) that auto-sonifies and announces new points without disrupting the user's position. The core design decision — rebuilding the model layer through existing constructors rather than making all 15 trace classes mutable — is the right call.

The test coverage is solid: 36 new unit tests written red-first, 11 Playwright E2E checks, and the full Jest suite still passing.

---

Issues

Bug / Correctness

1. Orphaned JSDoc comment in liveData.ts

Lines 1437–1439 contain a JSDoc comment (/** A single data point that can be appended to a live chart layer. */) that is immediately followed by a second JSDoc comment for NESTED_DATA_TYPES. The first comment is never attached to a declaration; it will silently appear on NESTED_DATA_TYPES in generated docs instead of on LiveDataPoint.

Fix: attach the first comment to LiveDataPoint, which is what it describes.

2. Temporary mutation in announceAppendedPoint is unsafe under an exception

// src/controller.ts – announceAppendedPoint
trace.isInitialEntry = false;
trace.row = appended.row;
trace.col = appended.col;
const state = trace.state;          // ← if this throws…
trace.row = previous.row;           // …these three lines never run
trace.col = previous.col;
trace.isInitialEntry = previous.isInitialEntry;

The try/catch around the whole block only logs the error; the trace remains in a mutated state if trace.state throws (unlikely but possible if a trace type has a defensive accessor). The restore should happen in a finally block:

try {
  trace.isInitialEntry = false;
  trace.row = appended.row;
  trace.col = appended.col;
  const state = trace.state;
  this.monitorService.handleNewPoint(state);
} catch (error) {
  console.warn('[maidr] Failed to announce appended data point:', error);
} finally {
  trace.row = previous.row;
  trace.col = previous.col;
  trace.isInitialEntry = previous.isInitialEntry;
}

---

Architecture / Design

3. M key now fires on all static charts

TOGGLE_MONITOR is registered in both TRACE_KEYMAP and BRAILLE_KEYMAP unconditionally, so pressing M on any static chart now announces "Monitoring is available only for live charts". Previously the key was unbound and silently ignored. Screen reader users navigating a static chart may hear an unexpected announcement every time they accidentally hit M.

Consider either: (a) skipping the keybinding registration when live !== true, or (b) using a terse, less disruptive message (e.g. "Monitor mode: live charts only").

4. replaceFigure overload accepts both Figure and () => Figure

public replaceFigure(createFigure: (() => Figure) | Figure, …)

The union type is used only in tests (passing a pre-built Figure directly). Production code always passes a factory. Mixed-type overloads complicate the signature and blur the documented reason for using a factory (old figure disposed before the new one is created). The test helper should construct via a factory, or replaceFigure should only accept () => Figure.

5. Module-level liveDataManager singleton

// src/service/liveData.ts
export const liveDataManager = new LiveDataManager();

This singleton is shared across every test file that imports from liveData.ts. The unit tests work around this by constructing new LiveDataManager() directly, but any test that uses the singleton (or any code path that calls the module-level setMaidrData / appendMaidrData helpers) will bleed state between test files. The test suite currently avoids this because the new tests use local instances, but the risk grows as the feature is extended. Consider exporting a factory function alongside the singleton, or documenting the limitation clearly.

---

Code Quality

6. resolveId casts through as string

return this.instances.keys().next().value as string;

IteratorResult.value is typed as string | undefined on MapIterator<string>. The guard this.instances.size === 1 makes this safe at runtime, but the cast hides a potential undefined if the code is refactored. Prefer the explicit non-null: this.instances.keys().next().value!.

7. Brittle private-field access in chat.test.ts

const models = (service as unknown as { models: Record<string, { json: string }> }).models;

This reaches into the private implementation to read .json. A narrow package-internal getter (even @internal / @visibleForTesting) would be more maintainable than a double type-cast. Alternatively, the test can verify updateData's effect indirectly by asserting on the prompt string sent to a stubbed fetch, which is the real observable behaviour.

8. cloneMaidrData is untested

The function handles a subtle edge case (peeling off onNavigate before structuredClone). It warrants at least one unit test verifying that the callback is preserved and that the non-function fields are deeply cloned (i.e. mutations to the clone do not affect the original).

---

Minor / Nits

• src/model/plot.ts – the new dispose() comment is the right pattern, but the phrasing ("inserted at construction") will be misleading if someone adds a second call site; the comment would be more durable as "remove the cloned highlight element to avoid stale DOM nodes after a live update".
• docs/react.md – the inline setData example in the useEffect builds the entire subplots tree by hand every tick. While technically valid, this is exactly the footgun appendMaidrData exists to prevent. A note steering readers toward appendMaidrData for streaming would improve discoverability.
• examples/live-line.html – the example maintains its own points array and windowSize in parallel with MAIDR's maxWidth. A comment explaining why the SVG redraw needs its own copy (the charting code is separate from MAIDR) would prevent copy-paste confusion.

---

Summary

The implementation is architecturally sound, well-tested, and follows MAIDR's MVVC conventions. The two correctness items (#1 orphaned JSDoc, #2 finally restoration) are the most important to address before merging. The UX concern around M on static charts (#3) is worth a quick team discussion. Everything else is polish.

Great work overall — this is a complex feature delivered cleanly.

[jooyoungseo]

Addressed both review rounds in 7fa8177 (and the earlier Copilot threads in 7d8668d). Summary:

Fixed

• announceAppendedPoint now restores the trace cursor in a finally block, so a throwing state getter can't leave the cursor mutated.
• The orphaned LiveDataPoint JSDoc is re-attached to its declaration.
• Context.replaceFigure accepts only a factory (tests updated), keeping the dispose-before-construct ordering explicit.
• resolveActiveColShift: documented the layer-index ↔ trace-row 1:1 mapping (subplots build traces[layerIndex][0] by construction) and the catch now logs via console.warn.
• Fields that intentionally dropped readonly are marked mutable-by-design; resolveId uses a non-null assertion; the liveDataManager singleton docs tell tests to construct their own instance.
• Added cloneMaidrData unit tests (deep-clone semantics, onNavigate preserved by reference, absent when omitted).
• Docs: the declarative React streaming example now trims its own window with a note that maxWidth applies only to appendData; react.md/LIVE_DATA.md steer streaming users to appendMaidrData; the demo explains its parallel SVG point copy; added the data-ownership comment in useMaidrController.

Not changed, with reasons

• layerId typed string but layer.id optional: MaidrLayer.id is declared required (id: string) in src/type/grammar.ts, so there is no optionality bug here.
• maxWidth on the prop/setData path: intentional — setData replaces data verbatim (matching chart2music's setData). Now documented explicitly in both docs and the example.
• M key announcing on static charts: kept as a short notification ("Monitoring is available only for live charts"). For screen-reader users, silent dead keys are harder to debug than a one-line explanation; happy to gate the keybinding on live: true instead if maintainers prefer.
• chat.test.ts private-field access: kept the narrow cast for now; asserting on a stubbed fetch payload would couple the test to provider payload shapes.

Verification: 243 unit tests passing (3 failures pre-exist on main), type-check/lint clean, and the 13-check Playwright run against the rebuilt production bundle passes.

---

Generated by Claude Code

[claude]

Code Review — PR #620: Realtime/Streaming Data Support

Overall: This is a well-designed, substantial feature that follows MAIDR's MVVC architecture cleanly. The opt-in live: true design protects existing users, the immutable data-merging approach is sound, and the test coverage (36 unit tests across 4 new test files) is thorough. The items below are mostly medium/low severity and should not block the feature from merging, but the one mutating-trace pattern in Controller warrants a look before final approval.

---

Potential Bugs

1. Temporary trace mutation in announceAppendedPoint (Medium — src/controller.ts:859–888)

trace.isInitialEntry = false;
trace.row = appended.row;
trace.col = appended.col;
const state = trace.state;
this.monitorService.handleNewPoint(state);

Mutating .row/.col/.isInitialEntry and relying on a finally to restore them is fragile under exceptions from trace.state. The finally block does correctly restore, but if trace.state were ever extended to call notifyStateUpdate() internally, this pattern would silently produce spurious observer notifications at the wrong position.

A safer alternative: add a computeStateAt(row, col) method to AbstractTrace that returns state without mutating position, and call that here. This would make the "no side effects" contract explicit.

2. describeShape checks layer count but not layer type (src/model/context.ts:1148–1153)

private describeShape(figure: Figure): string {
  return figure.subplots
    .map(row => row.map(subplot => subplot.getSize()).join(','))
    .join(';');
}

getSize() returns the number of layers per subplot. So a subplot with one bar layer and one replacement with one line layer would be considered "same shape" and have navigation restored. The restored trace.row/col indices should still be valid (they get clamped), but the trace types are incompatible — the restored row could land on an entirely different data structure. Worth verifying that position clamping is always safe across type changes, or including the layer types in the shape signature.

3. restoreNavigation depth-1 ambiguity (src/model/context.ts:1182–1195)

The default branch of the depth switch handles both "user was navigating at Figure level (multiple subplots)" and "user was navigating at Trace level (single-trace chart)". The discrimination figure.subplots.flat().length === 1 ? trace : figure is correct but brittle — it doesn't match how initializePlotContext originally decided the depth, so any future change to construction logic could silently produce a wrong stack. A named enum for stack depth (e.g. NavigationDepth.FIGURE | SUBPLOT | TRACE) would make this explicit.

---

Code Quality

4. ChatService.updateData runs JSON.stringify on every append (src/service/chat.ts:1300–1306)

For high-frequency streaming the serialization cost accumulates. Lazy serialization (compute the string on the first LLM request after a data change, not on each append) would be more efficient and is a straightforward change: store maidr and serialize on getLlmResponse.

5. Test accesses private field via type-cast (test/service/chat.test.ts:2267–2272)

const models = (service as unknown as { models: Record<string, { json: string }> }).models;

This ties the test to the private json property of AbstractLlmModel. If the field is renamed or the storage strategy changes (e.g. lazy serialization per point 4), the test breaks silently. A thin public getDataSnapshot(): string getter on ChatService for test/debug purposes, or testing via observable side-effects (verifying the LLM receives updated content in a mock request), would be more resilient.

6. liveDataManager.setData stores a caller-owned reference without cloning (src/service/liveData.ts:1721–1729)

instance.data = maidr;  // stores the reference
instance.listener({ maidr });

The stored instance.data is read immutably by appendData (via appendPointToMaidr which is correctly immutable), and useMaidrController clones before passing to the controller — so the current call chain is safe. But this is a subtle contract: any caller who calls setData(ref) and then mutates ref corrupts the stored data. A comment documenting that callers must not mutate the passed object after setData returns (or an internal clone) would prevent future bugs.

7. restoreNavigation leaves scope unchanged for shape-matching updates (src/model/context.ts)

When the shape matches, scopeContext is never touched — only the stack is rebuilt. For most cases this is correct (scope should be preserved). But if the user is ever navigating at depth=1-as-figure (SUBPLOT scope) and after the update the plot shape remains valid but the user's depth was depth=1-as-trace (TRACE scope), the scope could be inconsistent with the stack. This is a very edge case but worth a comment that scope is intentionally preserved.

---

Architecture Notes

8. figure and _instructionContext are now mutable in Context (src/model/context.ts)

The PR correctly comments this with // Mutable: replaced in place on live data updates. The same treatment is done for HighContrastService.figure and Controller.figure. This is a reasonable design compromise — the alternative (creating a new Controller on each update) would require re-registering all services and ViewModels. The existing comment is clear enough; just noting this as an intentional deviation from the originally readonly fields.

9. MonitorService takes Observer<PlotState> for audio/text, not service concrete types

This is excellent — accepting interfaces rather than concrete service types keeps MonitorService decoupled and makes it trivially testable. Well done.

---

Minor

• AppendedPointInfo.layerId (src/service/liveData.ts:1497) is always populated from layer.id, but MaidrLayer.id is listed as optional in some contexts. Worth verifying the field is always present in practice, or making the type string | undefined.
• The NESTED_DATA_TYPES constant includes TraceType.VIOLIN_KDE (src/service/liveData.ts:1456). The docs say violin plots use object-shaped heatmap-like data and are not supported for append. If VIOLIN_KDE is a nested-array subtype, this is correct — but a brief comment clarifying it's distinct from heatmap violin data would help.
• The live example (examples/live-line.html) uses aria-hidden="false" on the SVG, which is the default. It could just be omitted rather than explicitly set to the default value.

---

Test Coverage Gaps

• Controller.announceAppendedPoint and Controller.resolveActiveColShift have no unit tests. Both contain non-trivial logic (multi-level navigation access, try/catch paths). They're hard to test via Controller directly, which reinforces the suggestion in point 1 to move the temporary-mutation logic into the Trace class where it can be unit-tested in isolation.
• No test for the useMaidrController useEffect registration / live-update path. This is the React integration glue and would benefit from at least one integration-style test.

---

Summary: The core design is solid and well-tested. The three highest-priority items to address before merge are:

1. The temporary trace mutation in announceAppendedPoint (correctness risk under future refactors)
2. The depth-1 ambiguity in restoreNavigation (maintenance risk)
3. The ChatService serialization on each append if this API will see high-frequency streaming in real deployments

[jooyoungseo]

Third review round addressed in 5c98acb:

Fixed

• placeholder #1 (temporary trace mutation): introduced AbstractTrace.getStateAt(row, col) — the compute-state-without-moving pattern now lives in one unit-tested method on the trace (cursor restored in finally, no observer notifications), and announceAppendedPoint just calls it. Covered by test/model/traceStateAt.test.ts.
• feat: add new architecture #2 (describeShape ignores layer types): the shape signature now uses subplot.traceTypes, so a same-count type change (bar → line) resets navigation instead of restoring onto an incompatible structure. Regression test added.
• feat: add new architecture #3 (depth-1 ambiguity): the depth-1 rebuild now discriminates with the exact predicate initializePlotContext uses (figureState.size !== 1) instead of a parallel heuristic, and the same-shape branch documents that scope is intentionally preserved.
• style: configure typescript-eslint #4 (serialization per append): went with the suggested lazy approach — ChatService stores the latest Maidr and serializes on the first LLM request after a change (cached via getDataJson()); models receive a shared supplier. Streaming appends now do zero serialization work.
• docs: add GitHub issue and PR templates #5 (brittle test): the chat tests now assert the public getDataJson() surface (including laziness and caching) instead of reaching into private model fields.
• docs: add GitHub issue and PR templates #6 (setData reference ownership): documented that the manager stores the caller's reference and it must not be mutated after handoff.
• fix: clean up grammar to align with each plot requirement #7 (scope preservation): comment added.
• Nits: VIOLIN_KDE nested-data clarification added; redundant aria-hidden="false" removed from the demo.

Not changed

• useMaidrController integration test: would need jsdom + React Testing Library, neither of which is in the toolchain; the 13-check Playwright suite exercises this exact path (registration, live updates while focused, monitor announcements) against the production bundle.
• layerId optionality (re-raised in Minor): MaidrLayer.id is required (id: string) in src/type/grammar.ts.

Verification: 250 unit tests passing (3 pre-existing failures on main), type-check/lint clean, 13/13 Playwright checks pass against the rebuilt bundle.

---

Generated by Claude Code

[claude]

Code Review: feat: add realtime/streaming data support (PR 620)

Overview

This PR implements a full realtime/streaming data pipeline for MAIDR across three phases: full data replacement (setData), incremental streaming (appendData with sliding window), and monitor mode (auto-sonification of live data points via the M key). It is a large, well-structured addition that follows the MVVC architecture and ships with 36 new unit tests and comprehensive documentation.

---

Strengths

• Architecture compliance: The feature is wired end-to-end through the Command pattern (ToggleMonitorCommand), Observer pattern (services on the rebuilt model), and Emitter pattern (live updates flow through LiveDataManager to Controller.updateData()). Layer boundaries are respected.
• Immutable data transforms: appendPointToMaidr never mutates the input, returning a new config with structural sharing.
• Position preservation under replacement: Context.replaceFigure() + captureNavigationSnapshot() handles same-shape, shape-changed, and sliding-window-trimmed cases with careful clamping.
• getStateAt pattern: Temporarily moving the cursor in a try/finally block to compute state at an arbitrary position is a pragmatic, safe approach in single-threaded JS.
• Lazy JSON serialization in ChatService: Caching the JSON and invalidating on updateData is a solid optimization for high-frequency streaming.
• Disposable hygiene: MonitorService disposes cleanly; LiveDataManager.register guards against stale listener removal.

---

Issues and Suggestions

Correctness

1. resolveActiveColShift relies on Subplot.row being the layer index

In controller.ts, the comment says: Subplot traces are built as one single-trace row per layer (traces[layerIndex][0]), so the subplot active row IS the layer index. This ties correctness to an internal layout invariant of Subplot. If Subplot ever supports multiple traces per row, this silently breaks sliding-window cursor tracking. Consider exposing an activeLayerIndex getter on Subplot to make the dependency explicit.

2. Verify Figure.dispose() clears the observer list

Controller.updateData() calls this.registerObservers() after the new figure is built. The new figure starts empty, so re-registering is correct -- but only if AbstractObservableElement.dispose() actually clears this.observers (or if old-figure references become unreachable). Worth confirming the dispose chain to ensure no stale observer accumulation across high-frequency streaming updates.

3. Depth-1 restore in restoreNavigation -- keep in sync with initializePlotContext

For a single-layer chart (atFigureLevel false), only trace is pushed with no subplot on the stack. The existing tests confirm this mirrors initializePlotContext behavior today. A comment cross-referencing initializePlotContext would help ensure the two stay in sync as the codebase evolves.

---

Performance

4. structuredClone on every live update for large datasets

cloneMaidrData deep-clones the entire Maidr config on every updateData call. At 1Hz (as shown in the demo) this is fine. For users who might push updates at higher rates, consider noting in the docs that maxWidth is the essential mitigation -- it keeps the serialized payload bounded and each clone cheap.

---

Design

5. ChatService.getDataJson() is public only for testing

The existing test already spies on JSON.stringify to verify laziness, which works without exposing this method. Making it private would avoid coupling the public API to the caching strategy.

6. LiveDataManager.updateStoredData missing ownership caveat in JSDoc

setData documents callers must not mutate the object after handing it over but updateStoredData (same contract) does not. Worth adding the same note for consistency.

7. Silent M key at SUBPLOT scope

The M key is added to TRACE_KEYMAP and BRAILLE_KEYMAP but not SUBPLOT_KEYMAP. On a multi-panel chart, a user at subplot level pressing M gets no response and no feedback. A notification like Monitor mode is available when navigating a trace would help screen reader users understand why M is silent there.

---

Minor

8. Group-out-of-range warning could include current group count

The warning [maidr] appendData: no group at index N would be more actionable if it also said chart has M groups, helping callers debug misconfigured streaming pipelines quickly.

---

Test Coverage

The four new test files are solid. Missing coverage worth considering:

announceAppendedPoint with cross-layer appends: currently untested at the unit level. The guard (if trace is null) is exercised by E2E but a unit test would confirm the off-by-one cases.

useMaidrController hook data.id change path: exercised by E2E; a React Testing Library test would add unit-level confidence.

---

Documentation

docs/LIVE_DATA.md is thorough. Two additions worth considering:

Highlight rebinding: the note if your SVG re-renders with the same selectors is brief. For D3/Vega integrations where element keys change on re-render, stable selector requirements are a common gotcha. A short example or note would help.

Performance guidance: a note that maxWidth is essential above ~5Hz (to keep clone cost bounded) would help users size their configs.

---

Summary

This is a high-quality implementation of a complex feature. The core architecture -- immutable merges, in-place model swap via replaceFigure, monitor mode through the existing Command/Observer pipeline -- is sound. The main items to address before merging:

1. Make the Subplot.row === layerIndex assumption in resolveActiveColShift explicit via an accessor or comment.
2. Confirm AbstractObservableElement.dispose() clears observers to prevent stale references on streaming updates.
3. Add a notification or note for the silent M-key gap at subplot scope.

The rest are suggestions rather than blockers.

[jooyoungseo]

Fourth review round addressed in 522098c:

Fixed

• placeholder #1: added Subplot.activeLayerIndex — the traces[layerIndex][0] invariant is now owned and documented by Subplot itself; resolveActiveColShift uses the accessor.
• feat: add new architecture #2: confirmed the dispose chain clears observers (AbstractPlot.dispose() does this.observers.length = 0, and Figure → Subplot → Trace all run through it), and added a regression test so streaming updates can never silently accumulate stale observers.
• fix: clean up grammar to align with each plot requirement #7: registered the M toggle in SUBPLOT scope as well, so multi-panel users at subplot level get feedback instead of a silent dead key.
• docs: add GitHub issue and PR templates #6: updateStoredData now carries the same reference-ownership caveat as setData.
• fix: notify user of end of plot #8: the out-of-range group warning now includes the layer's group count.
• Docs: added selector-stability guidance for highlight rebinding (stable classes for keyed D3/Vega re-renders, and the graceful count-mismatch fallback) plus performance guidance — per-update cost scales with chart size, so maxWidth is the essential bound for fast streams.

No change needed

• feat: add new architecture #3 (depth-1 cross-reference): the comment already states "Discriminate exactly like initializePlotContext does" and the branch uses that method's exact predicate as of the previous round.
• docs: add GitHub issue and PR templates #5 (getDataJson visibility): keeping it public — the previous review round specifically criticized tests reaching into private state, and this read-only accessor is the agreed public surface; making it private would reintroduce that problem.
• Test gaps: announceAppendedPoint unit tests remain impractical at the Controller level (its core logic now lives in the unit-tested Trace.getStateAt); the React hook path stays covered by the 13-check Playwright suite since jsdom/RTL aren't in the toolchain.

Verification: 251 unit tests passing (3 pre-existing failures on main), type-check/lint clean, 13/13 Playwright checks pass against the rebuilt bundle.

---

Generated by Claude Code

[claude]

Code review for PR 620 - realtime/streaming data support.

OVERVIEW: The PR adds setData (full replacement), appendData (streaming), and monitor mode. Architecture is sound - rebuilds model from new data through existing constructors, restores navigation at Context level. Good MVVC compliance and test coverage.

ISSUES FOUND:

1. TYPE BUG - AppendedPointInfo.layerId typed as string but layer.id is string|undefined (src/service/liveData.ts). The AppendedPointInfo interface declares layerId: string, but MaidrLayer.id is optional. The assignment layerId: layer.id silently stores undefined through a string-typed field. Fix: type as string|undefined or add a fallback.

2. NON-NULL ASSERTION - resolveId uses instances.keys().next().value! - safe due to size===1 guard but pattern smell. Prefer Array.from(this.instances.keys())[0].

3. GETSTATEAT MUTATION - AbstractTrace.getStateAt temporarily mutates this.row/this.col/this.isInitialEntry then restores in finally. Works for sync JS. Add a comment warning about re-entrancy hazard so future async getters or calls from within observer notifications do not see stale values.

4. WINDOW ASSIGNMENT - window.maidrLive assigned unconditionally at module top level in index.tsx. Throws in SSR/Node. Add typeof window !== 'undefined' guard.

5. MONITORSERVICE TYPES - Constructor accepts Observer but receives AudioService/TextService which implement Observer. Bivariance lets it compile but Observer would be more accurate.

6. DESCRIBESHAPE - Does not include group/series counts so two multiline charts with different series counts get same shape string and navigation is restored rather than reset. Clamping prevents crashes but edge case should be documented.

7. PERFORMANCE - No debounce on appendData. Each call rebuilds full model. maxWidth bounds the cost but a note in AppendDataOptions JSDoc about high-frequency streaming would help consumers avoid accidental misuse.

POSITIVES: Minimal blast radius (live:true opt-in), DOM cleanup in Subplot.dispose via highlightValue.remove(), immutable append logic, lazy JSON cache in ChatService, excellent navigation snapshot/restore implementation, good test coverage.

SUMMARY: Approve after addressing (1) layerId type unsafety and (3) getStateAt re-entrancy comment. Everything else is minor.

Review generated with Claude Code

[jooyoungseo]

Fifth review round addressed in 572f23c:

Fixed (documentation items)

• feat: add new architecture #3: getStateAt JSDoc now warns about the re-entrancy hazard — the pattern is safe only while state getters stay synchronous and notification-free.
• docs: add GitHub issue and PR templates #6: describeShape now documents why per-layer series counts are deliberately excluded: appendData can start a new series (groupIndex === count) and that must preserve the user's position; restored indices are clamped, so a differing series count can never land out of range.
• fix: clean up grammar to align with each plot requirement #7: AppendDataOptions JSDoc notes that per-update cost scales with total chart size and that Maidr.maxWidth is the mitigation for high-frequency streams (this also lives in docs/LIVE_DATA.md).

Not changed, with verification

• placeholder #1 (layerId type bug): verified again — MaidrLayer.id is declared required (id: string) at src/type/grammar.ts:403. There is no string | undefined assignment; this claim has now been checked three times.
• docs: add GitHub issue and PR templates #5 (Observer<PlotState> vs Observer<TraceState>): both AudioService and TextService declare implements Observer<PlotState> with update(state: PlotState) (audio.ts:71/189, text.ts:38/622). MonitorService's parameter type matches them exactly — no bivariance is involved; it passes a TraceState, which is a member of the PlotState union, soundly.
• style: configure typescript-eslint #4 (SSR guard on window.maidrLive): src/index.tsx is the browser-only script-tag bundle and already accesses window/document unconditionally at module top level (e.g. window.disconnectMaidrObservers, the DOMContentLoaded wiring). SSR/npm consumers use maidr/react, which never imports this entry. Adding a guard to one line would be inconsistent with the file's established conventions without enabling any new environment.
• feat: add new architecture #2 (resolveId non-null assertion): the previous review round specifically requested this exact ! form (replacing an as string cast); Array.from(keys())[0] types as string | undefined too, so it wouldn't remove the assertion, just hide it.

Verification: 251 unit tests passing (3 pre-existing failures on main), type-check/lint clean.

---

Generated by Claude Code

[claude]

Code Review — feat: add realtime/streaming data support

Overview

Substantial and well-executed feature PR. The architecture follows MAIDR's MVVC pattern cleanly — LiveDataManager acts as a registry/router, MonitorService handles monitoring logic, and Context.replaceFigure() encapsulates in-place model swapping. The feature is correctly opt-in (live: true) so static charts are unaffected.

Verdict: Approve with minor suggestions. One potential bug worth confirming before merge.

---

Potential Bug — announceAppendedPoint indexing

In controller.ts:

const trace = subplot?.traces[appended.layerIndex]?.[0];

appended.layerIndex is the layer's index within the subplot's layers array, while subplot.traces is a 2D navigation grid (Trace[row][col]). For a single-layer subplot this happens to work, but for multi-layer subplots the indexing may be transposed. Please confirm this is intentionally traces[layerIndex][0] and not traces[0][layerIndex].

---

Issues to Address

1. window.maidrLive runs unconditionally at module load (src/index.tsx)

In SSR environments (Next.js, Remix) window is undefined at server render time, so importing the module crashes. Guard it:

if (typeof window !== 'undefined') {
  window.maidrLive = { setData: ..., appendData: ... };
}

2. AppendedPointInfo.layerId type mismatch

The interface declares layerId: string (non-optional), but layer.id in MaidrLayer is typed as string | undefined. In appendPointToMaidr, layerId: layer.id could be undefined at runtime. Change to layerId: string | undefined or add a fallback (layer.id ?? '').

3. getStateAt re-entrancy note is incomplete

The comment documents the hazard for async state getters, but autoplay running concurrent setTimeout ticks is a more immediate concern: if a microtask yields between the cursor assignment and the finally-restore, an autoplay tick could observe the temporary position. The comment should explicitly state this is only safe because no await points exist in the entire call chain.

4. ViolinKDE silently included in NESTED_DATA_TYPES

KDE bin values are typically pre-computed from raw data — appending a literal point doesn't recompute the distribution, which may produce semantically incorrect output. Either document this limitation in LIVE_DATA.md (alongside the heatmap note) or emit a warning.

---

Non-blocking Suggestions

5. Observer re-registration cost per append — updateData calls registerObservers() on every live update, rebuilding all observer lists each time. For high-frequency streams capped with maxWidth, this cost is paid repeatedly and is proportional to total chart size.

6. Silent disposal on update failure — When a live update throws, the controller is silently disposed and only a console.error is emitted. An accessibility user on a live chart gets no indication that updates have stopped. A NotificationService.notify('Live updates paused') in the catch block would announce this to screen readers.

7. MonitorService type parameter — The constructor accepts Observer<PlotState> but handleNewPoint passes a TraceState. This is structurally valid since TraceState extends PlotState, but using Observer<TraceState> would make the intent explicit and prevent future misuse.

8. Document setData cursor clamping when series count changes — describeShape intentionally excludes series counts so appendData can add a new group without resetting navigation. The side effect is that a setData that removes a series silently clamps the cursor instead of resetting it. Worth adding to "Behavior Details" in LIVE_DATA.md.

---

What's Done Well

• Immutable data merging in appendPointToMaidr — spreads all the way down without mutating the original config.
• Clean separation of concerns: LiveDataManager is pure routing, MonitorService owns announcement, Context.replaceFigure() owns position preservation.
• Position clamping + shape-change reset in restoreNavigation is correctly implemented, including the sliding-window shift edge case.
• Lazy JSON serialization in ChatService.getDataJson() avoids JSON.stringify overhead on every streaming update.
• Test coverage is excellent — TDD approach, edge cases for empty layers and new group creation, 11 E2E checks covering the full streaming lifecycle.
• Backward compatibility — static charts are completely unaffected; all new paths require explicit opt-in.
• Documentation — LIVE_DATA.md is comprehensive and the runnable examples/live-line.html demo is a great addition.

---

Generated with Claude Code

[jooyoungseo]

Sixth review round addressed in 033fab3:

Confirmed (no change needed)

• announceAppendedPoint indexing: traces[layerIndex][0] is correct by construction — Subplot builds this.traces = layers.map(layer => [TraceFactory.create(layer)]) (plot.ts:346), i.e. one single-trace row per layer. The Subplot.activeLayerIndex accessor added in an earlier round exists precisely to own and document this invariant.

Fixed

• fix: clean up grammar to align with each plot requirement #7: MonitorService now types its collaborators as Observer<TraceState>, matching exactly what handleNewPoint feeds them. (PlotState-accepting services remain assignable — a function accepting the wider union is soundly contravariant — so no call sites changed.)
• feat: add new architecture #3: the getStateAt re-entrancy note now states explicitly that safety depends on the entire call chain being synchronous, so timers such as autoplay ticks cannot interleave before the finally-restore.
• style: configure typescript-eslint #4: LIVE_DATA.md documents the violin KDE caveat — appends are structurally accepted but do not recompute the density distribution; setData with freshly computed densities is recommended.
• fix: notify user of end of plot #8: LIVE_DATA.md Behavior Details now covers series-count changes: a setData removing the user's series clamps the cursor to the nearest remaining series rather than resetting navigation.

Not changed, with reasons

• placeholder #1 (SSR guard) and feat: add new architecture #2 (layerId type): both re-raise claims verified false in the previous round — src/index.tsx is the browser-only script-tag bundle that accesses window/document unconditionally throughout (npm/SSR consumers import maidr/react, which never loads it), and MaidrLayer.id is required (id: string, grammar.ts:403), so layerId: layer.id can never be undefined.
• docs: add GitHub issue and PR templates #5 (observer re-registration cost): re-registering observers is a handful of array pushes per trace — orders of magnitude below the model rebuild it accompanies; maxWidth bounds both.
• docs: add GitHub issue and PR templates #6 (announce on update failure): intentionally left as console-level. The failure path disposes the controller, which also tears down the live region that would carry the announcement; the next focus-in rebuilds from stored data, restoring function when the data is valid. When the data itself is invalid, this is an integrator error surfaced to the developer console — happy to revisit the recovery UX if maintainers prefer a different behavior.

Verification: 251 unit tests passing (3 pre-existing failures on main), type-check/lint clean.

---

Generated by Claude Code

[claude]

Code Review — feat: add realtime/streaming data support

Overview: This PR adds realtime/streaming data support via three phases: setData (full replacement), appendData (streaming), and monitor mode (M key). The implementation is architecturally sound, well-documented, and includes strong test coverage (36 unit tests + 11 E2E checks). Below are targeted findings.

---

Correctness / Potential Bugs

1. AppendedPointInfo.layerId type unsafety (src/service/liveData.ts:1689)

export interface AppendedPointInfo {
  layerId: string;  // typed as string...

But the source is:

layerId: layer.id,  // ...but layer.id may be undefined

MaidrLayer.id appears to be optional. If a layer has no id, the appended.layerId field silently becomes undefined while the type says string. Consider typing it string | undefined or asserting that layers used in streaming must have an id.

---

2. announceAppendedPoint always indexes traces[layerIndex][0] (src/controller.ts:863)

const trace = subplot?.traces[appended.layerIndex]?.[0];

For multi-group traces the append info already carries appended.row (= group index), but the monitor is given the state for the appended point via trace.getStateAt(appended.row, appended.col). This works correctly today because each layer is a single Trace object that handles groups internally. The comment on activeLayerIndex makes this invariant explicit, which is good—just worth a brief inline note here too so a future refactor doesn't silently break it.

---

3. Formatter refresh on every streaming update (src/controller.ts:813)

this.formatterService.refresh(maidr);

This calls formatters.clear() + full reinit on every appendData call. For formatters that are invariant across streaming updates (axis labels don't change), this is wasted work proportional to layer count. Consider diffing or only refreshing when the format config actually changes. Low priority for typical usage with maxWidth, but could matter for dense multi-layer charts.

---

Architecture

4. MonitorService directly drives AudioService and TextService (src/service/monitor.ts:122-128)

public handleNewPoint(state: TraceState): void {
  this.audio.update(state);
  this.text.update(state);
}

This is a deliberate, well-justified deviation from the Observer pattern (we need to announce without moving the cursor). The getStateAt + direct update approach is clean. Worth a brief rationale comment here (similar to the re-entrancy note in getStateAt) so reviewers don't mistake it for a pattern violation: "Direct call instead of notifyStateUpdate() to avoid moving the user's cursor."

---

5. window.maidrLive unconditional assignment (src/index.tsx:949-952)

window.maidrLive = {
  setData: maidr => liveDataManager.setData(maidr),
  appendData: (point, options) => liveDataManager.appendData(point, options),
};

No guard against an existing window.maidrLive. If a user has defined their own global (or MAIDR is loaded twice), it silently overwrites. A window.maidrLive ??= or at least a console.warn on clobber would be more defensive.

---

Design / API Surface

6. ChatService.getDataJson() made public for testability

This exposes an implementation detail. The tests validate lazy-serialization behavior correctly, but a @internal JSDoc tag (or a note in the docstring) would signal to consumers that this is not part of the public contract and may change.

---

Tests

7. TraceState stub in monitor.test.ts is brittle (test/service/monitor.test.ts:2981)

return { empty: false, type: 'trace' } as TraceState;

The as TraceState cast will silently fail if handleNewPoint ever accesses additional state fields (e.g., for richer audio or text generation). This is a known limitation of partial-stub testing; a comment acknowledging it (// stub — handleNewPoint only checks .empty) would make the intent clear.

---

8. No test for the TOGGLE_MONITOR command factory path

CommandFactory.create('TOGGLE_MONITOR') wiring is exercised implicitly by E2E, but there's no unit test asserting that the factory constructs a ToggleMonitorCommand and that execute() delegates to the service. Consistent with how other toggle commands are tested in the suite.

---

Minor

• resolveId non-null assertion (src/service/liveData.ts:2000): this.instances.keys().next().value! — the size check above guarantees non-null, but an explicit as string or a guard would be clearer.
• SUBPLOT_KEYMAP gets TOGGLE_MONITOR — intentional for consistency with other toggles, but monitor mode isn't meaningful at subplot scope. No behavior harm since MonitorService.toggle() guards on isLive.
• getStateAt re-entrancy hazard comment (src/model/abstract.ts:976) is exactly the right level of documentation for a subtle synchronous mutation-and-restore pattern.

---

Summary

This is a high-quality feature implementation. The immutable data-merge approach in appendPointToMaidr, the position-preserving replaceFigure with shape diffing, and the getStateAt pattern for cursor-free state inspection are all well-designed. The main actionable items are: (1) fix the layerId: string vs possibly-undefined type, and (2) consider a guard on the window.maidrLive assignment. Everything else is polish.

[jooyoungseo]

Seventh review round addressed in 1f8a7e5:

Fixed

• docs: add GitHub issue and PR templates #5: window.maidrLive assignment now warns when it would clobber an existing global (script loaded twice / user-defined global). Latest bundle still wins so its own components stay wired to the matching manager instance.
• fix: notify user of end of plot #8: added a ToggleMonitorCommand unit test asserting execute() delegates to MonitorService.toggle().
• feat: add new architecture #2, style: configure typescript-eslint #4, docs: add GitHub issue and PR templates #6, fix: clean up grammar to align with each plot requirement #7 (comments): inline note at the monitor announce site documenting the traces[layerIndex][0] invariant (cross-referencing Subplot.activeLayerIndex); rationale comment in MonitorService.handleNewPoint explaining the deliberate direct update() calls (announce without moving the cursor or fanning out to highlight/braille); getDataJson JSDoc marks the caching strategy as not part of the stable API; the monitor test's partial TraceState stub documents its intent.

Not changed, with reasons

• placeholder #1 (layerId type): fifth occurrence of this claim. MaidrLayer.id is required — id: string at src/type/grammar.ts:403 — so layerId: layer.id cannot be undefined.
• feat: add new architecture #3 (formatter refresh per append): refresh re-resolves a handful of format functions per layer — constant-time work orders of magnitude below the model rebuild it accompanies. Diffing the format config would cost about as much as rebuilding it.
• Minor (resolveId assertion): this line has now received three mutually contradictory style requests across rounds (! over as string in round 3; Array.from(...)[0] in round 5; as string in round 7). Keeping the round-3 form, which the codebase already uses elsewhere (e.g. plotContext.peek()!).

Verification: 252 unit tests passing (3 pre-existing failures on main), type-check/lint clean, 13/13 Playwright checks pass against the rebuilt bundle.

---

Generated by Claude Code

[claude]

Code Review: feat: add realtime/streaming data support

Overview

This PR implements a well-designed realtime/streaming data system for MAIDR across three phases: full data replacement (setData), point-level streaming (appendData), and monitor mode (M key). The core approach — rebuilding the model from the full merged data and restoring navigation position at the Context level — is the right call over making 15 trace classes mutable, and the implementation is largely solid.

---

Architecture Compliance

• MonitorService follows the existing service pattern correctly, implementing Disposable and using dependency injection.
• ToggleMonitorCommand is a clean, minimal Command pattern addition.
• LiveDataManager is a reasonable new pattern (registry/router), and the singleton is documented with a test warning.
• replaceFigure elegantly rebuilds the Figure/Subplot/Trace hierarchy through existing tested constructors — no new mutation surface on any of the 15+ trace classes.
• Observer re-wiring via registerObservers() after each update reuses existing infrastructure correctly.
• MonitorService.handleNewPoint correctly calls audio.update() and text.update() directly (bypassing notifyStateUpdate()) — the comment explaining why is essential and present.

---

Issues

Medium: getStateAt temporarily mutates shared state

AbstractTrace.getStateAt moves this.row/this.col to a requested position, reads this.state, then restores them in a finally block. The method's own comment acknowledges the re-entrancy hazard: "If a getter ever becomes async or triggers notifications, callers could observe the temporary cursor."

This pattern is fragile. If state getters are ever extended with side effects, or if a future async-aware refactor is applied, this silently breaks. The finally-restore is safe today because JS is single-threaded and all getters are synchronous — but that constraint is invisible at the call site.

Suggestion: Consider a protected computeStateAt(row, col) that accepts position explicitly, with the existing get state() delegating to computeStateAt(this.row, this.col). This makes the non-mutating path the primary one rather than a cursor-swap exception.

Medium: layerId typed as string but layer.id may be undefined

In AppendedPointInfo, layerId is typed as string (non-optional), but it is assigned from layer.id in appendPointToMaidr. If MaidrLayer.id is optional in the grammar, this silently coerces undefined to string. Since AppendedPointInfo.layerId is used as a routing key in resolveActiveColShift and announceAppendedPoint, an undefined-masking-as-string could produce subtle failures.

// Verify layer.id is guaranteed non-optional at this point
layerId: layer.id,

Medium: Immutable update overhead scales with chart size, not point size

Each appendData call allocates new objects for every layer and subplot via spread/map. For a multi-panel dashboard with many subplots or many layers per subplot, this is O(subplots x layers) per streamed point. The docs mention cost "scales with total chart size" but frame maxWidth as the only mitigation.

For multi-panel live dashboards at high update rates, this could be significant. The AppendDataOptions JSDoc and docs could explicitly call out that multi-panel configurations require extra care with update frequency.

Low-Medium: TOGGLE_MONITOR in SUBPLOT_KEYMAP

Monitor mode is wired to M in TRACE, BRAILLE, and SUBPLOT scopes. At the subplot scope there is no active trace data point to announce, so toggling monitor on at this level sets enabled = true with no immediate effect. If the user then navigates into a trace, monitoring starts immediately without pressing M again. This matches the stored state but may surprise users who accidentally pressed M while browsing subplots.

Low: setData stores reference without defensive copy

LiveDataManager.setData (and updateStoredData) stores the caller's Maidr reference directly. The JSDoc says "callers must not mutate the object," but window.maidrLive.setData calls liveDataManager.setData without cloning. A caller who mutates the object after calling setData would silently corrupt the registry state without triggering a notification. The useMaidrController hook correctly calls cloneMaidrData before updateData, so the risk is mainly for script-tag consumers. Consider documenting this in the MaidrLiveApi interface JSDoc.

Low: Error recovery after failed updateData loses chart focus

When controllerRef.current.updateData throws, disposeController() is called, dropping the active controller. The next focus-in rebuilds from latestDataRef. This is a safe fallback, but a malformed streaming update causes the user to lose focus entirely. The error path is logged clearly, so this is an acceptable trade-off — just worth calling out explicitly.

---

Positive Highlights

• Position restoration with clamping in restoreNavigation is thorough — covering figure/subplot/trace row+col, initial-entry flag, multi-layer subplots, shape-change detection, and the sliding-window shift. The 9 contextReplaceFigure test cases cover the important branches well.
• Lazy JSON.stringify caching in ChatService (cachedJson = null on update, stringify only on first getDataJson() call) is a clean solution for high-frequency streaming without AI-chat overhead.
• cloneMaidrData centralization properly handles the onNavigate function-stripping edge case in one place, used consistently.
• Observer cleanup on dispose() — Subplot.dispose() now removes the cloned highlight DOM node, preventing stale element accumulation across live updates.
• Test coverage is extensive: 36 new unit tests across liveData merging, sliding window, position preservation, monitor mode, and chat serialization. The mutation-safety test is especially important for a streaming API.
• describeShape excluding group/series counts is a deliberate and correct design choice: appendData can add a new series, and restoring position across that must not reset navigation.

---

Minor Nits

• AppendedPointInfo.layerId: string — if this is intentionally required, consider adding a validation assert or fallback at the assignment site to make the non-undefined contract explicit.
• The examples/live-line.html demo mutates a local points array for the SVG redraw while MAIDR uses immutable updates. The comment explains this is the charting-library side, but the juxtaposition could mislead developers into thinking it is acceptable to mutate MAIDR data directly.

---

Summary

The design is sound and the implementation is careful. The main risk is getStateAt's cursor-swap pattern and the layerId: string typing assumption against a potentially optional MaidrLayer.id. The immutable-append overhead is worth documenting more prominently for multi-panel use cases. Everything else is minor. The test coverage for the core logic is genuinely good.

[jooyoungseo]

Eighth review round addressed in 9380ec0:

Fixed (documentation)

• Multi-panel cost now called out explicitly: per-update cost covers all subplots/layers of a multi-panel figure, not just the layer receiving the point (LIVE_DATA.md).
• The no-mutation-after-handoff contract is now stated on the public MaidrLiveApi.setData JSDoc, where script-tag consumers actually read it.
• The demo comment now draws the boundary explicitly: mutating its local SVG-redraw array is fine; never mutate data already handed to MAIDR.

Not changed, with reasons

• getStateAt → parameter-threading refactor: the suggestion ("get state() delegating to computeStateAt(row, col)") would require rewriting the audio/braille/text/highlight getters across all 15 trace classes to take positions as parameters — exactly the high-risk refactor this PR's design deliberately avoids (per the issue's barrier analysis). The cursor-swap is contained in one unit-tested method whose synchronous/no-notification constraint is documented at the definition; a future async refactor of state getters would be a deliberate architectural change that revisits this method by name.
• layerId optionality: sixth occurrence. MaidrLayer.id is required (id: string, src/type/grammar.ts:403) — "verify layer.id is guaranteed non-optional" has been verified in each of the last four rounds. Adding a runtime assert for a state the type system already excludes would be dead code.
• TOGGLE_MONITOR at SUBPLOT scope: added at review round four's explicit request. Toggling there announces "Monitoring on", so the user is informed before any later announcements arrive; monitoring is chart-wide by design (chart2music behaves the same).
• Failed-update focus loss: acknowledged trade-off, already documented at the recovery site in useMaidrController ("drop the controller so the next focus-in rebuilds cleanly").

Verification: 252 unit tests passing (3 pre-existing failures on main), type-check/lint clean.

---

Generated by Claude Code

[claude]

Code Review — feat: add realtime/streaming data support

Overall this is a well-architected, impressively thorough feature addition. The design rationale (rebuild-through-constructors rather than making 15 trace classes mutable) is sound, the test coverage is commendable (36 unit + 11 E2E), and the documentation is comprehensive. Below are targeted observations and suggestions.

---

Architecture & Design

Strengths

• The rebuild-and-restore strategy in Context.replaceFigure() correctly leverages the existing, tested constructors rather than introducing bespoke mutation logic across every trace type. This is the right trade-off.
• LiveDataManager is a clean singleton registry with proper guard against stale registrations on dispose (listener identity check). Good defensive programming.
• Routing monitor announcements through direct Observer.update() calls (bypassing notifyStateUpdate()) to avoid moving the cursor is a clever reuse of existing interfaces.

Concerns

1. Controller.figure mutability is now implicit. Removing readonly from this.figure without a corresponding type-level annotation makes the mutation invisible to future maintainers. The comments help, but consider ensuring both mutation sites in Controller and Context are consistently documented.

2. getStateAt re-entrancy hazard is load-bearing but fragile. The comment acknowledges the synchronous-only guarantee, but this is a contract that every future async change to state getters must not break. Consider adding a guard flag to turn silent corruption into a loud failure during development.

3. AppendedPointInfo.layerId has a type mismatch. layerId is typed string (non-optional) but is set to layer.id, and MaidrLayer.id is optional (id?: string). This can produce undefined in a string-typed field at runtime, which TypeScript will not catch. Either widen AppendedPointInfo.layerId to string | undefined, or add a fallback.

---

Performance

4. Full-model rebuild on every appendData call. Documented and the stated trade-off. It would be worth adding a concrete rough guideline (e.g. a safe update-rate estimate) in docs/LIVE_DATA.md to set realistic expectations for users building high-frequency dashboards.

5. No maxWidth = unbounded memory growth. If users append rapidly without maxWidth, the stored config grows unboundedly. Worth a one-liner note in the docs.

---

Correctness / Edge Cases

6. SUBPLOT_KEYMAP includes TOGGLE_MONITOR but the help panel presumably renders all keybindings unconditionally. Users in subplot scope on a non-live chart will see "Toggle Monitor Mode (Live Charts)" in the help panel even when monitoring is unavailable. Consider this a follow-up if the help UI does not support conditional entries.

7. Context.replaceFigure disposes the old figure before calling createFigure. If createFigure() throws, this.figure is already disposed and the context is left with a dangling reference. The Controller.updateData catch in the hook disposes the controller, which is the correct recovery path, but a comment or local try/catch inside replaceFigure would make this invariant more explicit.

---

Security / API Surface

8. window.maidrLive is assigned unconditionally at module load. For SSR or CSP-strict environments where window may be undefined, the direct assignment will throw. A typeof window !== 'undefined' guard would future-proof this.

9. setData stores the caller's reference without cloning. instance.data inside the manager holds the original reference. A mutation by the external caller between setData and the next appendData would corrupt the stored baseline. Consider cloning on ingress inside setData, or explicitly documenting the narrow race window.

---

Test Coverage

Strengths

• 36 unit tests covering merging, sliding window, manager registry, monitor toggle, position preservation/clamping, and chat freshness is excellent.
• Using new LiveDataManager() in tests rather than the singleton avoids test pollution — well done.

Gaps

10. getStateAt restore-on-finally is not directly tested. A targeted unit test confirming row/col/isInitialEntry are restored even when this.state throws would close this gap.

11. window.maidrLive double-load warning is untested. The guard is easy to accidentally remove; a quick test asserting console.warn fires when the property already exists would add confidence.

12. Shape-change path in replaceFigure (branch that calls scopeContext.clear() and initializePlotContext) has no dedicated test. Adding a shape-change case would give confidence that scope reset works correctly.

---

Minor / Style

• ToggleMonitorCommand in toggle.ts assigns monitorService explicitly in the constructor body rather than using TypeScript's parameter shorthand. All other commands in the file use private readonly X: T in the constructor signature — this one should be consistent.
• MonitorService.isLive is assigned in the constructor body even though the other injected fields use the parameter shorthand — minor inconsistency.
• examples/live-candlestick.html appears in the diff but is not mentioned in the PR description under "Documentation".

---

Summary

Priority	Item
High	AppendedPointInfo.layerId type mismatch with optional MaidrLayer.id (item 3)
High	window.maidrLive SSR / undefined-window guard (item 8)
Medium	replaceFigure leaves dangling figure reference if createFigure throws (item 7)
Medium	setData caller-mutation race on stored reference (item 9)
Low	getStateAt and shape-change test gaps (items 10, 12)
Low	ToggleMonitorCommand constructor style inconsistency

Great feature — the accessibility angle (monitor mode mirroring chart2music) and the documentation completeness are especially appreciated.

[claude]

Code Review: feat: add realtime/streaming data support

This is a substantial, well-engineered feature that implements live/streaming data in a sound architectural way. The core design decision — rebuilding the model through existing constructors and restoring navigation state in Context, rather than making all 15 trace classes mutable — is excellent and keeps the blast radius small for static charts.

---

Overview

The PR delivers three coordinated capabilities:

• Phase 1 (setData): full model replacement via Context.replaceFigure()
• Phase 2 (appendData): immutable point-level merging with applySlidingWindow and a LiveDataManager routing registry
• Phase 3 (monitor mode): cursor-free sonification/announcement of new points via MonitorService

The architecture is clean, MVVC layer boundaries are respected, and test coverage is comprehensive (50+ unit tests, 17 E2E checks).

---

Potential Issues

1. getStateAt temporary-mutation hazard (medium)

AbstractTrace.getStateAt temporarily mutates this.row, this.col, and this.isInitialEntry, relying on JS single-threaded execution to prevent interleaving. The comment documents this well, but the assumption is load-bearing. The autoplay service uses setInterval/setTimeout, but because those are macrotasks they cannot preempt this synchronous block, so the current code is safe. However, any future change that makes state getters async (or adds observer notifications inside them) would silently corrupt the cursor. Consider a guard flag (e.g., _computingStateAt) that throws if set when observers are notified, to catch regressions early.

2. replaceFigure disposes old figure before factory runs (low–medium)

If createFigure() throws (e.g., malformed live data), the Context holds a disposed figure and an empty plot stack. The catch in useMaidrController correctly calls disposeController(), so the user recovers on the next focus-in. The disposal-first order is intentional (to remove stale SVG highlight clones before new selectors are queried), but adding a short comment in updateData noting that the error path is handled at the hook level would help future readers.

3. VIOLIN_KDE silently accepted by appendData without a runtime warning (low)

Heatmap layers emit a clear console.warn when appended. Violin KDE layers are listed in NESTED_DATA_TYPES so they do not error, but the docs note that appending raw observations does not recompute the distribution. Adding a console.warn (similar to the heatmap one) would make this limitation discoverable at runtime.

4. window.maidrLive unconditionally reassigned (low)

The warning fires but the reassignment happens regardless. If a consumer intentionally pre-populated window.maidrLive before the script loaded (e.g., to shim the API in a test environment), it will be silently replaced. A simple guard (window.maidrLive ??= { ... }) or explicit early return would avoid this.

---

Minor Notes

• restoreTracePosition uses O(n) clamping loops — scanning backwards via trace.isMovable to find a valid row/col. For large maxWidth values this is linear in the worst case. Acceptable for current usage.

• liveDataManager module singleton — the comment correctly advises tests to construct their own new LiveDataManager(). Worth enforcing via a Jest setup hook that clears state between suites, since accidental registration leaks across tests are subtle.

• MaidrLiveApi interface is defined in src/index.tsx but not re-exported — React TypeScript consumers who import from maidr/react cannot currently type window.maidrLive as MaidrLiveApi without going through the internal path. Consider re-exporting from react-entry.ts.

• depth-1 discrimination in restoreNavigation mirrors initializePlotContext almost verbatim. Extracting a shared private helper would reduce duplication risk if the initialisation logic changes.

---

Strengths

• Immutable data path in appendPointToMaidr — returning a new config sharing untouched subtrees, never mutating the input, is exactly right for the ownership model.
• cloneMaidrData peeling onNavigate before structuredClone — cleanly handles the edge case of non-serialisable function references.
• LiveDataManager.register dispose guard — comparing listener identity before deleting prevents a later registration from being evicted by an earlier one's cleanup. Nice defensive touch.
• Lazy JSON serialisation in ChatService — cachedJson = null on each data update means high-frequency streaming incurs zero serialisation cost per point.
• Monitor mode output path — MonitorService.handleNewPoint calls audio.update()/text.update() directly instead of notifyStateUpdate(), correctly preventing highlight, braille, and cursor updates from firing.
• Test coverage is thorough — edge cases in liveData.test.ts (empty layer, new series, candlestick trim shift, cursor follow) are exactly the ones that could regress silently.
• Documentation quality — docs/LIVE_DATA.md is clear, covers all code paths, and includes runnable examples. The behavior-details section on silent updates, position preservation, and performance is particularly useful for integrators.

---

Summary

This is high-quality work. Issues 1–2 are worth addressing before merge: the getStateAt mutation contract is safe today but fragile, and the replaceFigure partial-failure path could leave the context unrecoverable without the current try/catch in the hook. Issues 3–4 are low-risk but easy to fix. Overall the implementation is solid, the architecture choices are sound, and the test coverage is reassuring.

[jooyoungseo]

Tenth review round — two items taken in a7550a9, the rest previously settled:

Fixed

• MaidrLiveApi not reachable from maidr/react: good catch — the interface moved into the liveData service and is now re-exported from maidr/react, so TypeScript consumers can type window.maidrLive without importing the side-effectful script-tag entry.
• Depth-1 discrimination duplication: extracted Context.isFigureLevel() as the single source of truth used at construction, instruction resolution, and depth-1 stack restoration (this concern had now come up twice).

Previously settled (no change)

• getStateAt guard flag / async hazard (rounds 6, 8, 9): contract documented at the definition, throw-path restore now unit-tested (traceStateAt.test.ts).
• replaceFigure dispose-before-factory failure path (rounds 7, 9): recovery is documented and implemented at the hook level; the reviewer's own analysis confirms it is the correct path.
• VIOLIN_KDE append semantics (round 6): documented per that round's offered alternative; a per-append runtime warn would be noise at streaming rates.
• window.maidrLive ??= (rounds 7, 9): deliberate latest-bundle-wins with a clobber warning — preserving a pre-load shim with ??= would leave real charts wired to a different manager instance than the API the page sees.
• Singleton test-isolation Jest hook: tests construct their own LiveDataManager; the constraint is documented on the singleton.

Verification: 259 unit tests passing (3 pre-existing on main), type-check/lint clean, 17/17 Playwright checks against the rebuilt bundle.

---

Generated by Claude Code

[claude]

Code Review — feat: add realtime/streaming data support

Overall: This is a large, well-engineered feature. The architecture choices are sound — rebuilding the model from tested constructors rather than adding per-class mutability, restoring navigation state at the Context level, keeping static charts completely unaffected, and backing everything with 50+ unit tests and 17 Playwright E2E tests. The documentation (inline JSDoc, LIVE_DATA.md, cross-linked references) is exemplary. The findings below are mostly polish and a few structural edge cases that warrant attention before merge.

---

Bugs / Correctness

1. window.maidrLive assigned at module evaluation time — SSR will throw

src/index.tsx:26-30

if (window.maidrLive) { ... }
window.maidrLive = { ... };

This runs unconditionally at module load. In SSR environments (Next.js, Astro), window is not defined and the bundle will throw at import time. A minimal guard:

if (typeof window !== 'undefined') {
  if (window.maidrLive) { ... }
  window.maidrLive = { ... };
}

Given the PR explicitly targets React consumers, this could be a real blocker for SSR-based apps.

---

2. Zombie figure reference if createFigure() throws inside replaceFigure

src/model/context.ts

this.figure.dispose();         // old figure disposed
const figure = createFigure(); // if this throws → this.figure is dead, unreplaced
this.figure = figure;

If createFigure() throws, this.figure remains pointing to the already-disposed Figure. The useMaidrController catch disposes the whole controller, which recovers eventually, but any synchronous code between the throw and the catch that reads this.context.figure (e.g. another observer firing) would see a dead object. Consider wrapping with try/catch and restoring to a "broken" sentinel state, or at minimum documenting this as a pre-condition for callers to handle.

---

3. Stale JSDoc block in context.ts

src/model/context.ts — two consecutive JSDoc blocks appear above isFigureLevel():

/**
 * Builds the plot context stack for a fresh figure, ...  ← intended for initializePlotContext
 * @returns The instruction context element
 */
/**
 * Whether navigation for this figure starts at figure level ...  ← the actual isFigureLevel doc
 */
private isFigureLevel(figure: Figure): boolean {

The first block was left over from initializePlotContext. This will confuse IDEs and doc generators.

---

Architecture / Design

4. liveDataManager singleton comment contradicts its own export

src/service/liveData.ts:2378

 * this instance, since state registered here is shared module-wide.
 */
export const liveDataManager = new LiveDataManager();

The JSDoc fragment (which appears to be a truncated "Do not export this instance…") contradicts the export. Either the comment is out of date or the intent was to keep the singleton internal. Either way, fix the comment.

---

5. ToggleMonitorCommand naming inconsistency

Other toggle commands omit the Command suffix (ToggleAudio, ToggleHighContrast, ToggleReview), but the new one is ToggleMonitorCommand. This is a style nit but creates inconsistency in the command module.

---

6. VIOLIN_KDE in NESTED_DATA_TYPES but documented as unsupported

src/service/liveData.ts

const NESTED_DATA_TYPES: ReadonlySet<TraceType> = new Set([
  ...
  TraceType.VIOLIN_KDE,  // ← structurally accepted but semantically wrong
]);

The comment and LIVE_DATA.md both note that appending raw observations to a violin doesn't recompute the distribution. But the code accepts it silently and merges the point. If this is intentional (advanced users who pre-compute KDE), make it explicit with a console.warn on append, mirroring how heatmap is rejected. If not, drop it from NESTED_DATA_TYPES.

---

7. No rate-limiting surface for high-frequency appends

Each appendData call triggers a full model rebuild (new Figure(maidr)). The docs note this scales with total chart size and recommend maxWidth, but there is no built-in debounce or throttle for callers who push faster than the model can rebuild. For a sensor stream at 60 Hz with 1 000 points, that is 60 full rebuilds per second. Consider documenting a recommended call pattern (e.g. batch N points before calling appendData, or use setData for bulk updates) more prominently in the API docs, or exposing a batchAppend variant.

---

Test Coverage

8. No test for the SSR/module-load scenario

There's no test verifying the window.maidrLive assignment is guarded against a missing window. This is the kind of bug that passes in Jest (jsdom provides window) but breaks in real SSR.

9. resolveId cross-test contamination via module singleton

test/service/liveData.test.ts constructs new LiveDataManager() instances directly, which avoids the singleton. Good. But src/index.tsx calls liveDataManager.appendData/setData via the module-level export. If any test imports src/index.tsx (or a bundle that includes it), the module-level state leaks across tests. Worth noting if integration tests are added later.

---

Minor

• src/model/candlestick.ts: Candlestick.getStateAt overrides AbstractTrace.getStateAt correctly, but the finally block only restores currentPointIndex/currentSegmentType, while the base class finally restores row/col/isInitialEntry. Since super.getStateAt(row, col) is called inside the Candlestick try block, the base-class finally runs before the subclass finally — this is correct, but reading the call stack inside out is non-obvious. A brief inline comment noting "base-class finally restores row/col; this finally restores the Candlestick-specific fields" would help reviewers.

• src/state/hook/useMaidrController.ts: The re-registration useEffect depends on [data.id, disposeController]. If disposeController (a useCallback) changes identity frequently, it re-registers the live listener on every render. Verify disposeController's dependency array is tight enough that its identity is stable.

• src/service/liveData.ts:appendPointToMaidr — layering: a service file imports CANDLESTICK_SECTIONS from @model/candlestick. Per MAIDR's MVVC, services can depend on models, so this is architecturally legal. But it does create a tight coupling between the generic append logic and one specific trace type. Exporting the constant to @type/grammar or a shared constants file would keep liveData.ts chart-type-agnostic.

---

Summary

Category	Count
Bugs / Correctness	3
Architecture / Design	4
Test coverage	2
Minor	4

The SSR window guard (#1) is the most urgent fix before merge. The zombie-figure edge case (#2) and the stale JSDoc (#3) are quick wins. Everything else is polish or forward-looking hardening.

Great work on the overall scope, test coverage, and documentation quality.

[jooyoungseo]

Eleventh round triaged — fixed in d1dca55/99240db:

Fixed

• feat: add new architecture #3 (stale JSDoc): genuine catch — the isFigureLevel extraction had orphaned initializePlotContext's doc block above the new helper. Reattached.
• Minor (candlestick finally ordering): added the inside-out restore-order note.
• Also caught and fixed a strict-mode type error in the new multilayer test data (CandlestickPoint requires trend/volatility).

Verified, no change

• placeholder #1/fix: notify user of end of plot #8 (SSR guard/test): fourth occurrence — src/index.tsx is the browser-only script bundle (touches document/window unconditionally at module load throughout); React/SSR consumers use maidr/react, which never imports it.
• feat: add new architecture #2 (zombie figure): third occurrence — the throw propagates synchronously to the hook's catch (no observers can fire in between on the same stack), which disposes the controller; documented at the recovery site.
• style: configure typescript-eslint #4 (singleton comment "truncated"): the full JSDoc reads coherently — "Tests should construct their own new LiveDataManager() instead of using this instance…"; the review snippet was cut mid-sentence by the reviewing tool, not the source.
• docs: add GitHub issue and PR templates #5 (naming): backwards — 16 toggle commands in toggle.ts use the Command suffix; ToggleHighContrast is the lone pre-existing exception.
• docs: add GitHub issue and PR templates #6 (VIOLIN_KDE), fix: clean up grammar to align with each plot requirement #7 (rate limiting), docs: add CONTRIBUTING.md #9 (singleton contamination), disposeController identity (stable: useCallback with empty deps): previously settled/documented; a batchAppend variant is reasonable follow-up territory if high-frequency multi-point producers appear.

Verification: 260 unit tests passing (3 pre-existing on main), type-check/lint clean.

---

Generated by Claude Code

[claude]

Code Review: feat: add realtime/streaming data support

Overview

This is a substantial, well-designed feature PR implementing live/streaming data support across three phases: full data replacement (setData), point-level streaming (appendData + LiveDataManager), and monitor mode (MonitorService). The architecture is thoughtful — rebuilding the model layer from data rather than mutating 15 trace classes in-place is the right call and results in clean, uniform behavior across all chart types.

Test coverage is excellent. 50+ TDD unit tests covering edge cases (sliding window, candlestick coordinate mapping, shape mismatches, observer cleanup) plus 17 E2E checks give strong confidence.

---

Architecture & Design

Strong compliance with MVVC:

• MonitorService is a proper service implementing the Disposable pattern
• ToggleMonitorCommand follows the Command pattern correctly
• Observer registration cleanup in updateData via registerObservers() prevents stale accumulations
• Context.replaceFigure is cleanly isolated and position-restoration logic is well-encapsulated

Concern — hotkeys.setScope in Context.replaceFigure (architectural boundary violation):

// src/model/context.ts
} else {
  this.scopeContext.clear();
  this._instructionContext = this.initializePlotContext(figure);
  hotkeys.setScope(this.scope);  // <- direct side-effect on keybinding state from model layer
}

Context lives in the model layer, which per the architecture should have no knowledge of services or keybinding infrastructure. The hotkeys.setScope call belongs in KeybindingService or the Controller, triggered by an event from Context. The test correctly mocks hotkeys-js to work around this, but the pattern sets a precedent for model->service coupling. Consider emitting a scoped event from replaceFigure and letting the Controller re-register the scope.

---

Potential Bugs / Risks

1. replaceFigure leaves a disposed figure on createFigure() throw (medium risk):

public replaceFigure(createFigure: () => Figure, options = {}): Figure {
  const snapshot = this.captureNavigationSnapshot();
  this.figure.dispose();       // <- old figure is gone

  const figure = createFigure(); // <- if this throws (e.g. bad maidr data)...
  this.figure = figure;          // <- never reached; this.figure now points to disposed object
  this.plotContext.clear();      // <- never reached
  ...
}

The caller in useMaidrController correctly catches this and calls disposeController(), but Context itself is left in an invalid state (disposed figure referenced). Any access to this.figure between the throw and the dispose would be undefined behavior. Consider wrapping the create in a try/catch inside replaceFigure itself to restore a minimal valid state before re-throwing, or add a null guard to subsequent this.figure reads.

2. AppendedPointInfo.layerId may silently be undefined at runtime:

// src/service/liveData.ts
appended: {
  ...
  layerId: layer.id,   // typed as string, but MaidrLayer.id may be optional
  ...
}

If MaidrLayer.id is an optional field (users can omit it on non-targeted layers), this casts undefined to string. Any downstream layerId comparison would silently fail. Add an explicit fallback: layerId: layer.id ?? '' and update the type, or assert that layers targeted by appendData must have an id.

3. The getStateAt reentrancy hazard is documented but needs a safety net:

// src/model/abstract.ts
// Re-entrancy hazard: this is only safe because the entire call chain is
// synchronous (no await points)...

The documentation is accurate and the current use is safe, but a future maintainer adding an async getter could silently break monitor mode. Consider adding a boolean guard that throws on re-entry, so the invariant is enforced rather than only documented.

---

Code Quality

Immutability in appendPointToMaidr is correct and well-implemented. Spread operators preserve all untouched structures and the original is never mutated. The test 'does not mutate the original maidr data' confirms this.

Lazy serialization in ChatService is a nice optimization — zero cost on streaming appends, one serialization per LLM request after a data change. The test for this pattern is a good addition.

The cloneMaidrData utility properly handles the structuredClone limitation with onNavigate callbacks. Using a single utility across both usages (useMaidrController, LiveDataManager) eliminates duplication.

Minor: resolveId uses a non-null assertion that could be more explicit:

// src/service/liveData.ts
return this.instances.keys().next().value!;

Since this.instances.size === 1 is checked immediately above, .value cannot be undefined. The ! is correct but could be replaced with a typed assertion or the check restructured for clarity.

---

Performance

• The documented per-update cost scaling with total chart size (all subplots + layers, not just the target layer) is an important caveat. The maxWidth guidance in the docs is appropriate.
• The lazy JSON serialization in ChatService avoids per-streaming-tick overhead on AI queries.
• For very high-frequency streams (> ~30 Hz), the full model rebuild per append could become a bottleneck. The docs correctly advise maxWidth as the mitigation. A throttle/debounce option could be a useful follow-up.

---

Test Coverage

The test suite is comprehensive. Highlights:

• contextReplaceFigure.test.ts covers shape mismatches, layer type changes, sliding-window shifts, multi-layer (candle+volume+MA), and observer cleanup.
• traceStateAt.test.ts covers the throw-path restore — exactly the defensive test needed for the reentrancy-sensitive getStateAt.
• The chat laziness tests spy on JSON.stringify directly — a reliable way to assert no-serialize behavior.

Gap: No test for replaceFigure with a createFigure that throws (the bug in item #1 above). A test asserting that disposeController is called on error in useMaidrController would close the loop on the recovery path.

---

Summary

Area	Status
Architecture / MVVC compliance	Good, with one noted boundary issue
Correctness	Two actionable bugs to address
Test coverage	Excellent
Documentation	Thorough
Performance	Appropriate with caveats documented
Security	No new attack surface beyond existing DOM exposure

The replaceFigure error-path issue (#1) is the most important to address before merge. The layerId type safety issue (#2) is a quick fix. The reentrancy guard (#3) is a defensive-programming improvement. The hotkeys.setScope architectural concern is lower priority but worth a follow-up issue.

Overall this is high-quality, well-tested code that delivers on the feature spec.

[claude]

Code Review: feat: add realtime/streaming data support

Overview

This PR implements realtime/streaming data support for MAIDR across three phases: full data replacement (setData), point-level streaming (appendData), and monitor mode (auto-sonification of newly streamed points). It's a large, well-scoped feature with strong documentation and test coverage. The opt-in design (live: true) is the right call — static charts are entirely unaffected.

---

Architecture: Strong

The layering is clean and follows MVVC:

• LiveDataManager lives in the service layer and routes updates to the Controller
• MonitorService delegates directly to AudioService/TextService (existing observers) rather than duplicating logic
• Controller.updateData() orchestrates the model rebuild; services, viewmodels, and keybindings are untouched
• AbstractTrace.getStateAt() is the only model-layer addition — well-encapsulated and minimal

The immutable-append pattern in appendPointToMaidr (spreading unchanged structures, only creating new objects along the mutated path) is the right approach for a library that hands data to a model layer that takes ownership.

---

Issues to Address

Medium: getStateAt re-entrancy hazard (documented but fragile)

The implementation temporarily mutates this.row/this.col/this.isInitialEntry and restores via finally. The PR comment itself flags this:

Re-entrancy hazard: this is only safe because the entire call chain is synchronous. If a getter ever becomes async or triggers notifications, callers could observe the temporary cursor.

Consider adding a boolean guard (private _computingStateAt = false) that prevents notifyStateUpdate() from firing during computation, rather than relying solely on the call-chain remaining synchronous:

public getStateAt(row: number, col: number): TraceState {
  const prev = { row: this.row, col: this.col, isInitialEntry: this.isInitialEntry };
  this._computingStateAt = true;
  try {
    this.isInitialEntry = false;
    this.row = row;
    this.col = col;
    return this.state;
  } finally {
    this._computingStateAt = false;
    this.row = prev.row;
    this.col = prev.col;
    this.isInitialEntry = prev.isInitialEntry;
  }
}

// Guard in notifyStateUpdate()
notifyStateUpdate(): void {
  if (this._computingStateAt) return;
  // ...existing logic
}

This makes the safety guarantee structural rather than documentary.

Medium: live-coinbase.html uses a real external API

examples/live-coinbase.html (present in the diff but not mentioned in the PR description) connects to the live Coinbase WebSocket API. This creates an undocumented external dependency that will silently break if Coinbase changes their feed format or the connection is blocked. Please:

1. Add it to the PR description
2. Add a disclaimer comment in the file noting it requires a live Coinbase connection
3. Consider whether a real-money exchange API example belongs in the library's examples directory

Low: Non-null assertion in resolveId

// src/service/liveData.ts
return this.instances.keys().next().value!;

The ! is safe because size === 1 is checked above, but Map's .keys().next().value returns string | undefined and the assertion silently lies if the code is refactored. A safer form:

const [id] = this.instances.keys();
return id ?? null;

Low: Module-level singleton and SSR

The exported liveDataManager is a module-level singleton. The PR documents that tests should use new LiveDataManager() — correct. However, in SSR environments (Next.js, Remix) this singleton persists across requests. A brief note in the JSDoc would help future maintainers:

/**
 * ...
 * @remarks Module-level singleton. SSR environments should not import this
 * module server-side; use the React prop API instead.
 */
export const liveDataManager = new LiveDataManager();

---

Observations Worth Calling Out

Depth-1 stack restoration — The restoreNavigation default case handles the depth-1 ambiguity (figure level vs. trace level for single-subplot charts) correctly. However, there is no test covering a multi-subplot figure where the user is at figure-level scope (depth === 1, isFigureLevel === true). The existing test suite is comprehensive but this specific path appears untested.

AppendedPointInfo JSDoc typo — "are announce coordinates" (in the nested field doc) should read "are navigation coordinates" or similar.

Performance note missing from top-level API — The AppendDataOptions JSDoc warns that per-update cost scales with total chart size and recommends maxWidth. This guidance is good but only visible on the options interface, not on appendMaidrData() or MaidrLiveApi itself. Script-tag consumers won't see it.

---

What's Working Well

• Test coverage: 50+ unit tests written red-first plus 17 Playwright E2E checks is excellent. The contextReplaceFigure.test.ts and traceStateAt.test.ts suites are thorough.
• Lazy serialization for ChatService: The getDataJson() cache avoids paying JSON.stringify on every streaming append while ensuring AI answers reflect current data. Clean refactor.
• Disposal chain: Subplot.dispose() now removes the cloned highlight SVG element, preventing DOM node accumulation on live updates. Good catch.
• Shape-signature check: Using trace types (not group counts) in describeShape() is the right invariant — series counts can grow without resetting navigation, while type changes correctly reset it.
• FormatterService.refresh() and HighContrastService.setFigure(): Both are clean, minimal additions that re-initialize service state from the new model without re-creating the service.
• Documentation: docs/LIVE_DATA.md with full API reference, React examples, candlestick section, and performance guidance is thorough and will significantly reduce support questions.

---

Summary

The design is solid and the implementation follows project conventions well. The two medium-priority issues are worth addressing before merge: the getStateAt guard (protects against a class of future observer-firing bugs) and the coinbase example (external API dependency needs at minimum a clear disclaimer). Everything else is minor polish.

[jooyoungseo]

Thirteenth round addressed in 0761eba:

Fixed

• getStateAt guard (raised across five rounds; this round's structural proposal sealed it): notifyStateUpdate() now throws if fired while getStateAt computes state at a temporary cursor, making the side-effect-free contract structural rather than documentary — a future getter that notifies observers fails loudly in the first test run instead of silently corrupting the cursor. Chose throw over the suggested silent return so the regression is visible, not masked. Unit-tested (guard fires, cursor restored, observers untouched).
• Depth-1 figure-level test gap: valid catch — added a test pinning multi-panel figure-position restoration at depth 1.
• Performance note visibility: the maxWidth guidance now also appears on appendMaidrData() and MaidrLiveApi.appendData JSDoc, where script-tag/npm consumers actually look.
• PR description: updated to include examples/live-coinbase.html.

No change, with reasons

• Coinbase example disclaimers: already present — the file header states it needs network access and points to the offline simulated demo, and the page shows live connection status with an offline fallback message. The example exists at the repo owner's request (issue feat: add realtime/streaming data support #536's motivating use case is stock tickers) and uses a keyless public market-data feed precisely because equity APIs require keys an example can't ship; whether it stays is the maintainers' call.
• resolveId: this single line has now received a fifth distinct style demand across rounds (as string → ! → Array.from()[0] → as string → destructure-with-??). Keeping the round-3 form.
• "announce coordinates" typo: not a typo — for flat layers those coordinates exist specifically for monitor announcements (e.g. the candle's close section).
• SSR @remarks on the singleton: liveData.ts has no DOM access, and registration only happens in a client-side useEffect, so server-side import is inert; the existing JSDoc covers the real (test-isolation) caveat.

Verification: 262 unit tests passing (3 pre-existing on main), type-check/lint clean, 20/20 offline Playwright checks against the rebuilt bundle.

---

Generated by Claude Code

[claude]

PR review posted via Claude Code review agent - see detailed analysis below.

[claude]

Code Review: feat: add realtime/streaming data support

Overview

This PR implements realtime/streaming data support across three phases: full data replacement (setData), point streaming (appendData), and monitor mode (auto-sonification via M key). The design rationale of rebuilding the model through existing constructors rather than making all 15 trace classes mutable is sound and avoids a large regression surface. The test suite is thorough (50+ unit tests, 17 E2E checks) and the documentation is excellent.

---

Architecture and MVVC Compliance ✅

The implementation fits cleanly into the existing architecture:

• LiveDataManager acts as a singleton registry routing updates to mounted chart instances
• MonitorService follows the Disposable pattern and is wired through the standard Command pattern (ToggleMonitorCommand)
• Context.replaceFigure handles position preservation at the right abstraction level
• AbstractTrace.getStateAt computes state without moving the cursor — the finally-block restore pattern is correct and well-documented

The conversion of private readonly figure to private figure in both Controller and Context is documented with a clear comment.

---

Potential Bugs

1. Type mismatch: layerId may be undefined

In src/service/liveData.ts, AppendedPointInfo.layerId is typed as string, but MaidrLayer.id is string | undefined:

• AppendedPointInfo has: layerId: string (typed as required string)
• But it is assigned from: layerId: layer.id (where MaidrLayer.id is string | undefined)

If a layer has no id, this silently assigns undefined to a string field. Suggested fix: layerId: layer.id ?? ''

2. Candlestick.getStateAt — pre-super code runs without isComputingStateAt guard

isComputingStateAt is set inside super.getStateAt(), but computeIndexAndSegment runs before the super call. If computeIndexAndSegment or a subclass override ever called notifyStateUpdate(), the guard would miss it. A defensive fix is to set the flag at the top of the override before any side-effecting code runs.

3. ws.onmessage without try/catch in the Coinbase example

The WebSocket feed can send protocol-level messages (e.g., subscriptions confirmation) whose shape does not match the expected ticker format. JSON.parse(e.data) is unguarded and would throw on malformed input. Wrapping with try/catch or an explicit type-check guard would make the demo more robust.

---

Design Observations

4. Dual-ref tracking in useMaidrController is complex

The hook now uses both latestDataRef and previousDataRef to track the data prop, alongside updates flowing from liveDataManager. latestDataRef is updated by both the registration effect AND the data-change effect, while previousDataRef is only updated in the data-change effect. A brief inline comment explaining why both refs are needed (and how they differ) would help future maintainers understand the ownership chain.

5. No built-in rate limiting for high-frequency streams

Each appendData call rebuilds the entire chart model (all subplots and layers). At greater than 10 Hz on a multi-layer chart this can become a real bottleneck. This is documented correctly, but a follow-up adding a minimum update interval or batching option to the API would be valuable.

6. isComputingStateAt is protected — accessible to subclasses

Since this field is an invariant enforced by notifyStateUpdate() and should only be written by getStateAt, marking it protected lets subclasses accidentally set it to the wrong value. A private field with a protected getter for the guard check would be safer.

---

Minor Issues

7. resolveId non-null assertion (instances.keys().next().value!) after a size === 1 guard is safe but unnecessary noise.

8. previousDataRef initialization could miss the first real prop change in concurrent React rendering modes, though this is unlikely in practice given the registration effect ordering.

---

Test Coverage ✅

Test coverage is excellent:

• contextReplaceFigure.test.ts — covers same-shape, shorter-data, sliding-window shift, layer-type change, multi-panel, and observer cleanup scenarios
• traceStateAt.test.ts — covers cursor preservation, throw-path restore, and observer non-notification
• candlestickStateAt.test.ts — covers candle-specific index mapping and cursor restore
• liveData.test.ts — covers data merging, sliding window, and manager registry
• monitor.test.ts — covers toggle behavior and live/non-live gating
• chat.test.ts — covers lazy serialization and cache invalidation

The test for notifyStateUpdate firing inside getStateAt (verifying the guard throws loudly) is a great defensive addition.

---

Documentation ✅

docs/LIVE_DATA.md is comprehensive and well-organized. The candlestick streaming section and the Notes for real feeds callouts are particularly useful. The React doc updates and inline API examples in docs/react.md are clean and accurate.

---

Summary

Category	Rating
Architecture fit	Excellent — fits MVVC cleanly
Code correctness	One type mismatch (layerId), one minor guard gap (candlestick)
Test coverage	Thorough — 50+ unit + 17 E2E
Documentation	Excellent
Performance	Rebuild-per-append documented; rate limiting is a future concern
Security	No new vectors introduced

The layerId type mismatch (string vs string|undefined) is the only issue worth fixing before merge. The rest are minor polish or future concerns. Overall this is a well-designed, well-tested addition to MAIDR.

[claude]

(Deleting the placeholder comment posted earlier — the full review is in the comment above.)

[jooyoungseo]

Fourteenth round addressed in d888328:

Fixed

• feat: add new architecture #2 (pre-super guard window): valid refinement of the new structural guard — Candlestick.getStateAt now raises isComputingStateAt before its pre-super section/index mapping and clears it in its own finally, so the whole override is covered including pre-super throws.
• feat: add new architecture #3 (Coinbase demo JSON.parse): wrapped defensively; non-JSON frames are ignored.

No change, with reasons

• placeholder #1 (layerId type): ninth occurrence. MaidrLayer.id is required (id: string, grammar.ts:403).
• docs: add GitHub issue and PR templates #6 (isComputingStateAt should be private): directly contradicts this same review's feat: add new architecture #2, which requires the Candlestick subclass to write the flag. It stays protected.
• style: configure typescript-eslint #4 (dual-ref comment): already present — the "Data ownership chain" comment in useMaidrController (added in an earlier round) explains exactly who writes latestDataRef and when.
• docs: add GitHub issue and PR templates #5 (rate limiting), fix: clean up grammar to align with each plot requirement #7 (resolveId, sixth style demand), fix: notify user of end of plot #8 (concurrent-mode speculation): previously settled / follow-up territory.

Verification: 262 unit tests passing (3 pre-existing on main), type-check/lint clean, 20/20 offline Playwright checks against the rebuilt bundle.

---

Generated by Claude Code

[claude]

Code Review: feat: add realtime/streaming data support

Overview

This is a substantial, well-designed PR that adds realtime/streaming data support to MAIDR in three phases: full data replacement (setData), point-by-point streaming (appendData), and a monitor mode that auto-sonifies new points without moving the user cursor. The approach of rebuilding the model layer on each update and restoring navigation positions is the right call vs. making all 15 trace classes mutable.

---

Positives

• Correctness by construction. Rebuilding from tested constructors and restoring navigation state externally at the Context level is far safer than invasive cache invalidation across all trace types.
• TDD discipline. The 60+ new unit tests cover meaningful edge cases: cursor preservation with sliding-window trim, shape-change reset, candlestick section mapping, no-notify guard, throw-path restore.
• Accessibility-first design. Monitor mode is consistent with MAIDR's core mission. Auto-sonifying without cursor movement is the right model, and the M-key scope registration (TRACE, BRAILLE, SUBPLOT) covers all places a user might be.
• No regressions. Static charts are completely unaffected; live features are opt-in via live: true.

---

Issues

Bug: Missing null-check for optional layer.id in AppendedPointInfo

src/service/liveData.ts in the appendPointToMaidr return block:

layerId: layer.id,

layer.id is typed as optional (MaidrLayer.id?: string). If a layer without an explicit id is appended to, this field will be undefined at runtime even though AppendedPointInfo.layerId is typed as string. Consider falling back to: layer.id ?? `layer-${layerIndex}`

SSR / module-evaluation guard missing on window.maidrLive

src/index.tsx runs this at module evaluation time:

if (window.maidrLive) { ... }
window.maidrLive = { ... };

If the bundle is imported in an SSR or test environment where window is not defined, this throws. A simple guard is worth adding:

if (typeof window !== 'undefined') {
  window.maidrLive = { ... };
}

Warning message uses -1 as layer id when layerId lookup fails

In src/service/liveData.ts, when layerId is provided but not found, findIndex returns -1, and the subsequent warning prints "no layer for index -1" instead of the id. It should read no layer for id "${options.layerId}" in that case.

Re-entrancy hazard documented but not self-enforcing

The comment in abstract.ts correctly notes the synchronicity assumption for getStateAt, but the isComputingStateAt flag could also block re-entrant calls to getStateAt itself (throw or bail early) so the invariant is enforced by code, not documentation alone.

Structural fragility: Subplot.activeLayerIndex relies on a layout invariant

The accessor returns this.row relying on the traces[layerIndex][0] layout. The comment documents this correctly, but it breaks silently if the Subplot layout ever changes. Consider adding a dev assertion: console.assert(this.traces[this.row]?.length === 1, 'activeLayerIndex invariant violated').

---

Test Coverage Gaps

1. MonitorService.setLive(false) while monitoring is enabled is not covered. The path where setLive(false) forces enabled = false is a correctness guarantee worth testing.

2. LiveDataManager.register guard for same-id re-registration. The comment notes "guard against a newer registration for the same id" but the test suite does not exercise this path.

3. React integration for the two-effect interaction. The useMaidrController live data flow (prop changes routing through liveDataManager.setData vs. updateStoredData) is not covered by unit or hook tests. An integration test verifying prop changes on live charts call controller.updateData (and non-live charts don't) would prevent regressions.

---

Code Quality

• Architecture is clean and follows MVVC conventions throughout.
• Observer wiring and disposal are correct; the old figure is disposed before the new one is created, preventing stale highlight-clone accumulation.
• cloneMaidrData centralizing the structuredClone + onNavigate re-attach pattern is a good extraction.
• The candlestick getStateAt override handles the dual-finally cleanup carefully and is well-commented.
• Documentation (LIVE_DATA.md, SCHEMA.md additions, react.md) is thorough and accurate.
• Examples are genuinely useful, especially live-coinbase.html which connects to a real feed.

---

Summary

High-quality, well-tested work that adds a significant capability while respecting the existing architecture. The identified issues are mostly minor. The layer.id null-check and the SSR window guard are the most concrete correctness fixes worth addressing before merge; the others can be follow-ups.

[jooyoungseo]

Fifteenth round — one valid item, taken in c3a4192:

Fixed

• Test gap feat: add new architecture #2 (registration dispose guard): genuine — added a test pinning that disposing a stale registration does not evict a newer one for the same chart id (the listener-identity guard).

Verified false / already covered

• layerId optionality: tenth occurrence. MaidrLayer.id is required (id: string, grammar.ts:403), not id?: string as claimed.
• Warning prints "index -1": false — the message is options.layerId ?? \index ${layerIndex}`, so when a layerIdlookup fails it prints the id, never-1`; the unknown-layerId unit test exercises exactly this path.
• Test gap placeholder #1 (setLive(false) while enabled): already covered — monitor.test.ts has had "setLive(false) turns monitoring off" since the second review round.
• SSR guard (fifth occurrence), re-entrant getStateAt (nested calls are safe by stack discipline of save/restore, and the no-notify invariant is now code-enforced), activeLayerIndex assertion (invariant holds by construction), React hook integration test (no jsdom/RTL in toolchain; covered by the 20-check Playwright suite): previously settled.

Verification: 263 unit tests passing (3 pre-existing on main), lint clean.

---

Generated by Claude Code

[claude]

test comment from claude bot - please ignore

[claude]

Contributing to maidr

Thank you for your interest in contributing to maidr project! We welcome contributions from everyone.

Pre-requisites

To contribute to this project, you need to have the following installed on your local machine:

1. git

2. node.js (version 20 or higher is recommended)

3. npm (version 10 or higher is recommended)

4. Visual Studio Code (VS Code) (This is the recommended IDE)

5. Docker Desktop (For leveraging the dev container within the project)

Getting Started

Installing Node

Check if node is available and above version 20 or higher on your system using the following command:

node -v

This command should display the version of node installed on your system. In case this command does not provide the expected version number, please download node from here, depending on your development environment.

Installing NPM

Check if NPM is available on your system and above version 10 using the following command:

npm -v

This command should display the version of node installed on your system. In case this command does not provide the expected version number, please download npm from here, depending on your development environment.

Setting up the Repository:

To locally set up the repository, please follow these steps in order:

1. Fork the repository on GitHub.

2. Clone your forked repository to your local machine.

3. Open the project in VS Code.

4. Install all the recommended VSCode extensions.

Setting up the Development environment:

Once the above steps are complete, we set up the development environment.

The repository comes with the ability to spawn a dev container so that you get all necessary dependancies for the project in an isolated development environment exclusive to maidr and devoid of any interference from local environment configuration.

Docker Desktop that was installed as part of the pre-requisites will be used for accessing the spawned dev container.

The Dev Container can be used via Visual Studio Code or Github Codespaces

To enable this dev container and use it for project development, please follow the steps in order:

Accessing Dev Container through Visual Studio Code

1. Use Ctrl/Cmd + Shift + P to invoke the VS Code command palette.

2. In the search box, type in > Dev Containers: Rebuild and Reopen in Container . This option will pick up the dockerfile configuration available in the directory at the time and spawn a new dev container.

3. VS Code takes a few minutes to prepare the dev container with all required dependancies. Once the dev container is successfully spawned, it can be used to work on development activities specific to the maidr repository.

   NOTE: Rebuild process is only required when the dev container is being spawned for the first time or when there are changes to the dockerfile configuration. Subsequent access to the dev container can be done by directly selecting the option > Dev Containers: Reopen in Container from the command palette.

Accessing Dev Container through Github Codespaces

1. Use Ctrl/Cmd + Shift + P to invoke the VS Code command palette.

2. In the search box, type in > Codespaces: Create New Codespace. This option will pick up the dockerfile configuration available in the directory at the time and spawn a new dev container.

   NOTE: We will periodically update the configuration of the dev container with any dependancies that are added in the future. In case there are any issues in spawning the dev container or working in that environment, please raise an issue with DevOps : <Issue Description> as the title.

Publishing Changes to Forked Repository

As contributors, you can publish changes for to your forked repository through the following procedure:

1. All changes should be in a dedicated branch on your local forked repository pertaining to the feature or bug you are working on. The naming of the branch should be as relevant as possible so that maintainers have a decent context on what it relates to.

2. Execute npm run build to evaluate if the changes do not break the sanity of the build.

3. Use git add to include all changes that are affected by your changes.

4. Use git commit -m <commit-message> to commit your changes. (please read Committing Your Changes section to learn more about the conventional commits).

5. The lint checker will run after git commit is executed. To address any lint issues thrown after this point, execute npm run lint:fix to address all lint issues detected in one go and attempt to commit the changes again. (There could be certain lint issues that may not be resolved even after automatic addressing. Such issues need to be resolved manually.)

6. Once the local commit is successful, utilize git push to publish changes to your forked repository.

Publishing Changes for Review

Once the changes and concerned branch are updated in your remote forked repository, you can prepare them for review by maintainers by the following procedure:

1. Create a PR from the relevant branch on your remote forked repository to the main branch of the maidr repository.

2. Provide all information pertaining to the changes in the PR and be as detailed as possible. This helps the reviewers to validate the changes and provide constructive feedback.

3. If and when feedback is received, please address them as promptly as possible for efficient integration.

Guidelines

Please follow these guidelines when contributing to the project:

• Use TypeScript (es6) for all code.
• Write clear and concise commit messages.
• Follow the code style and formatting guidelines.
• Write tests for new features and bug fixes.

Code Style and Formatting

We use ESLint to enforce a consistent code style and formatting. Please run npm run lint to check for lint errors and address them using npm run lint:fix (Even after auto fixing the lint errors, there may be a few lint errors that will require manual inspection)

Tests

We use Playwright for end-to-end testing. Please refer to the documentation if you are new to Playwright.

1. Use npm run e2e to run the end-to-end tests
2. Use npm run e2e:ui to run tests with UI mode
3. Use npm run e2e:debug to debug tests

Committing Your Changes

We use conventional commits to maintain a clear and consistent commit history. Here's how to write a conventional commit message:

Format

Each commit message should follow this format: <type>[optional scope]: <description>

• Types: Describes the type of change you're making. Common types include:
  • feat: Introduces a new feature.
  • fix: Fixes a bug.
  • docs: Documentation changes.
  • style: Code style changes (formatting, missing semi-colons, etc. – does not affect code logic).
  • refactor: Code changes that neither fixes a bug nor adds a feature.
  • perf: Performance improvements.
  • test: Adding or correcting tests.
  • chore: Routine tasks, maintenance, and other non-production changes.
  • ci: Changes to our CI configuration files and scripts, specifically for GitHub Actions.
• Scope: (Optional) A word or two describing where the change happens, like login, signup, etc.
• Description: A succinct description of the change in lowercase.

Denoting a Breaking Change

To denote a breaking change, include an exclamation mark ! before the colon.

• Example: feat(database)!: remove deprecated methods

Mentioning Issue Numbers

If your commit addresses a specific issue, mention the issue number at the end of the commit message.

• Example: fix(file-upload): correct MIME type handling, closes #123

Examples

• feat(authentication): add JWT token support
• fix(api): resolve data retrieval issue
• docs(readme): update installation instructions
• style(header): adjust layout spacing
• refactor(user-profile): streamline user data processing
• perf(image-loading): optimize image caching
• chore(dependencies): update lodash to 4.17.21
• test(login): add additional unit tests for password reset
• ci(github-actions): update build and deployment workflow

Commit Message Linting

When you commit your changes, Husky and Commitlint will automatically check your commit messages. If your message does not meet the conventional commit format, the commit will be rejected, and you'll need to modify the message.

Maintaining the website

Here's how to update the MAIDR documentation:

1. Update Source Code Documentation (if needed)

• Edit JSDoc comments in TypeScript files (src/*/.ts)
• These comments will be extracted by TypeDoc for the API Reference

2. Update Content Pages (if needed)

Edit these files for different sections:

• README.md - Main landing page content (converted to index.html)
• docs/template.html - HTML template for generated pages
• examples/ - Example files (embedded in examples.html)

3. Build Documentation

npm run docs

This runs the build script which:

• Converts README.md to index.html
• Generates examples.html
• Copies media and examples folders
• Runs TypeDoc to generate API documentation in _site/api/

4. Preview Locally

npm run docs:serve

This builds the docs and starts a local HTTP server. Alternatively, open _site/index.html directly in your browser.

5. Deploy to GitHub Pages

The documentation will automatically deploy when you push to:

• main branch
• docs/jsdoc branch (for testing)

React Component Development

MAIDR exposes a reusable <Maidr> React component via the maidr/react export. Here's what you need to know when working on it:

Build Commands

npm run build          # Builds both vanilla JS bundle and React library
npm run build:script   # Builds only the vanilla JS bundle (dist/maidr.js)
npm run build:react    # Builds only the React library (dist/react.mjs + dist/react.d.mts)

The React build uses a separate Vite config (vite.react.config.ts) and TypeScript config (tsconfig.build.json).

Key Files

File	Purpose
src/maidr-component.tsx	The <Maidr> React component
src/react-entry.ts	Public API barrel export for maidr/react
src/state/hook/useMaidrController.ts	Controller lifecycle hook (focus/blur, create/dispose)
src/state/context.ts	React Context for per-instance dependency injection
src/state/store.ts	Redux store factory (createMaidrStore())
vite.react.config.ts	Vite build config for React library
tsconfig.build.json	TypeScript config for declaration generation

Testing with the Test App

A local test app exists at test-react-app/ (git-ignored) for verifying the React component:

# 1. Build the maidr library first
npm run build

# 2. If the test app doesn't exist yet, create it:
npm create vite@latest test-react-app -- --template react-ts
cd test-react-app
npm install
npm install ..   # Install local maidr as a dependency

# 3. If the test app already exists:
cd test-react-app
npm install      # Re-links the local maidr package

# 4. Run the test app
npm run dev

The test app imports Maidr from maidr/react and wraps a bar chart SVG, allowing you to verify focus/blur lifecycle, audio sonification, text descriptions, keyboard navigation, and visual highlighting.

Architecture Notes

• Each <Maidr> instance creates its own isolated Redux store (no global singletons)
• The Controller is created on focus-in and disposed on focus-out
• React Context (MaidrContext) provides per-instance viewModelRegistry and commandExecutor
• React and ReactDOM are peer dependencies (not bundled into the library)

Code of Conduct

Please note that this project is released with a Contributor Code of Conduct. By participating in this project you agree to abide by its terms.

[claude]

Line 1
Line 2 no hash
Line 3

[claude]

Code Review: feat: add realtime/streaming data support (#620)

Summary: This is a substantial and well-architected feature adding realtime/streaming data support across three phases: full data replacement (setData), point-level streaming (appendData), and monitor mode (M key). The implementation follows the existing MVVC patterns well and includes strong test coverage (60+ unit tests, 20 E2E tests).

---

Architecture & Design

Strengths:

• The "rebuild model, restore position" approach in Context.replaceFigure is elegant — it reuses every existing trace constructor and observer pathway without mutating the 15 trace classes. The shape-signature check before restoring navigation cleanly handles structural incompatibilities.
• MonitorService is correctly decoupled — it delegates to AudioService/TextService via their Observer<TraceState> interface, so monitor announcements flow through the same pipeline as normal navigation without bypassing middleware.
• LiveDataManager as a singleton registry is the right call for script-tag consumers. The disposal guard correctly handles same-id re-registrations without leaking.
• The isComputingStateAt guard that throws from notifyStateUpdate() during getStateAt is strong structural enforcement — future contributors cannot accidentally introduce side-effectful state getters without hitting a loud runtime error.

Concerns:

1. Type safety gap — AppendedPointInfo.layerId typed as string but assigned from layer.id

In appendPointToMaidr (liveData.ts), the return value sets layerId: layer.id. However, MaidrLayer.id is optional (id?: string), so this assignment is string | undefined going into a field typed as string. Depending on strict settings TypeScript may not catch this, but it is a latent bug: announceAppendedPoint in Controller and any downstream consumer reading appended.layerId as a string could silently get undefined when layers have no id.

Suggestion: Either type AppendedPointInfo.layerId as string | undefined, or add a guard at the assignment site: layerId: layer.id ?? ''.

2. getStateAt temporary cursor mutation — synchrony assumption is fragile

The approach works today and the re-entrancy caveat is documented. A future async path in any state getter would silently violate the invariant (that row/col are restored before any other code observes them) without any compile-time warning.

Suggestion: Acceptable for now. As a belt-and-suspenders measure, consider asserting !this.isComputingStateAt at the entry of moveOnce and other cursor-mutating methods — that would surface an async violation immediately rather than silently corrupting state.

3. Candlestick.getStateAt — double isComputingStateAt management

The override sets this.isComputingStateAt = true before calling super.getStateAt, which also sets it. Both finally blocks lower the flag (parent's first, child's redundantly). Dual-ownership of the guard makes the invariant harder to reason about even with the explanatory comment.

Suggestion: A protected withStateAt(setup: () => void): TraceState template-method on AbstractTrace that calls the setup callback, then runs the base state logic, would eliminate the need for the override to touch isComputingStateAt at all.

---

Performance

• Per-append full model rebuild is clearly documented and mitigated by maxWidth. The cost is proportional to the entire figure (all subplots/layers), not just the updated layer — this could surprise users who add many inactive layers to a live chart. The Behavior Details section in the docs mentions this.
• structuredClone on every updateData call — fine for human-speed streams; worth a docs note if anyone tries sub-second update rates on large multi-layer charts.
• Lazy JSON serialization in ChatService — the cachedJson cache correctly avoids JSON.stringify cost on each streaming tick. Nice.

---

Security

• window.maidrLive set unconditionally at module load time — in SSR or non-browser environments (typeof window === 'undefined'), this throws a ReferenceError.

  Suggestion: Wrap in typeof window !== 'undefined' before the assignment in index.tsx.

• No validation on appendData point shape — a badly-shaped point from an external WebSocket feed (e.g. { y: NaN }) could produce NaN in audio frequency calculations or garbage text. This matches the existing static API behavior so it is not a regression — worth noting for future hardening.

---

Test Coverage

Good overall — unit tests cover key edge cases (empty layer, new series, sliding window, shape changes, type changes). A few gaps worth considering:

• Candlestick.getStateAt field restoration on throw — tests cover the happy path and the base-class no-notify guard. A test verifying currentPointIndex/currentSegmentType are restored after a throw inside computeIndexAndSegment would close the loop on the double-finally concern.
• Monitor key in SUBPLOT scope on non-live chart — TOGGLE_MONITOR is wired into BRAILLE, SUBPLOT, and TRACE keymaps. A test confirming that pressing M in SUBPLOT scope on a non-live chart shows the explanatory notification (rather than silently failing) would complete coverage for that branch.
• React hook effect ordering — the two useEffect calls in useMaidrController have an ordering dependency. A unit test verifying the correct path is taken when data.id changes simultaneously with data content would catch ordering regressions.

---

Minor Nits

• ToggleMonitorCommand uses @param {MonitorService} JSDoc style while neighboring commands in toggle.ts use inline TypeScript — small style inconsistency.
• The Coinbase example hardcodes wss://ws-feed.exchange.coinbase.com — a comment noting geo-blocking in some regions would help developers debugging a non-working demo.
• liveDataManager.updateStoredData is public but only called from the React hook. Marking it @internal would keep the public API surface cleaner.

---

Summary

This is production-quality work. Architecture is clean, observer/command integration is correct, and test coverage is solid. The main actionable items before merge:

1. Fix AppendedPointInfo.layerId type (string → string | undefined, or assert non-undefined at the assignment site) — type correctness bug.
2. Add an SSR guard around window.maidrLive = ... in index.tsx — prevents crashes in server-side/Node environments.
3. (Optional) Add a test for Candlestick.getStateAt field restoration on throw.

The rest are suggestions, not blockers.

[xabilitylab]

🎉 This PR is included in version 3.70.0 🎉

The release is available on GitHub release

Your semantic-release bot 📦🚀