feat: add auditory directional guidance towards svg element with mouse

[soundaryakp1999]

Pull Request

Description

When the user moves the mouse, auditory queues point them towards the nearest point in the curve.

Related Issues

Changes Made

Earlier behavior: the sonified (x,y) position is played if the mouse curser or touchscreen pointer touches any of the elements present in the svg. Nearby points provide no indication of proximity to the curve.
Current behavior: The relative direction to the nearest point from curser is represented with auditory notification. High pitch indicates that the point is above the curser. stereo panning to the right indicates that the point is to the right side. This is designed to guide the user towards the point, which will help the user physically draw the curve on the mouse, thus reinforcing a realistic shape of the curve.

Screenshots (if applicable)

Checklist

• I have read the Contributor Guidelines.
• I have performed a self-review of my own code and ensured it follows the project's coding standards.
• I have tested the changes locally following ManualTestingProcess.md, and all tests related to this pull request pass.
• I have commented my code, particularly in hard-to-understand areas.
• I have updated the documentation, if applicable.
• I have added appropriate unit tests, if applicable.

Additional Notes

[claude]

Code Review

Thanks for the contribution! The feature concept is well thought-out — using pitch and stereo pan to guide users toward the nearest curve point is a meaningful accessibility improvement. Here are some issues to address before this is ready to merge.

---

Critical: DOM Access in the Model Layer

src/model/abstract.ts — getTouchGuidance

const bbox = nearest.element.getBoundingClientRect();
const centerX = bbox.x + bbox.width / 2;
const centerY = bbox.y + bbox.height / 2;

getBoundingClientRect() is a DOM/layout API and must not live in the Model layer. Per the MVVC architecture, the Model contains data and navigation only — no UI concerns. This should be moved to a Service that already has legitimate DOM access (e.g. HighlightService or a dedicated TouchGuidanceService).

---

Architecture: Service Calling Service Directly

src/service/keybinding.ts

const guidance = this.commandContext.context.getTouchGuidance(x, y);
this.commandContext.audioService.playTouchGuidance(guidance);

Mousebindingservice now calls AudioService directly, bypassing the Command → Observer chain that the rest of the codebase uses. User input (mouse/pointer) should translate into a Command, which then triggers model changes, which then notify Observers (AudioService included). Direct cross-service calls make the flow harder to follow and test, and this sets a precedent that undermines the architecture.

Suggested approach: compute guidance in the mouse listener, then route it through the existing command infrastructure rather than calling audioService directly.

---

Bug: Incomplete Resource Cleanup in stopAll

src/service/audio.ts — playTouchGuidanceBeep

this.activeAudioIds.set(audioId, oscillator); // only the oscillator is tracked

gainNode and stereoPanner are not stored in activeAudioIds. When stopAll() is called (e.g. on dispose()), only oscillator.disconnect() and oscillator.stop() run; the gain and panner nodes remain connected to the compressor, leaking Web Audio graph nodes until GC.

Either track all three nodes (the map already supports OscillatorNode[]), or disconnect the gain/panner inside the existing setTimeout callback before removing from the map.

---

Minor: Volume Floor Ignores Mute

const guidanceVolume = Math.max(this.volume * TOUCH_GUIDANCE_VOLUME, 0.05);

When this.volume is 0 (user has muted audio), guidance beeps still play at 0.05. If this is intentional (always-audible guidance), it should be documented explicitly; if not, the floor should be removed or conditional on intent.

---

Minor: Missing Blank Line in Interface

src/model/plot.ts — missing blank line between getTouchGuidance JSDoc and the next method (getExtremaTargets), inconsistent with the rest of the interface formatting.

---

No Tests

The checklist notes "I have added appropriate unit tests, if applicable" is unchecked. Given that getTouchGuidance has directional logic (above/below, left/right, on-curve detection) and playTouchGuidance has rate-limiting and interpolation logic, unit tests would be valuable and straightforward to write here.

---

Summary

Severity	Issue
Critical	DOM access (getBoundingClientRect) in Model layer
High	Mousebindingservice calling AudioService directly (architecture bypass)
Medium	gainNode/stereoPanner not tracked → not disconnected in stopAll()
Low	Volume floor of 0.05 when muted — clarify intent
Low	Missing blank line in plot.ts interface
Low	No unit tests for new directional logic

The directional guidance idea is solid and the audio implementation (frequency mapping, rate-limiting by distance, stereo pan) is well-structured. Addressing the architecture and DOM-access concerns will make this mergeable.

[soundaryakp1999]

I have addressed the feedback of claude bot

[claude]

Follow-up Review

Good progress addressing the previous review — the resource cleanup, volume floor, and command encapsulation are all improved. Here is a review of the code as it stands now.

---

What's Well Done

• PointerGuidanceCommand correctly follows the Command pattern, keeping pointer input in the command flow rather than directly in the binding service.
• touchGuidance.ts is a clean, pure module — easy to test and reason about in isolation.
• stopAudioNode() refactor is a solid improvement: properly handles the generic AudioNode vs OscillatorNode distinction with a safe try/catch around .stop().
• activeAudioIds now stores [oscillator, gainNode, stereoPanner] — all three nodes will be disconnected in stopAll().
• Tests included for the core pure-function logic.

---

Bug: Web Audio Nodes Created Before Volume Check

src/service/audio.ts — playTouchGuidanceBeep

const oscillator = this.audioContext.createOscillator();  // created
const gainNode   = this.audioContext.createGain();        // created
const guidanceVolume = this.volume * TOUCH_GUIDANCE_VOLUME;
if (guidanceVolume <= 0) {
  return;  // ← oscillator and gainNode are abandoned here
}

AudioContext.createOscillator() and createGain() allocate internal Web Audio resources. Returning early without ever calling .disconnect() on them leaves orphaned nodes in the AudioContext graph. They will GC eventually, but in high-frequency pointer-move scenarios this can accumulate.

Fix: move the guard before node creation.

const guidanceVolume = this.volume * TOUCH_GUIDANCE_VOLUME;
if (guidanceVolume <= 0) {
  return;
}
const oscillator = this.audioContext.createOscillator();
const gainNode   = this.audioContext.createGain();
// ...

---

Performance: findNearestPoint Called Twice Per Pointer Event

src/command/pointerGuidance.ts

this.context.moveToPoint(clientX, clientY);           // internally calls findNearestPoint
const guidance = this.context.getTouchGuidance(clientX, clientY);  // calls it again

moveToPoint and getTouchGuidance both delegate to the active trace's findNearestPoint. On a dense scatter plot with many points, this doubles the work on every pointermove event, which can fire dozens of times per second.

Consider caching the result of findNearestPoint per (x, y) call, or having moveToPoint return (or cache) the nearest point so getTouchGuidance can reuse it.

---

Naming Inconsistency

The command is PointerGuidanceCommand (file: pointerGuidance.ts), but the state type is TouchGuidanceState, the service file is touchGuidance.ts, and the audio method is playTouchGuidance. Pick one naming convention — pointer or touch — and apply it consistently. Since the feature works for both mouse and touch input, pointer is the more accurate term.

---

Bar Chart Guidance Behaves Differently From Other Traces

BarTrace.findNearestPoint does a hit-test against bar bounding boxes and returns null when the cursor is between bars. Every other trace (LineTrace, ScatterTrace, BoxTrace, etc.) returns the nearest center regardless of cursor position. This means:

• On line/scatter charts: guidance beeps fire whenever the cursor is within maxDistancePx of any point.
• On bar charts: guidance fires only when the cursor is inside a bar; no guidance between bars.

This asymmetry should be documented, either in a comment on BarTrace.findNearestPoint or in the PR description, so future maintainers understand it is intentional.

---

Test Gaps

The new test file covers resolveTouchGuidanceBeep well, but the following remain untested:

What	Why it matters
PointerGuidanceCommand.execute() — with and without event	Verifies the command correctly resets guidance on leave
AbstractTrace.getTouchGuidance()	The onCurve, verticalRelation, horizontalRelation logic is non-trivial
resolveTouchGuidanceBeep with distancePx > maxDistancePx	Boundary case — should return null
AudioService.playTouchGuidance rate-limiting (nextTouchGuidanceBeepAt)	The throttle is core to the UX

---

Summary

Severity	Issue
Medium	oscillator/gainNode created before volume guard → node leak
Medium	findNearestPoint called twice per pointer event
Low	PointerGuidance / TouchGuidance naming inconsistency
Low	Bar chart guidance gap not documented
Low	Test gaps for command, model, and audio rate-limiting

The feature architecture is sound and the major issues from the previous review have been addressed. Fixing the node-leak bug and the double findNearestPoint call would make this ready for a final pass.

[claude]

Code Review — PR #587: Auditory Directional Guidance for Mouse Exploration

Overview

This PR adds pointer/touch guidance sonification: as the user moves the mouse near a chart, directional beeps (pitch = vertical relation, pan = horizontal relation, interval = proximity) guide them toward the nearest data point. The feature is well-scoped and the core idea is solid, but there are a few architectural and correctness issues worth addressing.

---

✅ Strengths

• Good Command pattern usage — PointerGuidanceCommand keeps pointer-event handling out of Mousebindingservice and in the command flow, as the docstring correctly explains.
• Performance optimization — Combining findNearestPoint + moveToNearest into moveToPointAndGetPointerGuidance avoids double nearest-point scans per pointer event. Clear win on dense plots.
• Clean separation — resolvePointerGuidanceBeep in src/service/pointerGuidance.ts is a pure function with no side effects, making it easy to test and reason about.
• Audio resource management — stopAudioNode correctly handles the broadened AudioNode | AudioNode[] map, gracefully skipping stop() on GainNode/StereoPannerNode nodes.
• Throttling — nextPointerGuidanceBeepAt prevents a flood of beeps on rapid pointer events.
• Test coverage for the pure utility — The pointerGuidance.test.ts file covers the key cases.

---

🔴 Issues

1. Incomplete findNearestPoint return-type updates

The diff updates findNearestPoint to include centerX/centerY in: bar, box, candlestick, heatmap, line, scatter, violin, violinBox. Missing from the diff:

• src/model/histogram.ts
• src/model/segmented.ts (STACKED / DODGED / NORMALIZED)
• Smooth trace (createSmoothTrace)

If any of these override findNearestPoint, they still return the old type shape and TypeScript will likely surface a compile error — or worse, centerX/centerY will be undefined at runtime in moveToPointAndGetPointerGuidance, producing NaN distances. Please verify npm run type-check passes and add the missing fields to every findNearestPoint override.

2. click hover mode produces a guidance beep instead of data sonification

// keybinding.ts
} else if (this.hoverMode === 'click') {
  this.plot.addEventListener('click', this.mouseListener);
  this.plot.addEventListener('pointerleave', this.pointerLeaveListener);

this.mouseListener now calls pointerGuidanceCommand.execute(event). On a click event, hasClientCoordinates returns true, so the command fires playPointerGuidance. If the click lands exactly on a data point, onCurve is true and the beep is suppressed — but if it lands slightly off, the user gets a guidance beep instead of the expected data sonification. This is a regression in the click hover mode behavior.

Suggestion: Check hoverMode inside PointerGuidanceCommand or in mouseListener, and skip guidance for click events (or keep the original moveToPoint path for clicks).

3. context.moveToPoint() is removed — verify no external callers remain

The moveToPoint method on Context is gone. The Trace interface still declares moveToPoint, and AbstractTrace.moveToPoint() still exists, but nothing routes through context.moveToPoint() anymore. A quick grep check should confirm no other callers exist (e.g., in future external integrations or tests):

grep -r "moveToPoint" src/ test/

If this was part of the public API surface, it should either be kept (delegating to moveToPointAndGetPointerGuidance and discarding the result) or explicitly documented as removed.

---

🟡 Minor / Non-blocking

4. clamp and interpolate duplicated from MathUtil

src/service/pointerGuidance.ts defines private clamp and interpolate functions. MathUtil in @util/math already provides these. Prefer importing from the shared utility to avoid drift.

5. Test coverage gaps

The test covers only resolvePointerGuidanceBeep. Consider adding:

• A test for AudioService.playPointerGuidance (e.g., verify no beep plays when mode === OFF, verify throttling resets on null guidance).
• A test for PointerGuidanceCommand.execute with and without client coordinates.

6. PointerGuidanceCommand bypasses CommandFactory

The command is instantiated directly in Mousebindingservice rather than through CommandFactory. This is pragmatic (the event object carries coordinates, unlike keyboard commands), but it means command creation is split across two places. At minimum, a brief comment explaining why this deviates from the factory pattern would help future contributors.

7. Directional mapping comment vs. PR description mismatch

The PR description states "stereo panning to the right indicates that the point is to the right side" — this means audio pans toward where the curve is. The code implements exactly that (pointer left of curve → pan right, toward the curve), and the inline comment // Inverted pan by requirement: pointer right of curve => pan left is correct. However, the phrase "inverted" may confuse readers. Consider rewording the comment to "pan toward the curve: pointer left of curve → positive (right) pan".

8. distancePx is set even when onCurve is true

return {
  onCurve,
  distancePx: Math.hypot(nearest.centerX - x, nearest.centerY - y),
  ...
};

This is harmless (the on-curve early-exit in playPointerGuidance discards the value), but could mislead future callers who check distancePx without first checking onCurve. Consider documenting this or returning 0 for the on-curve case.

---

Summary

The feature is well-architected and the core logic is correct. The critical items to resolve before merge are:

1. Verify all findNearestPoint overrides return centerX/centerY (histogram, segmented, smooth).
2. Fix the click hover mode regression — clicks should not play guidance beeps.
3. Confirm context.moveToPoint() removal has no external callers.

The minor items (utility duplication, test gaps, comment clarity) can be addressed as follow-ups.

---

Review generated with Claude Code

[claude]

Code Review: feat: add auditory directional guidance towards SVG element with mouse

Overview

This PR introduces proximity-based auditory guidance for pointer/touch exploration. When the pointer is off-curve, directional beeps encode:

• Pitch: high when cursor is below the nearest data point, low when above
• Stereo pan: pulls audio toward the curve
• Rate: faster when closer, slower when farther (up to 160px max)

The feature is well-conceptualized and meaningfully advances MAIDR's core accessibility mission.

---

Architecture & Design

What works well:

• The resolvePointerGuidanceBeep pure function in src/service/pointerGuidance.ts cleanly isolates the mapping logic and is independently testable.
• Combining findNearestPoint + navigation into moveToPointAndGetPointerGuidance avoids double-scanning on every pointermove event — a real and meaningful optimization.
• moveToNearest as a protected hook is an elegant extension point; BoxTrace and ViolinBoxTrace override it cleanly.
• The stopAudioNode helper with 'stop' in node correctly handles the heterogeneous AudioNode | AudioNode[] map.

Concerns:

1. PointerGuidanceCommand bypasses CommandFactory (acknowledged but worth discussing)

The JSDoc explains the deviation. However, Mousebindingservice now does double duty: event routing + command lifecycle. An alternative would be a factory method that accepts coordinates, or passing a CommandContext-aware builder. Low priority but worth noting for future consistency.

2. moveToNearest override with empty parameter list

// box.ts and violinBox.ts
protected override moveToNearest(): void {
  // Disabled for boxplots
}

TypeScript allows this (parameter contravariance), but overriding a 4-parameter method with 0 parameters is surprising. Retaining the parameters as _x, _y, _nearest, _onCurve or adding a comment would help future readers.

3. NearestPoint.centerX / centerY is now a required interface field

All concrete implementations in-repo are updated — confirmed. But since this is a public interface, any external code implementing findNearestPoint would silently break at runtime. Consider making these optional (centerX?: number) or documenting the breaking change.

---

Potential Bugs

4. AudioContext.currentTime when suspended returns 0 continuously

const now = this.audioContext.currentTime;
if (now < this.nextPointerGuidanceBeepAt) { return; }

If AudioContext is suspended (common before first audio interaction), currentTime stays at 0. Pointer-leave/re-enter resets nextPointerGuidanceBeepAt to 0, clearing the guard immediately each cycle. Worth a comment noting this matches the rest of the audio system's behavior (suspension is handled upstream).

5. verticalRelation naming is ambiguous

verticalRelation: y < nearest.centerY ? 'above' : 'below',

'above' means "cursor is above the nearest point." The audio mapping verticalRelation === 'below' → high frequency is correct and matches the PR description ("high pitch indicates the point is above the cursor"). But the field name doesn't make the reference frame clear. A rename to cursorVerticalPosition (and same for horizontal) would make the semantics unambiguous at call sites.

---

Missing Tests

6. AudioService.playPointerGuidance has no unit tests

The rate-limiting logic (nextPointerGuidanceBeepAt), AudioMode.OFF short-circuit, and on-curve bypass are non-trivial behaviors that could regress silently. Suggested cases:

• Beep is suppressed when guidance.onCurve === true
• Beep is suppressed when AudioMode.OFF
• nextPointerGuidanceBeepAt prevents rapid re-firing
• Reset to 0 on playPointerGuidance(null)

7. No test for pointerleave listener wiring in Mousebindingservice

The existing tests cover PointerGuidanceCommand and resolvePointerGuidanceBeep in isolation, but switching hover modes and the leave-event reset path have no coverage.

---

Minor / Style

• The else branch in registerEvents calls this.pointerGuidanceCommand.execute() to reset guidance when hoverMode is 'none'. A named reset() method on PointerGuidanceCommand would make the intent self-documenting.
• MathUtil.interpolate refactoring — flat signature is more composable than the old Range-object form; the DRY improvement in AudioService is welcome.
• Mousebindingservice capitalization — pre-existing; MouseBindingService would be proper PascalCase. Out of scope for this PR.

---

Summary

Area	Status
Feature correctness	✅ Logic is sound and internally consistent
MVVC architecture	✅ Mostly followed; CommandFactory deviation is local and documented
Audio cleanup	✅ StereoPanner properly tracked and disconnected
Trace coverage	✅ All traces covered (Histogram via AbstractBarPlot, Smooth via LineTrace)
Test coverage	⚠️ AudioService.playPointerGuidance untested
Naming clarity	⚠️ verticalRelation semantics ambiguous
Breaking change	⚠️ NearestPoint interface extended with required fields
AudioContext edge case	ℹ️ Benign but worth a comment

Overall this is solid work — the architecture is clean, the optimization rationale is well-justified, and the accessibility value is high. Addressing the missing AudioService tests and the verticalRelation naming would be the highest-value improvements before merge.

[claude]

Code Review – PR #587: Auditory Directional Guidance for Mouse Exploration

Overview

This PR adds hover-driven directional audio guidance: as a user moves the mouse near a data curve, stereo-panned beeps guide them toward the nearest point. High pitch = point is above cursor; low pitch = point is below. Beep frequency increases as the user gets closer. The implementation combines navigation and guidance into a single findNearestPoint call to avoid redundant work on dense plots.

Files changed: ~1,000 LOC across model, service, command, type, and test layers.

---

What Works Well

Architecture

• PointerGuidanceCommand correctly follows the Command pattern. The inline comment explaining why it bypasses CommandFactory (pointer events carry coordinates the keyboard factory contract doesn't accept) is a good self-documenting design decision.
• resolvePointerGuidanceBeep in src/service/pointerGuidance.ts is a clean pure function — easy to test in isolation, and the tests prove it.
• moveToPointAndGetPointerGuidance correctly fuses navigation + guidance to avoid a double findNearestPoint scan. The abstract base returns null for non-trace contexts, keeping Figure/Subplot transparent.
• Extracting moveToNearest as a protected hook for subclasses (BoxTrace, ViolinBoxTrace, ScatterTrace) is cleaner than the previous moveToPoint override pattern.

Audio resource management

• Widening activeAudioIds to AudioNode | AudioNode[] and adding stopAudioNode properly handles GainNode/StereoPanner nodes (which have no .stop()). The try/catch in stopAudioNode avoids crashes if a node was already stopped.
• Cleanup via activeAudioIds ensures guidance beep nodes are disposed on dispose().

Test coverage

• Three new test files cover the command (edge cases for missing coordinates), the AudioService guidance method (mode, on-curve, rate-limiting, throttle reset), and the pure beep resolver.
• Rate-limiting tests use the mock currentTime to simulate time advancing — pragmatic and effective.

Refactoring wins

• MathUtil.clamp / MathUtil.interpolate extracted from AudioService — these were duplicated across the codebase.
• Removing the stub moveToPoint no-ops from Figure and Subplot cleans up dead code.

---

Issues & Suggestions

1. Throttle reset is tied to AudioMode.OFF (minor logic concern)

// src/service/audio.ts
if (this.mode === AudioMode.OFF || !guidance || guidance.onCurve) {
  this.nextPointerGuidanceBeepAt = 0;
  return;
}

Resetting the throttle when audio is OFF means that if the user enables audio while hovering mid-path, the very next pointermove fires a beep immediately. This conflates two distinct reset semantics: "pointer left" (null guidance) vs "audio disabled". Consider resetting the throttle only on null guidance and on-curve, not on mode:

if (!guidance || guidance.onCurve) {
  this.nextPointerGuidanceBeepAt = 0;
  return;
}
if (this.mode === AudioMode.OFF) {
  return;  // don't reset throttle — preserve timing state
}

2. Histogram and SegmentedTrace not in the diff

NearestPoint now requires centerX and centerY. TypeScript will catch missing fields in any subclass that overrides findNearestPoint, but please confirm that Histogram, SegmentedTrace, and the smooth trace variant either inherit an already-updated findNearestPoint or have been updated elsewhere. If they're missed, the build will fail — worth a quick verification.

3. The 2× multiplier in the cleanup timeout is undocumented

// src/service/audio.ts
}, POINTER_GUIDANCE_BEEP_DURATION * 1000 * 2);

Why 2×? Is this to give the exponentialRampToValueAtTime tail time to complete before disconnecting? A short inline comment would prevent future confusion.

4. cursorVerticalPosition naming can be misread

cursorVerticalPosition === 'below' means the cursor is below the curve (so the curve point is above). A maintainer scanning the beep logic might initially read 'below' as "point is below" rather than "cursor is below the point." The existing comment in pointerGuidance.ts helps, but repeating that clarification near the type definition (or renaming to cursorPositionRelativeToCurve) would make the intent harder to accidentally invert.

5. Missing test: moveToNearest no-op for BoxTrace / ViolinBoxTrace

There are no tests verifying that BoxTrace.moveToNearest and ViolinBoxTrace.moveToNearest are no-ops while moveToPointAndGetPointerGuidance still returns non-null guidance. Given this is the whole point of the override, a test for it would strengthen the contract.

6. PointerGuidanceCommand bypasses CommandExecutor scope validation

This is documented and intentional, but pointer guidance can theoretically fire in scopes where it shouldn't (e.g., braille mode, figure-level navigation). Consider adding a guard in execute that checks context.scope and no-ops outside trace scope, mirroring how CommandExecutor.isValidForScope works.

---

Minor Nits

• Separating pointermoveListener, clickListener, and pointerLeaveListener into distinct fields (rather than reusing a single mouseListener) is a clear improvement for readability and correctness.
• Using PointerEvent type for pointermoveListener (rather than MouseEvent) is more accurate since the listener is bound to pointermove.
• Mousebindingservice (lowercase 's') is a pre-existing naming issue — not introduced here, fine to leave.

---

Summary

This is a well-designed, well-tested feature that follows the project's MVVC architecture. The main items to address before merging are: (1) verifying that all trace types with findNearestPoint overrides include centerX/centerY (build will catch misses), and (2) the throttle-reset semantics when audio is OFF. The code is clean, the design decisions are documented, and the tests cover the non-obvious rate-limiting behavior. Good work overall.

[claude]

Code Review: Auditory Directional Guidance for Mouse Exploration

Overall Assessment: This is a well-designed, well-tested feature addition. The implementation follows MAIDR's MVVC architecture cleanly, the code is thoroughly documented, and the test coverage is solid. A few medium-priority issues and several minor points are worth addressing before merge.

---

Overview

The PR adds real-time auditory directional guidance when a user moves a pointer near a chart. High pitch indicates the curve is above the cursor; stereo panning draws the user left/right toward the nearest data point. Guidance beeps rate-limit using the AudioContext clock and silence when on-curve (deferring to the existing sonification chain). The implementation spans:

• A new PointerGuidanceCommand that deviates intentionally from CommandFactory (pointer coordinates don't fit the keyboard command contract — well-justified by the doc comment)
• A moveToNearest protected hook replacing the old moveToPoint — avoids calling findNearestPoint twice per pointer event
• MathUtil.clamp / MathUtil.interpolate utility extractions (good DRY improvement)
• 4 new test files with meaningful coverage

---

Medium Issues

1. executeNavigateOnly signature is weaker than needed

The execute method handles a missing event gracefully (resets guidance). executeNavigateOnly silently no-ops if the event lacks coordinates — this behavior is undocumented. If the raw Event type is intentional to allow future callers, at minimum document that. Otherwise consider tightening the type to PointerEvent | MouseEvent to make the intent clear at the call site.

2. moveToNearest optional parameter can mask bugs

The onCurve parameter is pre-computed by the caller to avoid double-calling isPointInBounds, but if a subclass overrides moveToNearest and ignores the passed value (as BoxTrace and ViolinBoxTrace do), there is no enforcement. A caller who passes the wrong value would silently misbehave. Consider making it non-optional (onCurve: boolean) so subclasses are forced to explicitly accept the contract.

3. Guidance config values are not user-configurable

The parameters in DEFAULT_POINTER_GUIDANCE_CONFIG (highFrequency: 1280, lowFrequency: 420, panMagnitude: 0.7, etc.) heavily influence the user experience. Users with hearing loss at specific frequencies may find one of the two pitch values inaudible. There is no path to expose these in SettingsService. A follow-up issue to surface at least highFrequency/lowFrequency in settings would be valuable.

---

Minor Issues

4. Linter workaround in test file is a code smell

In test/model/moveToNearestOverride.test.ts there is void jest.fn(); // touch jest import so the linter keeps it. If jest is only used for that workaround, both the import and the line should be removed. If the linter requires the import, restructure the import block so the real usage (describe, expect, test) satisfies it.

5. Edge case: cursor exactly at center

cursorVerticalPosition: y < nearest.centerY ? 'above' : 'below',
cursorHorizontalPosition: x < nearest.centerX ? 'left' : 'right',

When the cursor lands exactly on centerX or centerY, the tie-break resolves to 'below'/'right'. This is an unlikely edge case (sub-pixel precision), but worth a one-line comment explaining the tie-break choice.

6. AudioMode interaction is implicit

The guidance beep fires in all non-OFF modes (SEPARATE, COMBINED, AUTOPLAY, etc.) without checking whether the current mode should suppress it. This may be intentional (guidance is mode-agnostic), but a comment clarifying why only OFF is excluded would prevent future maintainers from adding unintended mode handling here.

---

Positive Highlights

• The moveToNearest hook cleanly solves the double-findNearestPoint problem without breaking the existing observer-notification chain
• Throttle implementation using audioContext.currentTime (rather than wall-clock Date.now()) keeps guidance timing coherent with audio scheduling — including the correct handling of the suspended-context currentTime=0 edge case
• The PointerGuidanceState doc comment in state.ts correctly disambiguates the "cursor frame" vs "curve frame" mental model — easy to get backwards, and the explanation prevents that
• Switching from mousemove to pointermove correctly handles touch and stylus input
• stopAudioNode with try/catch around stop() is the right defensive approach for nodes that may have already finished
• Test coverage is thorough — especially the rate-limiter, mode-toggle, and throttle-reset scenarios

---

Not Blocking, Worth Filing

• Histogram: confirm AbstractBarPlot inheritance gives it the updated findNearestPoint return type automatically (the diff has no changes to histogram.ts)
• Text/braille modality: off-curve guidance is currently audio-only. A screen reader user with audio disabled gets no directional cue. A follow-up to emit a brief text description (e.g. "above left") would extend the feature to those users

[claude]

Code Review — PR #587: Auditory Directional Guidance for Mouse/Touch

Overview

This PR adds proximity-based auditory guidance to help users navigate to data points by moving the mouse. When hovering near (but not on) a curve, short beeps signal direction to the nearest data point — high pitch means the point is above the cursor, stereo panning pulls the user horizontally toward the curve. The feature is well-scoped and the implementation is generally clean.

---

✅ Strengths

• Single-pass design — combining findNearestPoint and navigation into moveToPointAndGetPointerGuidance avoids double-scanning on every pointermove event. Good performance thinking.
• moveToNearest template hook — clean way to let BoxTrace/ViolinBoxTrace suppress movement while still surfacing guidance.
• pointerGuidance.ts service is pure — resolvePointerGuidanceBeep takes only data in, returns only data out. Easy to unit-test and reason about.
• MathUtil.clamp / MathUtil.interpolate — good DRY refactor; the private AudioService copies were replaced correctly.
• stopAudioNode with try/catch — defensive handling of already-stopped oscillators is correct; the 'stop' in node guard makes the AudioNode widening safe.
• Rate-limiting — nextPointerGuidanceBeepAt throttle is well thought-out, and the intentional choice not to reset it on audio-OFF is documented and tested.
• Test coverage — four test files covering the command, model hook, service logic, and audio integration. The moveToNearestOverride contract test is especially valuable.

---

🐛 Issues

1. HistogramTrace / SegmentedTrace / SmoothTrace not updated

The NearestPoint interface now requires centerX and centerY, but the diff doesn't touch histogram, segmented, or smooth traces. If any of those have a findNearestPoint implementation that returns the old shape, TypeScript will catch it at build time — but it's worth confirming npm run type-check passes with no errors before merge.

2. pointerleave listener not added for click hover mode

In registerEvents, the pointerLeaveListener is only added in the pointermove branch. But in removeEventListeners, there's a guard (if (this.pointerLeaveListener)) that silently tries to remove it even when it was never registered. This is harmless today, but it means if click mode ever needs pointer-leave cleanup, the guard condition alone won't be enough. A comment noting the intentional asymmetry would help future readers.

3. StereoPannerNode browser support

createStereoPanner() is not available in Safari < 14.5 (which includes many iOS users who rely on assistive technology). If MAIDR targets older iOS/Safari, this will throw at runtime. Consider a fallback panner or a feature-detection guard:

const stereoPanner = this.audioContext.createStereoPanner?.()
  ?? this.audioContext.createPanner();

4. No ScatterTrace mode-switch test

ScatterTrace.moveToNearest sets this.mode = NavMode.COL before delegating. There's no test verifying this side-effect occurs during pointer guidance. Since mode drives navigation direction, a missed switch could silently break keyboard navigation after a hover interaction.

---

💬 Suggestions

5. PointerGuidanceCommand constructor visibility

// current
public constructor(context: Context, audioService: AudioService) {

// suggestion — `public` is the TypeScript default; omit it to reduce noise
constructor(context: Context, audioService: AudioService) {

6. Test style inconsistency

test/command/pointerGuidance.test.ts and test/model/moveToNearestOverride.test.ts use test(...), while test/service/pointerGuidance.test.ts uses it(...). Neither is wrong, but consistent style across the suite is easier to scan.

7. No user-configurable guidance settings

DEFAULT_POINTER_GUIDANCE_CONFIG (max distance, frequency range, interval range, pan magnitude) is hardcoded. For an accessibility feature, the ability to tune sensitivity could be meaningful — especially maxDistancePx and the frequency range for users with partial hearing loss or different monitor DPI. Worth a follow-up issue if not addressing in this PR.

8. Bar chart silence between bars is undiscoverable

The design decision to return null from BarTrace.findNearestPoint when the cursor is between bars is intentional and documented in the code. Consider noting it in the PR description or release notes so accessibility users know to expect silence between bars (no "approaching" guidance for area marks).

9. Minor: (globalThis as any).AudioContext in tests

// current
(globalThis as any).AudioContext = function () { return ctx; };

// prefer — avoids `any`
(globalThis as unknown as { AudioContext: new () => MockAudioContext }).AudioContext = ...

---

Summary

The architecture is sound — the guidance flow fits naturally into the existing Observer/Command/Service layers, and the test suite covers the important invariants. Resolve items 1 and 3 (type-check verification + StereoPannerNode fallback) before merge, and consider filing follow-up issues for items 4 and 7. The rest are minor polish.

[claude]

Code Review - PR 587: Auditory Directional Guidance for Mouse/Touch Exploration

Overview

This PR adds proximity-based audio guidance that helps users trace data curves with mouse/touch. When the cursor is off-curve, a beep fires with pitch encoding vertical direction (high = point is above cursor) and stereo pan encoding horizontal direction. Beep rate increases as cursor nears the curve. Well-documented, thoughtful implementation with solid unit test coverage.

Architecture and Design

Mostly compliant with MVVC. A few things to discuss:

Command bypasses CommandFactory - PointerGuidanceCommand deliberately bypasses CommandFactory because pointer events carry coordinates the factory contract does not accept. The justification is sound, but the scope-validation logic from CommandExecutor.isValidForScope is now manually re-implemented inside PointerGuidanceCommand.isInTraceScope(). If the scope logic ever changes centrally, this copy will not update automatically. Consider a comment warning future maintainers, or a lightweight adapter that lets CommandFactory produce parameterized commands.

Bar chart guidance is intentionally asymmetric - Bar charts only produce onCurve: true (because findNearestPoint returns null outside bars), so guidance beeps never fire when approaching from empty space. This is documented and intentional, but the UX differs noticeably from line/scatter. Worth confirming with the team that this is the desired final behavior.

Potential Bugs and Concerns

1. AudioContext.currentTime stalls when context is suspended - If the AudioContext is suspended mid-session (e.g. browser tab backgrounded), currentTime freezes and the throttle can block guidance permanently until the user triggers a gesture that resumes it. The comment acknowledges the suspended-on-load case, but the mid-session scenario is distinct and could be confusing. A fallback to performance.now() when currentTime is 0 could help, but may be out of scope.

2. Histogram / Segmented traces not visible in diff - The NearestPoint interface now requires centerX and centerY. The diff shows updates for Bar, Box, Candlestick, Heatmap, Line, Scatter, Violin, and ViolinBox. Please confirm that Histogram, SegmentedTrace, SmoothTrace, and any other concrete traces were also updated, or that TypeScript compilation confirms no gaps.

3. Out-of-range guidance resets the throttle - In playPointerGuidance, when resolvePointerGuidanceBeep returns null (off-curve but distancePx > maxDistancePx), the code zeros nextPointerGuidanceBeepAt. This re-arms the throttle on every pointermove beyond 160px, so the next in-range event always fires immediately. If a user oscillates just inside/outside maxDistancePx quickly, they could get a beep on every crossing. Low severity, but worth verifying the UX is acceptable or documenting the intent.

Code Quality Highlights

Positives:

• The moveToNearest template-method hook is clean - Box/ViolinBox no-op the move while still returning guidance, Scatter sets its nav mode before delegating. Extensible without touching the base class.
• Combining navigation and guidance into a single moveToPointAndGetPointerGuidance call avoids a second findNearestPoint traversal - a genuine performance win on dense plots.
• Safari createStereoPanner fallback is correct and graceful - horizontal direction is lost on unsupported platforms but vertical pitch still works.
• stopAudioNode with a try/catch handles already-stopped oscillators cleanly, consistent with existing cleanup patterns.
• Refactoring interpolate/clamp into MathUtil is good housekeeping.

Minor nits:

• PointerGuidanceCommand has a 35-line JSDoc block for a 108-line file. Per project conventions, comments should target non-obvious WHYs - the class-level doc could be trimmed to the factory-bypass reason and the two execution paths.
• In the audio test, createSettings() mocks get to return 100 for all keys. A brief comment clarifying this represents volume=100 would help future readers.

Test Coverage

Three new test files cover the critical paths well:

• pointerGuidance.test.ts - command behavior, scope guards, reset semantics
• moveToNearestOverride.test.ts - hook pattern contract (Box/ViolinBox no-op, Scatter side-effect, null nearest)
• audio.pointerGuidance.test.ts - rate-limiting, mode handling, throttle reset

Missing:

• No test for the out-of-range distancePx > maxDistancePx path that resets the throttle (concern 3 above).
• No unit tests for MathUtil.clamp and MathUtil.interpolate (flagged in the checklist). Given these now support a new audio feature, a few edge-case tests would prevent regressions.

Summary

This is a well-engineered feature with clear design intent and solid core test coverage. Main items before merge:

1. Confirm all concrete trace types updated findNearestPoint (TypeScript build passing is evidence, but a quick grep would confirm).
2. Clarify the bar chart UX decision - no off-curve guidance is intentional, but worth team sign-off.
3. Decide on the out-of-range throttle reset (concern 3) - document as expected or adjust the logic.
4. Add MathUtil.clamp/interpolate unit tests as flagged in the checklist.

Overall the architecture is sound and the accessibility improvement is meaningful. Happy to discuss the CommandFactory deviation further if there is a preferred pattern for parameterized commands going forward.

[claude]

Code Review — PR #587: Auditory Directional Guidance for Mouse Exploration

Overview

This PR adds real-time audio guidance when users hover near data points: pitch signals vertical direction (high = point is above cursor), stereo pan signals horizontal direction (pan right = point is to the right). The feature is off-curve only — on-curve navigation continues through the existing Observer chain. The implementation is generally clean with solid test coverage.

---

Architecture & Design

What is working well:

• PointerGuidanceCommand and resolvePointerGuidanceBeep correctly separate concerns. The command handles scope/coordinate validation; the pure function handles audio math.
• The moveToNearest template-method hook is a clean extension point. BoxTrace / ViolinBoxTrace no-op the hook without sacrificing guidance state; ScatterTrace injects its mode switch before delegating — both are readable patterns.
• Removing the empty moveToPoint implementations from Figure and Subplot (which were already no-ops that called notifyStateUpdate for no reason) is a good cleanup.
• Extracting clamp / interpolate to MathUtil is the right call; the private copies in AudioService were redundant.
• Safari < 14.5 createStereoPanner fallback is thoughtful — pitch still conveys vertical direction on unsupported platforms.

---

Concerns

1. PointerGuidanceCommand bypasses CommandFactory — scope check duplication risk

The PR comment acknowledges this:

"Because of that bypass, the scope check in isInTraceScope duplicates CommandExecutor.isValidForScope — keep the two in sync if scope validation evolves."

This is a real maintenance hazard. If a new scope is added that should also suppress guidance, there are two places to update. Consider extracting scope validation to a shared utility or passing a ScopeValidator dependency so both paths use the same logic.

2. Histogram / Segmented / Smooth traces — findNearestPoint return type may not be updated

Every findNearestPoint override now needs to return { …, centerX: number; centerY: number }, but I don't see changes for Histogram, SegmentedTrace, or smooth-trace variants in this diff. If those classes override findNearestPoint, their return types won't match the updated NearestPoint interface, which would cause runtime undefined when moveToPointAndGetPointerGuidance reads nearest.centerX / nearest.centerY. Please confirm before merge.

3. cursorVerticalPosition naming is counter-intuitive

The field documents where the cursor is, but the pitch mapping inverts this:

• cursor below → high pitch (point is above)
• cursor above → low pitch (point is below)

A future contributor reading cursorVerticalPosition: 'below' will naturally interpret it as "play a low sound" — the opposite of intended behaviour. The inversion is documented in JSDoc, but names should not require remembering a frame inversion.

Suggested alternatives:

• pointIsAboveCursor: boolean (direct mapping to pitch intent)
• curveVerticalPosition: 'above' | 'below' (frame flipped to match audio output)

4. distancePx set unconditionally even when onCurve: true

The JSDoc says "callers should check onCurve before using this", which is a code smell. A discriminated union would make the type self-enforcing:

type PointerGuidanceState =
  | { onCurve: true }
  | { onCurve: false; distancePx: number; cursorVerticalPosition: 'above' | 'below'; cursorHorizontalPosition: 'left' | 'right' };

5. Volume-0 silently advances the throttle

// in playPointerGuidanceBeep:
if (guidanceVolume <= 0) { return; }   // returns early — no beep

// back in playPointerGuidance — always runs:
this.nextPointerGuidanceBeepAt = now + beep.interval;

When volume is 0 the user hears nothing, but the throttle still advances. After turning volume back up, they won't hear a beep until the current interval expires. Whether this is correct depends on intent — worth an explicit comment either way.

6. Guidance parameters not user-configurable

POINTER_GUIDANCE_VOLUME = 0.35 and the constants in DEFAULT_POINTER_GUIDANCE_CONFIG are hardcoded. Users who need slower feedback (e.g., motor impairments) or different pitch ranges cannot adjust them. At minimum, a TODO comment would signal intent to expose these through SettingsService.

7. activeAudioIds now holds mixed node types

The map was widened from OscillatorNode to AudioNode, and guidance beeps add [OscillatorNode, GainNode, StereoPannerNode?] entries. stopAudioNode correctly guards with 'stop' in node, so this works. Just note for future contributors that stopAll will now encounter non-oscillator nodes.

---

Bar Chart UX Gap

The comment in bar.ts explains the intentional design — bars are area marks, so no directional guidance fires between bars. This is a reasonable v1 decision, but a user hovering between bars receives complete silence with no indication they are in a chart region. Worth noting as a follow-up: a quiet "near bar chart but not on a bar" cue could improve discoverability.

---

Test Coverage

The test suite is thorough:

• pointerGuidance.test.ts covers the command's edge cases cleanly
• moveToNearestOverride.test.ts correctly tests the template method contract without a real DOM
• audio.pointerGuidance.test.ts covers rate-limiting, throttle preservation, and mode interactions — the "audio OFF does not reset throttle" case is a non-obvious edge case that is good to have locked down
• math.test.ts covers clamp / interpolate with zero-width source range

One gap: no tests for Mousebindingservice listener wiring — e.g., that pointerleave triggers reset, or that switching hover mode removes the previous listener before adding a new one.

---

Nits

• Mousebindingservice has inconsistent capitalization (b lowercase) — MousebindingService matches TypeScript conventions. Pre-existing, but easy to fix.
• The PR checklist marks "I have commented my code" unchecked, but JSDoc coverage is actually quite good throughout.

---

Summary

Category	Notes
Architecture fit	Follows MVVC; Command/Observer patterns used correctly
Test coverage	Good unit tests; gap in Mousebindingservice wiring tests
Naming clarity	cursorVerticalPosition inverts the audio frame — likely to confuse future contributors
Type safety	distancePx unconditionally set on-curve; histogram/segmented traces need verification
Accessibility	Guidance config not user-adjustable; bar chart UX gap
Subtle bugs	Volume-0 path still advances throttle

The core implementation is solid and the test coverage is commendable. Issues #2 (missing trace updates) and #3 (naming) are the most important to resolve before merge.

[claude]

Code Review — PR #587: Auditory Directional Guidance for Mouse/Pointer

Overview

This PR adds pointer-guidance sonification: when the user hovers near a data point, directional audio beeps cue them toward the nearest point (pitch = vertical, stereo pan = horizontal, beep rate = proximity). It is a meaningful accessibility enhancement.

The implementation is largely solid — good test coverage, clean type design with a discriminated union (PointerGuidanceState), and a thoughtful performance optimization (combining findNearestPoint + navigation into one call to avoid a double scan). Below are the issues worth addressing before merge.

---

Bugs / Correctness

1. Throttle is consumed even when the beep is skipped (volume = 0)

In audio.ts, playPointerGuidance advances nextPointerGuidanceBeepAt before calling playPointerGuidanceBeep. But playPointerGuidanceBeep has an early return when guidanceVolume <= 0. Result: if the user has volume at 0, the throttle slot is consumed but no beep fires — the next valid beep is silently delayed.

// In playPointerGuidance (~line 766):
this.playPointerGuidanceBeep(beep.frequency, beep.pan, now);
this.nextPointerGuidanceBeepAt = now + beep.interval; // set regardless of whether beep played

Fix: only advance the throttle when a beep was actually emitted, or move the guidanceVolume <= 0 guard into playPointerGuidance before the throttle update.

2. Scope changes mid-hover don't reset guidance throttle

PointerGuidanceCommand.execute only resets when there's no event or no coordinates. If the scope changes from TRACE to SUBPLOT while the pointer is still moving, subsequent pointermove events will no-op (scope check fails) without resetting. The throttle from the previous TRACE-scope hover persists until the next pointer-leave or on-curve event. Low-severity (no beeps fire during the stale window) but could cause an unexpected initial beep silence after re-entering TRACE scope.

---

Architecture

3. PointerGuidanceCommand bypasses CommandFactory

The command is constructed directly in Mousebindingservice. The inline comment acknowledges this and notes the scope-check duplication risk. Given the coordinates aren't expressible in the factory's current contract, the bypass is understandable — but consider whether CommandFactory could be extended with a dedicated createPointerGuidanceCommand(context, audioService) method to keep the bypass self-contained rather than leaking into the service layer.

4. ARIA live region spam during hover

When moveToNearest calls moveToIndex, the Observer chain fires — including TextService → TextViewModel → Redux → aria-live region. For bar/line charts where onCurve fires frequently as the user moves across bars, this can generate rapid screen-reader announcements that make the text modality unusable during hover.

Please confirm that moveToIndex triggered by hover suppresses the TextService announcement (or that an existing announce flag already handles this), since the PR description focuses only on the audio beep channel.

---

Performance

5. No debouncing of findNearestPoint on pointermove

pointermove can fire at 60–120 Hz. Every event calls moveToPointAndGetPointerGuidance → findNearestPoint, which is an O(n) center scan. The audio throttle prevents beep spam, but the DOM scan itself is unthrottled. For dense scatter plots this could be noticeable.

The pre-existing hover code had the same issue, so this isn't a regression — but since the PR touches Mousebindingservice it would be a good time to add a requestAnimationFrame or time-based debounce at the listener level.

---

Minor Issues

6. Comment verbosity

CLAUDE.md asks for minimal comments (only when WHY is non-obvious). Several new JSDoc blocks explain WHAT the code does rather than WHY (e.g. the hasClientCoordinates method, PointerGuidanceBeep interface fields). The substantive comments — throttle boundary-crossing rationale, Safari panner degradation, tie-break reasoning — are genuinely useful and should stay.

7. Screen-space y clarification in resolvePointerGuidanceBeep

The comment // Point above cursor → high pitch; point below → low pitch. is correct but assumes screen-space coordinates (y=0 at top). Worth a brief note since SVG can have flipped y-axes and the two don't always agree.

---

What's Good

• The PointerGuidanceState discriminated union is well-designed; type narrowing is safe and explicit throughout.
• Safari createStereoPanner degradation is correctly handled — horizontal guidance is gracefully lost rather than throwing.
• Rate-limiting via audioContext.currentTime (not Date.now) is the right choice for audio scheduling.
• The moveToNearest protected template method is a clean extension point — BoxTrace and ViolinBoxTrace override correctly without duplicating guidance logic.
• Test coverage is thorough: throttle edge cases (boundary-crossing, mode-toggle, null reset) are all exercised.
• Extracting MathUtil.clamp and MathUtil.interpolate and removing the duplicate interpolate from AudioService is a welcome cleanup.

---

Summary

Items 1 and 4 (throttle/volume correctness and ARIA announcement verification) should be resolved before merge. Item 5 (performance) is a pre-existing concern that can be tracked separately. Everything else is polish. The feature is well-motivated and the overall implementation is architecturally sound.

[claude]

Code Review: feat/auditory directional guidance (#587)

Overview

This PR adds real-time auditory directional guidance when hovering near a curve — high/low pitch encodes vertical direction, stereo panning encodes horizontal direction, and beep rate scales with distance. The implementation is well-reasoned with clear thought given to edge cases. The main concerns are architectural.

---

Architecture & Design

CommandFactory bypass (medium concern)

PointerGuidanceCommand is constructed directly in Mousebindingservice instead of going through CommandFactory / CommandExecutor. The file itself documents this:

"Mousebindingservice rather than through CommandFactory: pointer events carry coordinates the keyboard-oriented factory contract does not accept... the scope check in isInTraceScope duplicates CommandExecutor.isValidForScope — keep the two in sync if scope validation evolves."

The "keep the two in sync" note is the problem — this is a maintenance risk. If scope validation logic ever changes, it needs to be updated in two places. Consider whether CommandExecutor could be extended to accept an optional coordinate payload, or whether an alternative factoring (e.g. a separate PointerCommandExecutor) can eliminate the duplication.

Direct AudioService dependency in a Command (medium concern)

Commands in this codebase operate on Context / the model layer, then let the Observer chain handle audio. PointerGuidanceCommand takes AudioService directly, making the command responsible for audio output — a responsibility that currently belongs to services listening to model changes. The PR description acknowledges that guidance is orthogonal to data-tone sonification, but this still bypasses the established Observer chain. The tradeoff is worth calling out explicitly so future maintainers can consciously accept it.

---

Code Quality

Positive highlights

• Combining navigation and guidance into a single findNearestPoint call is a clean performance win on dense plots.
• The throttle design handles all the tricky edge cases: volume-0 beeps don't consume a slot, audio-OFF doesn't reset the throttle, out-of-range events don't reset the throttle. Each case has a dedicated test.
• The moveToNearest hook pattern lets BoxTrace and ViolinBoxTrace opt out of hover moves while still returning guidance state — clean extension point.
• Safari createStereoPanner fallback is explicitly handled and commented.
• stopAudioNode correctly wraps stop() in a try-catch since nodes may already be stopped.

Unconventional type formatting

// src/type/state.ts
export type PointerGuidanceState
  = | { onCurve: true }
    | { onCurve: false; ... };

The leading | on the first branch is non-standard in this codebase. Conventional form:

export type PointerGuidanceState =
  | { onCurve: true }
  | { onCurve: false; distancePx: number; curveVertical: 'above' | 'below'; curveHorizontal: 'left' | 'right' };

AudioNode type widening

activeAudioIds was widened from Map<AudioId, OscillatorNode | OscillatorNode[]> to Map<AudioId, AudioNode | AudioNode[]>. This is necessary to track GainNode and StereoPannerNode for cleanup, but the wider type removes compile-time guarantees across the existing callers. The new stopAudioNode helper uses 'stop' in node to guard the call, which is correct.

---

Potential Issues

Trace types not visible in the diff

NearestPoint now requires centerX and centerY. The PR updates eight trace implementations, but src/model/histogram.ts, src/model/segmented.ts, and the smooth-curve trace are not in the diff. TypeScript strict mode should catch any missing fields at compile time — please confirm npm run type-check passes and that all traces overriding findNearestPoint were updated.

Cleanup timeout vs. throttle minimum

POINTER_GUIDANCE_BEEP_DURATION is 60 ms; cleanup runs after 120 ms. The minimum throttle interval is 80 ms, so a second beep can fire before the first node's cleanup timeout runs. This is safe (each audioId is distinct), but the activeAudioIds map can hold several nodes simultaneously during rapid mouse moves. Not a bug, just worth being aware of on dense/fast hover paths.

No supplemental text cue for audio-off users

The guidance is audio-only. Users who have audio disabled (AudioMode.OFF) receive no indication of direction. This may be intentional, but worth confirming with accessibility stakeholders — even a brief ARIA live-region message or a warning notification could help.

---

Test Coverage

Excellent coverage for the new surface area:

• test/command/pointerGuidance.test.ts — all execute/reset paths including out-of-scope
• test/model/moveToNearestOverride.test.ts — no-op override, side-effect override, null nearest
• test/service/audio.pointerGuidance.test.ts — 8 scenarios covering all throttle edge cases
• test/service/pointerGuidance.test.ts — pitch/pan mapping, interval scaling
• test/util/math.test.ts — clamp/interpolate edge cases

The only gap is an integration-level test exercising the full chain (pointermove → Mousebindingservice → command → context → model → AudioService). Unit tests suffice for CI, but manual testing on each trace type before merge is important.

---

Summary

Category	Status
Architecture compliance	Partial — command bypass is pragmatic but a documented maintenance risk
Code quality	Clean, well-commented, consistent with existing patterns
Performance	Single findNearestPoint call; time-based throttle
Test coverage	Strong unit coverage; integration testing needed manually
Security	No concerns
Accessibility	Audio-only; consider supplemental cue for AudioMode.OFF users

The feature is thoughtfully implemented. The two action items before merge are: (1) confirm npm run type-check passes for all trace types, and (2) either extract the duplicated scope-check into a shared helper or accept the duplication with an explicit comment linking the two sites.

[claude]

Code Review: feat: add auditory directional guidance towards svg element with mouse

Overall Assessment: This is a well-designed and thoughtful feature. The accessibility motivation is clear, the architecture is sound, and the test coverage is strong. A few issues worth addressing before merge.

---

What This PR Does

When a user moves the mouse near a data curve, audio cues now guide them toward the nearest data point:

• High/low pitch = curve is above/below the cursor
• Left/right stereo pan = curve is to the left/right
• Beep frequency (rate) increases as the pointer gets closer
• Beeps fire only when in Scope.TRACE, and are rate-limited via a throttle

---

Strengths

• Clean discriminated union for PointerGuidanceState — the onCurve: true | false type narrows safely throughout the codebase and prevents accessing distancePx when on-curve.
• Avoids double findNearestPoint calls — combining navigation + guidance into moveToPointAndGetPointerGuidance() is the right call for a hot code path.
• Well-designed throttle — the volume=0 and out-of-range cases both correctly preserve the throttle slot (not resetting it), preventing audio bursts on re-entry.
• Safari fallback — graceful degradation when createStereoPanner is unavailable is excellent for accessibility tooling that needs broad browser support.
• moveToNearest protected hook — clean extensibility pattern; BoxTrace and ViolinBoxTrace no-op navigation while still surfacing guidance.
• MathUtil extraction — pulling clamp and interpolate into shared utilities is a good cleanup.
• Test coverage — 4 new test files with edge cases for throttle behavior, out-of-range events, volume=0, and scope guards.

---

Issues to Address

1. Missing Jest globals import in test/service/pointerGuidance.test.ts (Minor — consistency)

The file uses describe, it, and expect but never imports them from @jest/globals. Every other new test file in this PR (and math.test.ts) does the explicit import. If the Jest config has injectGlobals: false in the future, this file would silently fail.

// test/service/pointerGuidance.test.ts — add this:
import { describe, expect, it } from '@jest/globals';

2. Behavioral change at Figure/Subplot scope (Low risk, worth noting)

Figure.moveToPoint() and Subplot.moveToPoint() previously called this.notifyStateUpdate(). The replacements in AbstractPlot.moveToPointAndGetPointerGuidance() just return null. This is safe because PointerGuidanceCommand.isInTraceScope() guards all callers — these code paths are only reachable in Scope.TRACE where context.active is a Trace, not a Figure or Subplot. However, it's a silent behavioral change worth a comment to prevent future confusion if the scope guard is ever relaxed.

3. stopAudioNode catches all errors, not just "already stopped" (Very minor)

private stopAudioNode(node: AudioNode): void {
  node.disconnect();
  if ('stop' in node) {
    try {
      (node as OscillatorNode).stop();
    } catch {
      // Node may have already been stopped; safe to ignore.
    }
  }
}

The catch swallows all errors. The only error OscillatorNode.stop() throws in practice is when already stopped (InvalidStateError), so this is functionally correct. But explicitly checking e instanceof DOMException would make the defensive intent clearer:

} catch (e) {
  if (!(e instanceof DOMException)) throw e;
}

4. PointerGuidanceCommand bypasses CommandFactory — document the tradeoff in CommandFactory too

The JSDoc on PointerGuidanceCommand explains why it's constructed directly by Mousebindingservice. Consider adding a brief note in CommandFactory (or its class-level JSDoc) so readers don't wonder why this command is "missing" from the factory.

---

Observations / Questions

• Bar chart guidance behavior is intentionally limited (no directional beep from outside a bar). This design decision is well-documented inline. Is it worth surfacing this constraint in user-facing docs or settings so users know bar charts behave differently from line/scatter?

• AudioMode.COMBINED vs SEPARATE — the comment in playPointerGuidance says guidance fires in all modes except OFF. Confirming: does COMBINED mode + pointer guidance result in both data tone AND guidance beep stacking audibly? If so, is the volume balance (POINTER_GUIDANCE_VOLUME = 0.35) tuned for that case?

• Touch device behavior — the PR description mentions touchscreen pointer, and pointermove fires on touch. On touch devices pointerleave also fires on lift-off, so the throttle reset should work correctly. Worth verifying on a real touch device if possible.

---

Summary

The feature is ready for merge pending the import fix in pointerGuidance.test.ts (trivial) and a response to the questions above. The architecture is clean, the throttle logic is subtle but correct, and the accessibility motivation is well-executed.

[claude]

Code Review — PR #587: Auditory Directional Guidance for Mouse/Pointer

Overview

This PR adds real-time directional audio beeps that guide users toward the nearest data point as they hover their mouse — high pitch = point is above, right pan = point is to the right, beep frequency increases as distance decreases. This is a meaningful accessibility improvement for blind and low-vision users exploring charts non-visually.

The implementation is generally well-structured and follows MAIDR's MVVC architecture. Below is my detailed analysis.

---

What Works Well

Architecture compliance

• PointerGuidanceState as a discriminated union ({ onCurve: true } | { onCurve: false; distancePx; ... }) is clean and forces callers to handle both branches at compile time.
• resolvePointerGuidanceBeep as a pure function in its own module (src/service/pointerGuidance.ts) is excellent — it is side-effect-free and trivially testable.
• Combining navigation and guidance into a single moveToPointAndGetPointerGuidance call (avoiding a second findNearestPoint scan) is a thoughtful performance decision for dense plots.
• Throttle uses audioContext.currentTime (audio timeline) rather than Date.now() — this keeps beep timing correctly aligned with the audio scheduler.

Safari graceful degradation

const stereoPanner = typeof this.audioContext.createStereoPanner === 'function'
  ? this.audioContext.createStereoPanner()
  : null;

Horizontal guidance is silently lost on Safari < 14.5; vertical pitch still works. This is the correct tradeoff.

Test coverage
Four new test files covering command routing, model hook contracts, audio service throttle behavior, and the pure resolver function. The throttle edge-case tests (volume=0, audio-OFF, out-of-range boundary crossing) are particularly valuable.

---

Issues and Concerns

Bug Risk: SettingsService constructor argument order in tests

In test/service/audio.pointerGuidance.test.ts the service is constructed as:

new AudioService(createNotification(), createSettings().service, INITIAL_STATE)

If this order doesn't match the real AudioService constructor signature (which may be notification, state, settings based on ARCHITECTURE.md), every test in this file is silently constructing the service with state and settings swapped. Please verify the constructor order matches the production code.

---

Missing trace types: Histogram, SegmentedTrace, Smooth

The NearestPoint interface now requires centerX and centerY:

export interface NearestPoint {
  element: SVGElement; row: number; col: number;
+  centerX: number; centerY: number;
}

The following trace files do not appear in the diff:

• Histogram
• SegmentedTrace (stacked/dodged/normalized bar)
• Smooth trace

If any of these override findNearestPoint and return objects without centerX/centerY, pointer guidance will silently get undefined at runtime. TypeScript would normally catch this, but only if they aren't cast through as. Please confirm these traces are covered (either via inheritance from AbstractBarPlot or by explicit updates).

---

Throttle not cleared on dispose()

nextPointerGuidanceBeepAt persists across disposal. If a Controller is disposed while the pointer is mid-hover (which the lifecycle shows happens on focusout), the stale throttle timestamp may block the first guidance beep when a new Controller is created and the user re-enters. Consider resetting it in dispose() or in the constructor (it already initializes to 0 in the constructor, but a new AudioService is created per Controller, so this is actually fine). This is low risk but worth confirming.

---

Audio node cleanup races with audio context disposal

In playPointerGuidanceBeep:

const audioId = setTimeout(() => {
  nodes.forEach(node => node.disconnect());
  this.activeAudioIds.delete(audioId);
}, POINTER_GUIDANCE_BEEP_DURATION * 1000 * 2);

If dispose() is called (on focusout) before the 120ms cleanup timer fires, stopAll() will call stopAudioNode on these nodes — which is fine. But the setTimeout callback will still run ~120ms later and call node.disconnect() on already-disconnected nodes. Web Audio API handles duplicate disconnect() calls gracefully (they're no-ops), so this is safe, but it leaks a setTimeout reference past disposal. A minor improvement would be to track this setTimeout id and clearTimeout it in dispose().

---

Vertical direction comment is easy to misread

In moveToPointAndGetPointerGuidance:

// `y < centerY` means cursor is above, so the point is below
curveVertical: y < nearest.centerY ? 'below' : 'above',

The comment says the cursor is above when y < centerY, which is correct in screen-space (y increases downward). However, the field documents where the curve point is, not the cursor. The type-level JSDoc in state.ts handles this clearly, but the inline comment creates ambiguity. A simpler phrasing: // screen-space: smaller y = higher on screen.

---

PointerGuidanceCommand bypasses CommandFactory — document the exception

The class comment explains the rationale well, but CommandFactory has no indication that pointer guidance commands exist outside its scope. Consider adding a short comment to CommandFactory noting that PointerGuidanceCommand is wired directly by Mousebindingservice due to its coordinate-carrying contract.

---

Minor Nits

• Mousebindingservice (pre-existing) has an inconsistent capital: should be MousebindingService. Out of scope for this PR but worth a follow-up.
• POINTER_GUIDANCE_BEEP_DURATION = 0.06 and POINTER_GUIDANCE_VOLUME = 0.35 are undocumented magic numbers. Brief JSDoc comments explaining the perceptual rationale (e.g. "60ms is the shortest audible discrete beep; 35% of master keeps guidance below data tones") would help future tuning.
• The createGain: makeGain pattern in the test mock means every ctx.createGain() call returns a new gain object (since makeGain is called fresh each time). This is correct, but visually misleading — adding a comment would prevent future reviewers from thinking it's a shared singleton.

---

Test Coverage Gaps

• Mousebindingservice event listener changes: no unit tests verify that pointerleave is attached in pointermove mode but not in click mode, or that reset() is called on mode switch.
• findNearestPoint returning centerX/centerY: no tests verify the coordinate fields are populated correctly (e.g. for AbstractBarPlot.findNearestPoint where center is computed from bbox).
• Integration path: no test follows pointermove → PointerGuidanceCommand.execute → Context.moveToPointAndGetPointerGuidance → AudioService.playPointerGuidance as an end-to-end flow.

---

Summary

The feature logic is sound, the MVVC architecture is respected, and the test coverage for the new pure-function layer is strong. The primary risks are the potential constructor argument order mismatch in tests, the unverified Histogram/SegmentedTrace/Smooth trace coverage, and the leaked cleanup timeout on disposal. Please address at minimum the constructor order verification and the missing trace audit before merging.

[claude]

End-to-End Testing Guide

This document provides comprehensive information about running and writing end-to-end (e2e) tests for the MAIDR project.

Prerequisites

• Node.js 22 or higher
• npm (comes with Node.js)
• Git

Available Test Scripts

The following npm scripts are available for running e2e tests:

# Run all e2e tests
npm run e2e

# Run e2e tests with UI mode (for debugging)
npm run e2e:ui

# Run e2e tests in debug mode
npm run e2e:debug

# Install Playwright browsers and dependencies
npm run e2e:install

Test Structure

Directory Structure

e2e_tests/
├── config/
│   ├── test-config.ts      # Playwright configuration
│   └── ...                 # Additional config files
├── page-objects/
│   ├── base.page.ts        # Base page object with common methods
│   └── plots/
│       ├── barplot.page.ts
│       ├── boxplotHorizontal.page.ts
│       ├── boxplotVertical.page.ts
│       ├── scatterplot-page.ts
│       └── ...            # Additional plot page objects
├── specs/
│   ├── barplot.spec.ts
│   ├── boxplotHorizontal.spec.ts
│   ├── boxplotVertical.spec.ts
│   ├── scatterplot.spec.ts
│   └── ...               # Additional test specs
└── utils/
    ├── constants.ts       # Test constants and selectors
    └── ...               # Additional utility files

Test Categories

Each plot type's test suite includes the following categories:

1. Navigation Controls

• Basic navigation (up, down, left, right)
• First/last element navigation
• Data point navigation
• Box navigation (for boxplots)

2. Autoplay Controls

• Forward/backward autoplay
• Upward/downward autoplay (for boxplots)
• Autoplay with timeout configuration

3. Mode Controls

• Text mode toggle
• Braille mode toggle
• Sound mode toggle
• Review mode toggle

4. Plot-Specific Features

• Axis title verification
• Data point information
• Plot type verification

Writing Tests

Page Objects

Each plot type has its own page object class that extends the base page object. Page objects encapsulate:

• Element selectors
• Navigation methods
• Mode control methods
• Plot-specific methods

Example:

class BoxplotVerticalPage extends BasePage {
  async moveToDataPointBelow() {
    // Implementation
  }

  async startDownwardAutoplay(options?: { timeout?: number }) {
    // Implementation
  }
}

Test Structure

Tests are organized using Playwright's test framework:

import { expect, test } from '@playwright/test';
import { BoxplotVerticalPage } from '../page-objects/plots/boxplotVertical-page';

test.describe('Boxplot Vertical Tests', () => {
  let boxplotVerticalPage: BoxplotVerticalPage;

  test.beforeEach(async ({ page }) => {
    boxplotVerticalPage = new BoxplotVerticalPage(page);
    await boxplotVerticalPage.navigateToPlot('boxplotVertical');
  });

  test('should move to data point below', async () => {
    await boxplotVerticalPage.moveToDataPointBelow();
    // Assertions
  });
});

Best Practices

1. Page Object Pattern

• Keep selectors in page objects
• Encapsulate common operations
• Extend base page for shared functionality

2. Test Organization

• Group related tests using test.describe
• Use meaningful test descriptions
• Follow the Arrange-Act-Assert pattern

3. Error Handling

• Use try-catch blocks for expected errors
• Implement proper error messages
• Handle timeouts appropriately

4. Constants

• Use constants for selectors and text
• Keep test data in a separate file
• Use enums for mode states

Debugging Tests

UI Mode

Run tests in UI mode for visual debugging:

npm run e2e:ui

This opens Playwright's UI where you can:

• View test execution
• Debug step by step
• Inspect elements
• View test traces

Debug Mode

For console debugging:

npm run e2e:debug

Common Issues

1. Timeout Errors

• Increase timeout in test configuration
• Check for slow operations
• Verify element visibility

2. Selector Issues

• Use unique, stable selectors
• Wait for elements to be visible
• Check for dynamic content

3. State Management

• Reset state between tests
• Handle async operations properly
• Verify mode changes

Continuous Integration

E2E tests are run:

• On every pull request
• On schedule (every 2 days)
• Can be triggered manually

GitHub Actions

The workflow includes:

• Node.js setup
• Dependency installation
• Browser installation
• Test execution
• Report generation

Contributing

When adding new tests:

1. Follow the existing structure
2. Add appropriate documentation
3. Include error handling
4. Update this guide if necessary

Resources

• Playwright Documentation
• GitHub Actions Documentation
• MAIDR Project Documentation

[claude]

Code Review — PR #587: Auditory Directional Guidance for Pointer Exploration

Overview

This PR adds real-time auditory guidance beeps that direct the user toward the nearest data point as they move the mouse over a chart. High pitch = curve is above cursor; low pitch = below; stereo pan pulls toward the horizontal direction. The beep rate increases as the cursor approaches the curve. A solid accessibility feature with good fundamentals.

---

Architecture & Design

Strengths

• Correctly follows MVVC: model returns guidance state, command wires context to audio service, service handles side effects. No layer boundary violations.
• The moveToNearest template-method hook is a clean solution — BoxTrace/ViolinBoxTrace no-op the navigation while still surfacing guidance, and ScatterTrace pre-sets its nav mode before delegating. Avoids duplicating findNearestPoint per pointer event.
• The discriminated union PointerGuidanceState is well-designed and makes illegal states unrepresentable.
• Safari createStereoPanner graceful degradation is thoughtful.
• Throttle design is correct: volume=0 and audio-OFF do not consume a throttle slot; out-of-range events do not reset it (prevents beep-on-every-boundary-crossing).
• Proper resource cleanup: guidance beep nodes tracked in activeAudioIds, clearTimeout called on dispose.

Concerns

1. AudioService responsibility creep — The class now plays data tones, warning tones, zero tones, sweep tones, and guidance beeps. Each addition is individually justified (shared AudioContext, compressor, activeAudioIds), but the class is growing large. Consider extracting a GuidanceAudioChannel in a follow-up.

2. PointerGuidanceCommand bypasses CommandFactory — Correctly documented and justified (coordinates cannot fit the keyword-based factory contract), but the command is not covered by factory scope validation. The explicit isInTraceScope() check compensates. A comment in CommandExecutor linking to this workaround would help future developers.

---

Potential Bugs / Issues

1. histogram.ts not updated

All other trace types that override findNearestPoint have been updated to include centerX/centerY. If HistogramTrace overrides findNearestPoint independently (rather than inheriting from AbstractBarPlot), it will return the old type. TypeScript should catch this at build time, but worth confirming explicitly before merge.

2. Vertical direction comment is misleading

In abstract.ts (moveToPointAndGetPointerGuidance): The comment reads as if the point has y < centerY, but y is the cursor position. Suggested rewrite: Screen-space y grows downward. When cursor y < centerY, the cursor is higher on screen than the nearest point — the curve point sits below.

3. Silent null return in Context for non-trace scopes

Old Figure.moveToPoint called this.notifyStateUpdate() — a side effect that is correctly removed. The new base moveToPointAndGetPointerGuidance returns null for non-trace contexts. This is correct, but a comment in Context.moveToPointAndGetPointerGuidance noting the invariant (only traces return non-null) would help future readers.

---

Code Quality

Minor issues

• Mousebindingservice (lowercase s) is inconsistent with codebase naming. Pre-existing; worth fixing if this class gets further work.
• POINTER_GUIDANCE_BEEP_DURATION and POINTER_GUIDANCE_VOLUME in audio.ts live outside DEFAULT_POINTER_GUIDANCE_CONFIG. Moving them in would make all guidance parameters overridable in tests without additional mocking.
• In stopAudioNode, the cast (node as OscillatorNode).stop() works but AudioScheduledSourceNode is the interface that actually declares stop() — a more accurate cast, though both compile fine.

---

Test Coverage

The test suite is genuinely strong:

Test file	What it covers
pointerGuidance.test.ts (command)	Scope filtering, reset-on-leave, navigate-only path
moveToNearestOverride.test.ts	Template-method contract for BoxTrace / ViolinBoxTrace
audio.pointerGuidance.test.ts	Rate-limiting, mode toggle, volume=0, boundary-crossing invariants
pointerGuidance.test.ts (service)	Resolver: null cases, direction mapping, interval scaling
math.test.ts	clamp / interpolate edge cases including zero-width range

One gap: no test for Mousebindingservice.update() switching hover modes and clearing guidance state. The path is covered transitively (registerEvents -> removeEventListeners -> reset()), but an explicit test would pin the invariant.

---

Summary

The implementation is solid and the test coverage is substantially better than average for a feature this size. Main items to address before merging:

1. Verify histogram.ts has centerX/centerY in its findNearestPoint return, or confirm it inherits AbstractBarPlot which is already updated.
2. Fix the vertical-direction comment in abstract.ts to avoid reader confusion.
3. (Optional) Move POINTER_GUIDANCE_BEEP_DURATION / POINTER_GUIDANCE_VOLUME into the config object for testability.

Everything else is minor or informational. Great feature addition for non-visual chart exploration.

[claude]

(stale comment — see review below)