fix: improve scatter plot grid navigation

[nk1408]

Pull Request

Description

1. Grid autoplay - Now traverses left→right, row by row (like heatmap)
2. Grid braille - Shows 2D grid of point counts per cell
3. Grid boundaries - Use same sound/text as regular boundaries
4. Boundary panning - Audio pans correctly left/right based on grid position

Changes Made

src/model/scatter.ts Fixed grid autoplay traversal, added grid braille support (2D point counts), unified boundary handling with notifyOutOfBounds(), added outOfBoundsState override for correct audio panning

src/service/braille.ts Registered SCATTER type with HeatmapBrailleEncoder for grid mode

src/service/rotor.ts Simplified moveGrid() to delegate boundary handling to trace

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.

[claude]

Code Review

Overall this is a well-scoped PR with clear intent. The unification of boundary handling through notifyOutOfBounds() and the 2D grid braille support are solid improvements. A few issues worth addressing:

---

Bug / Logic Issues

1. Stale comment in moveOnceInGridMode (scatter.ts)

// Grid movement methods already call notifyStateUpdate() or notifyRotorBounds()
return moved;

The grid methods now call notifyOutOfBounds(), not notifyRotorBounds(). This comment will mislead future contributors.

2. Missing default case in moveOnceInGridMode switch

The switch covers the four known directions but has no default branch. If a new MovableDirection variant is ever added, moved silently stays false and the method returns false with no notification and no state update. At minimum a defensive log or assertion would help.

---

Code Quality

3. Unsafe as BrailleState cast (scatter.ts line 243)

// Normal row/col mode: braille not supported (return empty state)
return this.outOfBoundsState as BrailleState;

When not in grid mode, super.outOfBoundsState returns a TraceEmptyState, which is structurally a valid member of the BrailleState union — so the intent is correct. But the as assertion bypasses TypeScript's type checker. A cleaner approach is to return super.outOfBoundsState directly without the cast, or inline the empty state object.

4. Braille grid rebuilt on every access

The braille getter rebuilds the full gridValues 2D array (iterating all grid cells) on every access. Since BrailleService.update() already caches the encoded output by id, this is not critical — but the grid values object itself is re-allocated on every state update. If grid dimensions are large this adds GC pressure. Caching gridValues as a field that's invalidated when the grid changes, or computing it once during initialization, would help.

---

Behavioral Change to Review

5. moveGrid in rotor.ts now always returns null on boundary

Previously when a boundary was hit, moveGrid returned an error string that callers used to display a text message. Now it always returns null (boundary feedback delegated to notifyOutOfBounds()). The JSDoc was updated, and the PR description calls this out as intentional ("Grid boundaries - Use same sound/text as regular boundaries"), which is fine. Just worth verifying the UX is consistent — confirm that notifyOutOfBounds() produces the same perceived feedback as the removed getMessage() path did for the grid boundary case.

---

Positive Notes

• Removing the console.warn in moveOnce and replacing it with actual grid-mode handling is the right fix — the old warning was a sign of a routing problem, not a valid guard.
• The outOfBoundsState override for correct audio panning in grid mode is well-documented and cleanly implemented.
• Registering SCATTER with HeatmapBrailleEncoder is safe — BrailleService.update() guards against empty braille states before encoding, so non-grid mode (which returns empty: true) correctly skips the encoder.
• The isMovable grid mode additions are consistent with the rest of the method.

[claude]

Code Review

Overall, this is a solid improvement to scatter plot grid navigation. The changes are focused, the intent is clear, and the approach of reusing HeatmapBrailleEncoder for grid braille output is a pragmatic choice. A few items worth discussing before merging.

Issues

1. Type cast to BrailleState in non-grid mode (scatter.ts:243)

outOfBoundsState is a TraceState with fields like type, traceType, and audio that do not exist on BrailleState. This cast works in practice because consumers gate on empty: true, and the same pattern is used in the highlight getter. However, non-grid scatter braille previously returned empty: false with actual values. The comment explaining braille is unsupported in non-grid mode is helpful, but a note on why the old values were wrong/unused would make the intent more durable for future readers.

2. Rotor boundary message suppressed (rotor.ts)

Before this PR, hitting a grid boundary returned an error string from moveGrid() → moveUp/Down/Left/Right() → dispatched via setValue() to the rotor Redux slice. Now moveGrid() always returns null, so the rotor slot gets null at boundary.

The PR description correctly states the intent is "use same sound/text as regular boundaries", and notifyOutOfBounds() does handle audio/text through the observer chain. But there is a subtle channel difference: setValue(null) dispatches to the rotor navigation slice, while notifyOutOfBounds() fires through text/audio services. If any consumer relied on the rotor slot having a non-null boundary message, that feedback is now silently dropped. Worth verifying rotor slot UI behavior at grid boundaries after this change.

3. moveOnceInGridMode has no default branch (scatter.ts:498-514)

MovableDirection is exhaustively covered so TypeScript will not complain. But a default: return false guard would be safer against future direction additions and makes the intent explicit.

Minor Observations

4. Autoplay description vs. behavior

The PR description says "Now traverses left→right, row by row (like heatmap)". The implementation correctly enables autoplay in grid mode — previously moveOnce would exit grid mode, breaking autoplay entirely. However, autoplay calls isMovable(direction) and stops when it returns false — it does not wrap to the next row automatically. The traversal is left→right until the right edge, then stops. If row-by-row wrapping is the actual goal, that would require additional work. If "left→right in the current row" is the correct intent, the PR description should be updated to avoid confusion.

5. No test coverage for new behavior

There are no existing scatter plot e2e tests (scatter.spec.ts does not exist), and none are added by this PR. The three core behaviors changed — grid autoplay, grid braille output, and unified boundary notifications — would each benefit from test coverage:

• Grid autoplay stops at boundary without exiting grid mode
• Braille in grid mode shows point counts as a 2D grid
• Boundary audio/text in grid mode uses grid coordinates for panning

What Works Well

• Removing the console.warn + forced setGridMode(false) in moveOnce is the right fix. The old code was a defensive hack that masked the real problem.
• Reusing HeatmapBrailleEncoder for the grid point-count representation is pragmatic and keeps the braille layer changes minimal.
• The outOfBoundsState override using gridRow/gridCol for audio panning is the correct approach — the parent row/col would give wrong panning in grid mode.
• Adding grid boundary checks to isMovable() properly integrates with the autoplay service without requiring changes to AutoplayService.
• The notifyRotorBounds → notifyOutOfBounds unification reduces special-casing and makes boundary handling consistent.

[claude]

Code Review (feat: add row-col navigation within a grid)

This review covers the third commit introducing grid cell point navigation (enter/exit a cell to navigate individual points). The first two commits were already reviewed. The overall approach is sound — reusing the COL mode text pattern for cell-level navigation is a good design decision. A few issues to address:

Bugs / Logic Issues

1. enterGridCell() does not reset on re-entry

If enterGridCell() is called while already in cell mode (e.g. from a future caller bypassing the context guard), the existing cellXPoints / cellSvgGroups / cellPointIndex are silently kept while new data is appended. The guard in Context.enterGridCell() prevents re-entry for now, but ScatterTrace.enterGridCell() itself does not reset state before rebuilding arrays. Move the reset logic (cellXPoints = [], cellSvgGroups = [], cellPointIndex = 0) to the top of enterGridCell(), before populating the arrays — this makes the method safe to call independently of the context guard.

2. Floating-point X equality in enterGridCell()

ScatterPoint.x is number, so two points at the same X value may compare unequal if computed via floating-point arithmetic. If X values come directly from raw JSON without mutation this is typically safe, but it is a fragile assumption. Consider grouping by String(point.x) or using an epsilon comparison, matching how other scatter grouping in the codebase handles X identity.

3. Redundant !this.gridCells guard in moveCellPointLeft / moveCellPointRight

isInGridCellMode can only be true when gridCells is non-null (enforced by enterGridCell guard). The !this.gridCells check is therefore unreachable — removing it would make the guard intent clearer without any behavioral change.

Code Quality

4. ENTER_GRID_CELL key binding fires for all trace types in TRACE scope

ENTER_GRID_CELL is added to TRACE_KEYMAP, so Enter is now consumed in TRACE scope for every trace type — bar charts, line charts, etc. On non-grid traces it no-ops silently (context guard returns early), but the keypress is eaten, blocking any future Enter handling for other trace types. Consider either registering this binding only when the active trace is in grid mode, or placing it in a dedicated GRID_MODE scope that is toggled when scatter grid mode activates.

5. Public accessors with no external callers

getCellPointIndex(), getCellPointCount(), and getCurrentCellPoint() are public on ScatterTrace but unused outside the class and absent from GridNavigable. If they exist only for future use, add a comment; otherwise remove them until needed. Untested public API surface makes refactoring harder.

6. Stale comment still present (scatter.ts)

The comment // Grid movement methods already call notifyStateUpdate() or notifyRotorBounds() was flagged in the earlier review. The methods call notifyOutOfBounds(), not notifyRotorBounds(). Still unchanged in this commit.

Minor Observations

• The Cell [row,col] format string is inline in multiple places (buildScatterText, formatVerboseGridText, formatTerseGridText). Extracting it to a named constant prevents future divergence.
• TextState.gridPosition JSDoc should note it is 1-indexed; currently only getGridPosition() carries that note.
• Context.exitGridCell() unconditionally returns to Scope.TRACE. This is correct for current call sites but the assumption is implicit — a comment noting the expected return scope would help.

What Works Well

• The Command pattern is applied correctly: four atomic commands, minimal logic, delegates to context.
• Grouping cell points by X (mirroring COL mode) is the right analogy and produces consistent text/audio with the rest of the trace.
• Context.enterGridCell() correctly guards against entry on incompatible traces (isGridNavigable && supportsGridMode), preventing spurious scope switches.
• The GRID_CELL scope is cleanly isolated in SCOPED_KEYMAP, and using esc to exit is consistent with other modal scopes in the codebase.

[claude]

Code Review

Overall this PR makes solid improvements to scatter grid navigation — the autoplay traversal fix, unified boundary handling, and grid braille support are all good changes. A few issues worth addressing before merge:

---

Bugs / Correctness

1. ENTER_GRID_CELL fires misleading notification on non-grid traces (src/command/gridCell.ts:19-21, src/service/keybinding.ts:227)

The Enter key is mapped in TRACE_KEYMAP (active for all traces), so pressing Enter on a bar chart, line chart, etc. will call context.enterGridCell(), which returns false for non-grid traces, and EnterGridCellCommand then notifies "No points in this cell" — a confusing message on a trace that has nothing to do with grids.

Either guard the notification with a grid-mode check, or move ENTER_GRID_CELL out of the global TRACE_KEYMAP (e.g., only bind it dynamically when in scatter grid mode):

// Option A: guard the notification
public execute(): void {
  const activeTrace = this.context.active;
  if (!isGridNavigable(activeTrace) || !activeTrace.supportsGridMode()) return;
  const success = this.context.enterGridCell();
  if (!success) {
    this.notification.notify('No points in this cell');
  }
}

2. Unsafe cast outOfBoundsState as BrailleState (src/model/scatter.ts, braille getter)

outOfBoundsState is a TraceState ({ empty: true, type: 'trace', traceType, audio }), not a BrailleState. The cast is technically fine today because the braille service checks empty: true, but it bypasses TypeScript's type system and will silently break if the braille encoder's guard logic changes. Return a proper empty BrailleState instead:

// Normal row/col mode: braille not supported
return { empty: true };

3. isInGridCellMode may not reset on setGridMode(false) (src/model/scatter.ts)

setGridMode(false) is called in several paths (e.g., from the rotor when switching modes). If isInGridCellMode is not reset there, the trace will be in an invalid state where isInGridCellMode === true but isInGridMode === false, causing unexpected audio/text/highlight output. Verify (or add) a reset in setGridMode:

setGridMode(enabled: boolean): void {
  this.isInGridMode = enabled;
  if (!enabled) {
    this.isInGridCellMode = false;
    this.cellPointIndex = 0;
    this.cellXPoints = [];
    this.cellSvgGroups = [];
  }
  ...
}

4. SVG element alignment in enterGridCell (src/model/scatter.ts:~814)

const pointsWithSvg = cell.points.map((p, i) => ({ point: p, svg: cell.svgElements[i] }));

This assumes cell.svgElements is parallel to cell.points. If those arrays diverge (e.g., some points lack SVG elements), svg will be undefined and points are silently dropped from cellSvgGroups. Consider logging a warning when lengths differ.

---

Design Concerns

5. HeatmapBrailleEncoder registered globally for SCATTER (src/service/braille.ts:764)

HeatmapBrailleEncoder is now registered unconditionally for all scatter plots. In normal (non-grid) scatter mode, the braille state returns { empty: true }, so it won't produce visible braille — but the encoder will still be invoked and need to gracefully handle empty: true. Confirm the HeatmapBrailleEncoder does this, or add a guard. Also consider whether SCATTER should fall through to a different encoder in normal mode (e.g., a point-by-point representation).

6. exitGridCell always returns to Scope.TRACE (src/model/context.ts:267)

this.toggleScope(Scope.TRACE);

If the implementation of toggleScope uses a stack pop or toggle rather than a hardcoded set, this is fine. But if GRID_CELL was pushed on top of a scope other than TRACE (unlikely today but possible with future scopes), this would be wrong. A safer pattern would be to save the pre-entry scope and restore it on exit, similar to how braille mode works.

---

Minor

7. moveOnceInGridMode is dead code when called from moveOnce — the rotor already calls moveGridUp/Down/Left/Right directly. moveOnce being called in grid mode would only happen during autoplay. The comment // Grid movement methods already call notifyStateUpdate() is helpful, but worth adding a note that this path is specifically for autoplay.

8. No tests added — 503 additions with no test coverage for the new grid cell navigation, enterGridCell/exitGridCell state transitions, or the outOfBoundsState override. The scatter model unit tests should cover at minimum: entering a cell, navigating left/right, exiting, and the boundary notification behavior.

---

Summary

Autoplay grid traversal fix	✅ Clean
notifyOutOfBounds() unification	✅ Good simplification
Grid braille 2D point counts	✅ Correct approach
outOfBoundsState panning override	✅ Necessary and well-scoped
ENTER_GRID_CELL on all traces	⚠️ Misleading notification
Unsafe as BrailleState cast	⚠️ Fix type safety
isInGridCellMode reset on mode exit	⚠️ Verify/add reset
Test coverage	❌ Missing

[xabilitylab]

🎉 This PR is included in version 3.60.1 🎉

The release is available on GitHub release

Your semantic-release bot 📦🚀