feat: grid based movement for scatterplots

[ellvix]

Pull Request

Description

Introduces a new navigation mode for scatter plots that divides the plot area into a configurable grid of rectangular cells. Users cycle to "GRID NAVIGATION" via the rotor (Alt+Shift+Up/Down), then use arrow keys to move between cells. Each cell announces its axis ranges and the points it contains.

Related Issues

#529

Changes Made

• Grid config lives in the axes schema (axes.x.min, axes.x.max, axes.x.tickStep, etc.) with dual-format support (axes.min.x also works)
• Rotor modes are now dynamic per trace type — scatter shows "ROW AND COLUMN NAVIGATION" + "GRID NAVIGATION", other plots show "DATA POINT NAVIGATION" + "LOWER/HIGHER VALUE NAVIGATION"
• Text announcements for grid cells show axis ranges and point coordinates in both verbose and terse modes
• Position notification (P key) shows axis ranges when in grid mode
• Visual highlighting works for all points within the active grid cell

[claude]

Code Review: Grid Navigation for Scatter Plots

Nice feature overall — the design follows MVVC well and the implementation is readable. Here are the findings, grouped by severity.

---

Bugs / Correctness Issues

1. Rotor and trace state go out of sync when normal navigation is used in grid mode (scatter.ts:412)

public moveOnce(direction: MovableDirection): boolean {
  // Exit grid mode when normal navigation resumes
  this.isInGridMode = false;
  ...
}

RotorNavigationService still reports GRID_MODE after this silent reset, so isGridMode() returns true while the trace has already exited. The next rotor key-press (up/down) will call moveGrid() on a trace that thinks it's not in grid mode. You likely need to call context.setRotorEnabled(false) or cycle the rotor back to data mode from within RotorNavigationService whenever a trace arrow key is pressed during GRID_MODE.

2. getMessage('grid', direction) produces grammatically odd copy (rotor.ts:1488)

return this.getMessage('grid', dirLabel);
// → "No grid value found to the right of the current value."

getMessage was written for lower/higher nav types; grid reads awkwardly. Consider a dedicated boundary string like "Edge of grid reached." instead of reusing getMessage.

---

Architecture Concerns

3. Grid methods on AbstractPlot pollute every trace type (abstract.ts:741–782)

Adding setGridMode, supportsGridMode, moveGridUp/Down/Left/Right as no-ops on AbstractPlot means every bar, box, line, etc. silently carries these methods. A GridNavigable interface (implemented only by ScatterTrace) would be cleaner, and RotorNavigationService could check activeTrace instanceof GridNavigable instead of activeTrace.supportsGridMode().

4. Stale/duplicate JSDoc before supportsCompareMode() (abstract.ts:724–731)

/**
 * Sets whether grid navigation mode is active. Override in traces that support grid navigation.
 */
/**
 * Returns true if this trace supports compare (lower/higher value) navigation.
 */
public supportsCompareMode(): boolean {

Two consecutive JSDoc blocks — the first one belongs to setGridMode but ended up before supportsCompareMode. This should be resolved.

5. Grid mode detection via state shape rather than explicit flag (text.ts:1502, describe.ts:668)

Both TextService and AnnouncePositionCommand detect grid mode by checking state.gridPoints !== undefined. This is implicit coupling. Consider adding an explicit isGridCell?: boolean to TextState or creating a discriminated union to make the intent clear without relying on optional field presence.

6. Two axis formats add schema complexity (grammar.ts:AxisConfig, AxisGridProperty)

Format A (axes.x = {label, min, max, tickStep}) and Format B (axes.min.x) solve the same problem. Supporting both in resolveGridConfig adds parsing branching that will need to be maintained indefinitely. Unless Format B is required for backward compatibility with existing users, dropping it in favor of Format A alone would simplify the type definitions and the resolution logic.

---

Minor Issues

7. rotorIndex can silently address a different mode after a trace switch

The clamp comment (// Clamp index in case modes list changed between cycles) acknowledges a real edge case: if the user switches subplots mid-rotor, the index might point to a different mode for the new trace. Consider resetting rotorIndex = 0 whenever the active trace changes.

8. buildGridCells silently discards SVG highlighting when count mismatches (scatter.ts:1176)

const hasElements = svgClones.length === data.length;

When svgClones.length !== data.length, all grid cells get empty svgElements and highlighting is silently disabled. A console.warn would help developers catch data/selector mismatches during integration.

9. Floating-point binning in computeGridSteps may not cover all edge cases (scatter.ts:1109)

Math.round(binMin * 1000) / 1000 handles 3 decimal places, but tick steps with more fractional digits (e.g. 0.333) may still produce floating-point gaps. Consider using a small epsilon in findGridBin comparisons, or rounding to a precision derived from the tick step itself.

---

Missing Tests

The new logic in scatter.ts — resolveGridConfig, computeGridSteps, findGridBin, buildGridCells — has no unit tests. These are pure functions with clear inputs/outputs and are good candidates for fast unit tests (edge cases: empty data, points on bin boundaries, tick step that doesn't divide evenly into range, missing grid config).

---

Summary

Category	Count
Bugs / correctness	2
Architecture	4
Minor	3
Missing tests	1

The grid navigation concept is solid and the main data-flow follows MVVC correctly. The primary concerns to address before merge are the rotor/trace sync issue (#1) and the abstract class pollution (#3). Everything else is advisory.