Skip to content

feat: add realtime/streaming data support#620

Open
jooyoungseo wants to merge 4 commits into
mainfrom
claude/brave-einstein-exasrq
Open

feat: add realtime/streaming data support#620
jooyoungseo wants to merge 4 commits into
mainfrom
claude/brave-einstein-exasrq

Conversation

@jooyoungseo

Copy link
Copy Markdown
Member

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. 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; 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
  • Axis ranges, navigation graphs, braille, and text recompute naturally since the model is rebuilt from the merged data

Phase 3 — Monitor mode

  • New Maidr.live: true opt-in flag
  • Pressing M (TRACE and BRAILLE 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
  • Wired through the standard Command pattern (ToggleMonitorCommand, CommandContext, keymaps)

AI chat freshness

ChatService.updateData() refreshes the serialized chart data held by each LLM model on every live update, so AI answers reflect the data currently on screen.

Documentation

  • New user manual: docs/LIVE_DATA.md (Live & Streaming Data) with full API reference, script-tag and React examples, monitor mode, sliding window, 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 demo: examples/live-line.html

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: 36 new unit tests (written red-first) — test/service/liveData.test.ts (data merging, sliding window, manager registry), test/service/monitor.test.ts, test/model/contextReplaceFigure.test.ts (position preservation/clamping/shape changes), test/service/chat.test.ts
  • Full Jest suite: 236 passing; the 3 failures in display.test.ts/braille-escape.test.ts pre-exist on main
  • End-to-end (Playwright, real browser, production bundle): 11/11 checks — API exposure, navigation, append + reachability, monitor on/off announcements, auto-announcement of appended points, cursor preservation across appends, setData with position clamping, and the maxWidth sliding window
  • tsc --noEmit, repo-wide ESLint, full 12-bundle production build, and the docs site build all pass

https://claude.ai/code/session_01KuiTyhDAEWtQX9oTKGatzN


Generated by Claude Code

claude added 2 commits June 10, 2026 23:01
Implements live data updates for accessible visualizations (closes #536):

- setData: full data replacement via window.maidrLive.setData() (script
  tag), setMaidrData() (npm), or the data prop on live React charts. The
  Controller rebuilds the model layer in place (Context.replaceFigure),
  preserving the user's navigation position with clamping, and rewires
  observers without touching services, view models, or keybindings.
- appendData: streaming point appends via window.maidrLive.appendData()
  / appendMaidrData(), with immutable data merging (flat and nested
  group layers), targeting by subplot/layerId/groupIndex, and a maxWidth
  sliding window that trims the oldest points while keeping the cursor
  on the same data point.
- Monitor mode: 'M' key (TRACE and BRAILLE scopes) toggles monitoring on
  charts configured with live: true; newly appended points are
  auto-sonified and announced to screen readers without moving the
  user's position.

New grammar options: Maidr.live (opt-in) and Maidr.maxWidth. Static
charts are unaffected. Includes a LiveDataManager registry routing
updates to mounted chart instances, a MonitorService, unit tests for
data merging, monitoring, and figure replacement, and a runnable
streaming demo (examples/live-line.html).

https://claude.ai/code/session_01KuiTyhDAEWtQX9oTKGatzN
… live updates

- Add docs/LIVE_DATA.md user manual covering setData, appendData,
  the maxWidth sliding window, monitor mode, and complete script-tag
  and React examples; register the page in the docs site build.
- Cross-link live data docs from README, SCHEMA.md (live/maxWidth
  top-level properties), CONTROLS.md (Monitor Mode key), react.md
  (Live & Streaming Data section with examples), and llms.txt.
- Fix AI chat staleness on live charts: ChatService.updateData()
  refreshes the serialized chart data held by each LLM model, and the
  Controller invokes it on every live data update so AI answers
  reflect the data currently on screen. Covered by unit tests.

https://claude.ai/code/session_01KuiTyhDAEWtQX9oTKGatzN
Copilot AI review requested due to automatic review settings June 11, 2026 00:17
@claude

claude Bot commented Jun 11, 2026

Copy link
Copy Markdown

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.tsappendPointToMaidr:

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 AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Comment thread src/service/liveData.ts Outdated
Comment thread src/state/hook/useMaidrController.ts Outdated
Comment thread test/service/liveData.test.ts
Comment thread src/service/chat.ts
- Detect nested-data layers by trace type so appending into an initially
  empty layer (e.g. line with data: []) produces the correct
  LiveDataPoint[][] shape; a groupIndex equal to the current group count
  now starts a new series (covered by new unit and e2e tests).
- Gate in-place live updates on live: true everywhere: setData/appendData
  on static charts now only store the new data for the next focus-in,
  matching the documented opt-in contract. Failed swaps dispose the
  controller so the next focus-in rebuilds cleanly.
- Keep MonitorService's live flag in sync when a data update flips
  Maidr.live (turning it off also stops monitoring).
- Serialize chart data once in ChatService.updateData and share the JSON
  string across LLM providers instead of stringifying per model.
- Support selective bundle builds (node scripts/build.js core react)
  without emptying dist.

https://claude.ai/code/session_01KuiTyhDAEWtQX9oTKGatzN
@claude

claude Bot commented Jun 11, 2026

Copy link
Copy Markdown

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.

- Restore the trace cursor in a finally block in announceAppendedPoint so
  a throwing state getter cannot leave the cursor mutated.
- Attach the LiveDataPoint JSDoc to its declaration (was orphaned above
  NESTED_DATA_TYPES).
- Tighten Context.replaceFigure to accept only a factory, keeping the
  dispose-before-construct ordering guarantee explicit; tests updated.
- Document the layer-index/trace-row 1:1 mapping in resolveActiveColShift
  and log instead of silently swallowing errors there.
- Mark fields that intentionally lost readonly (controller/context/
  highContrast figure) as mutable-by-design, note that tests should
  construct their own LiveDataManager, and replace the resolveId cast
  with a non-null assertion.
- Add cloneMaidrData unit tests (deep clone semantics, onNavigate
  preservation).
- Docs: bound the declarative React streaming example with an explicit
  window and steer streaming users to appendMaidrData (which applies
  maxWidth automatically); clarify the demo's parallel SVG point copy.

https://claude.ai/code/session_01KuiTyhDAEWtQX9oTKGatzN

Copy link
Copy Markdown
Member Author

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

claude Bot commented Jun 11, 2026

Copy link
Copy Markdown

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

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

feat: add realtime/streaming data support

3 participants