fix: address violin plot horizonatal orientation bug

[nk1408]

Pull Request

Description

Horizontal violin KDE support (src/model/violin.ts)

• Added Orientation field and full horizontal navigation support
• Horizontal: Up/Down switches violins, Left/Right traverses curve (reversed from vertical)
• Orientation-aware isMovable(), moveOnce(), moveToExtreme()
• Audio panning swapped for horizontal; autoplay direction mapping overridden
• Text getter uses correct axis labels (mainAxis/crossAxis) per orientation
• Reversed points and highlight selectors for horizontal to match box layer ordering
• Cached reference violin audio values at construction (perf: avoids Math.min/max on ~200-element arrays per tick)
• Removed onSwitchFrom() — layer switching now handled generically via moveToXAndYValue()

Horizontal violin box cleanup (src/model/violinBox.ts)

• Added mainAxis/crossAxis to text getter for correct axis formatting
• Removed outlier sections (LOWER_OUTLIER/UPPER_OUTLIER) — violin plots don't produce outliers
• Removed onSwitchFrom() — same generic switching path
• Simplified selector mapping (no outlier elements)

Autoplay fix (src/model/abstract.ts, src/service/autoplay.ts)

• Changed autoplay getter from private to protected so ViolinKdeTrace can override it with orientation-aware direction mapping
• Added orientation to TraceState for instruction text

Layer switching (src/service/navigation.ts, src/model/plot.ts)

• Removed onSwitchFrom from Trace interface — Y-value preservation now works generically through getCurrentYValue()/moveToXAndYValue() which both violin traces
  implement

Orientation in instructions (src/model/context.ts, src/maidr-component.tsx)

• Instruction text now includes orientation: "This is a maidr plot of type: horizontal box."
• Added formatPlotType() helper — safely returns just the type name when orientation is absent (no extra spaces/artifacts)
• Applied to both initial instruction (pre-focus) and runtime instruction (post-focus)
• Added orientation?: Orientation to TraceState (src/type/state.ts)

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]

PR Review: Horizontal Violin Plot Orientation Support

Overall, this is a solid PR that addresses a real gap in horizontal violin plot support. The architectural improvements (removing onSwitchFrom in favor of a generic getCurrentYValue/moveToXAndYValue path, caching audio reference values) are clean wins. A few issues worth addressing before merge:

---

Issues

1. formatPlotType is duplicated (maidr-component.tsx and context.ts)

The same helper is defined identically in two files. This is a DRY violation — if the logic changes once, the other copy will drift. Extract it to a shared utility (e.g., src/util/format.ts) and import from both locations.

// src/util/format.ts
export function formatPlotType(plotType: string, orientation?: Orientation | string): string { ... }

2. Redundant string literal checks in formatPlotType

if (orientation === Orientation.HORIZONTAL || orientation === 'horz') {

If Orientation.HORIZONTAL === 'horz', this condition is always a duplicate check. If they differ, the string literal is an undocumented magic value. Either rely solely on the enum values, or document why raw strings must also be handled here (e.g., legacy data). Mixing both signals uncertainty about what orientation can actually contain at runtime.

3. highlightValues?.reverse() mutates in place

this.highlightValues = this.mapToSvgElements(layer.selectors as string[]);
if (this.orientation === Orientation.HORIZONTAL) {
  this.highlightValues?.reverse();  // ← in-place mutation
}

Array.prototype.reverse() mutates the original array. This is inconsistent with the pattern used one line above for points:

this.points = [...(layer.data as ViolinKdePoint[][])].reverse();  // ← new array ✓

Fix for consistency and safety:

if (this.orientation === Orientation.HORIZONTAL) {
  this.highlightValues = this.highlightValues ? [...this.highlightValues].reverse() : null;
}

4. if (this.isInitialEntry) { this.isInitialEntry = false; } is a no-op branch

In moveToXAndYValue() (both violin.ts and violinBox.ts):

if (this.isInitialEntry) {
  this.isInitialEntry = false;
}

The conditional adds no value — it unconditionally sets the flag to false regardless of its prior value. Simplify to:

this.isInitialEntry = false;

5. MIN_SPEED change (50ms → 10ms) is unrelated and potentially disruptive

-const MIN_SPEED = 50;
+const MIN_SPEED = 10;

This changes global autoplay behavior for all plot types, not just violin. At 10ms between movements the autoplay will be extremely fast — likely imperceptible for audio. If this is intentional, it deserves its own PR with a rationale. If it's a side effect of working on the violin feature, it should be reverted here.

6. showOutliers removal from ViolinOptions is a breaking change

-  /** Show outlier points. Default: true */
-  showOutliers?: boolean;

If any existing callers pass showOutliers: true/false in their violin configuration, removing this field silently discards their setting (TypeScript will warn but won't block). If outliers are genuinely not supported for violin plots, this removal is architecturally correct — but it should be documented clearly in the PR description as a breaking change and ideally guarded by a deprecation notice or migration path.

---

Minor Suggestions

• autoplay getter visibility change (private → protected) is necessary for the ViolinKdeTrace override, and the use of method overriding here is the right pattern. Just make sure the base class contract is documented: subclasses overriding autoplay should return counts matching their actual navigation dimensions.

• Navigation symmetry in ViolinKdeTrace.isMovable() and moveOnce(): The horizontal and vertical branches are structurally symmetric but fully duplicated. This is acceptable given the logic is small, but a swapRowCol() helper or parameterizing by (rowAxis, colAxis) would cut the code in half if more divergence is expected later.

---

Positives

• Removing onSwitchFrom in favor of the generic getCurrentYValue/moveToXAndYValue path is a clear improvement — it follows KISS and avoids trace-type coupling in NavigationService.
• Caching refSafeMin/refSafeMax at construction time is a correct performance optimization.
• The speedUp fix (> → >=) is correct — the old code prevented userSpeed from ever reaching minSpeed.
• Text state using mainAxis/crossAxis for orientation-aware labeling is the right abstraction.
• Comments explaining navigation conventions (vertical vs horizontal) are helpful.

---

Please address items 1–5 before merging. Item 6 needs a decision on whether it's intentional.

[xabilitylab]

🎉 This PR is included in version 3.55.1 🎉

The release is available on GitHub release

Your semantic-release bot 📦🚀