Feat point nav scatter

[ellvix]

Pull Request

Description

Adds 2 rotor modes:

• Point navigation. Arrow keys now move point to point, then wrap to the next row/col until they reach a corner.
• Intersection mode. Just like multiline, your current set (of row/col) now is its own little thing that you can explore with left/right arrow keys.

Related Issues

Fixes #610

[claude]

Code Review — PR #617: Point Navigation & Intersection Mode for Scatter

Overview

This PR adds two new rotor modes to ScatterTrace:

1. POINT_MODE — individual point-by-point navigation in reading order (left/right) and column-major order (up/down)
2. INTERSECTION_MODE — scatter-specific extension of the line intersection rotor; walks stacked points at a shared x (COL) or y (ROW) coordinate

The architecture is well-aligned with MAIDR's MVVC pattern. The PointNavigable interface mirrors GridNavigable; the AbstractTrace hook (setIntersectionMode) mirrors the existing opt-in API. Tests are included. The core ideas are sound. A few concrete issues follow.

---

Bugs

1. panY = 0 in COL intersection audio state (likely bug)

In scatter.ts (intersection audio getter, COL branch):

const panY = isCol ? 0 : this.row;

panning.y drives the position announced by AnnouncePositionCommand ("${y + 1} of ${rows}"). Hardcoding panY = 0 means every step through the stack is announced as "position 1 of N", regardless of where the user actually is. It should be idx (the current stack position):

const panY = isCol ? idx : this.row;

ROW mode uses this.row correctly for a comparable purpose.

---

2. Mode transition — new mode enabled before old one is cleared

In rotor.ts toggleMode():

this.notifyGridMode(currMode === Constant.GRID_MODE);
this.notifyPointMode(currMode === Constant.POINT_MODE);       // may set isInPointMode = true
this.notifyIntersectionMode(currMode === Constant.INTERSECTION_MODE); // clears isInIntersectionMode

When transitioning from INTERSECTION_MODE to POINT_MODE, both isInIntersectionMode and isInPointMode are true during the gap between the two calls. No notifyStateUpdate() fires between them (by design), so this doesn't cause an observable output error right now. But it is a latent correctness risk. The safer order is: disable old modes first, then enable the new one:

// Clear all first
this.notifyGridMode(false);
this.notifyPointMode(false);
this.notifyIntersectionMode(false);
// Then enable the active one
this.notifyGridMode(currMode === Constant.GRID_MODE);
this.notifyPointMode(currMode === Constant.POINT_MODE);
this.notifyIntersectionMode(currMode === Constant.INTERSECTION_MODE);

---

3. Floating-point equality in computeEntryPointIndex and xValues.indexOf

// scatter.ts
const idx = this.flatPoints.findIndex(p => p.x === targetX && p.y === targetY);
const xIndex = this.xValues.indexOf(point.x);
const panX = isCol ? ... : Math.max(0, this.xValues.indexOf(value));

All three use exact === equality on number values that may come from user-supplied floating-point data. If any value was computed (e.g. 1.0000000000000002 vs 1), indexOf / findIndex will silently fall back to index 0, producing wrong panning and a misidentified seed point. Since flatPoints and xValues are built from the same data array in the constructor, most cases will be fine — but it is brittle. At minimum, add a defensive comment; better would be storing original indices alongside coordinates so lookups are O(1) and exact.

---

Test Coverage Gap

POINT_MODE has no unit tests. test/model/scatter.test.ts only covers INTERSECTION_MODE. There is no coverage for:

• setPointMode(true/false) enabling/disabling
• movePointLeft, movePointRight, movePointUp, movePointDown navigation
• Boundary conditions (attempting to move past the first/last point)
• computeEntryPointIndex seeding from COL vs ROW base mode
• Rotor service routing (movePoint) in test/service/rotor.test.ts

This is the larger of the two new features and should have at least the same level of test coverage as intersection mode.

---

Style / Architecture Notes

4. Inconsistent type-guard pattern for intersection mode notification

notifyPointMode uses the correct interface-based guard:

if (isPointNavigable(activeTrace)) { ... }

notifyIntersectionMode uses a class-based check:

if (activeTrace instanceof AbstractTrace) { ... }

Since setIntersectionMode is defined on AbstractTrace itself (as a no-op default), every trace already has it and the instanceof check is redundant. Either call it unconditionally (since it's always a no-op on non-scatter traces), or introduce an IntersectionNavigable interface to match the PointNavigable pattern consistently.

---

5. Breaking change to AudioState.panning.x

// src/type/state.ts
x: number | number[];  // was: number

This widens a public interface type. collapsePanning() in audio.ts and the array check in describe.ts handle the new shape correctly. A brief audit of any other consumers of AudioState.panning.x (e.g. other services, tests, or external usages) would confirm the change is fully handled.

---

Minor

• The comment "Right/Left walk readingOrder (y desc, x asc)" on the readingOrder field is correct but becomes confusing next to movePointUp (which steps backward in columnOrder because higher y = lower index). Consider: "columnOrder is x asc, y desc; 'up' (higher y) = lower index = delta -1" — the current comment in movePointUp already explains this, so it is not a blocker.

• In test/model/scatter.test.ts, the test that checks NavMode is not toggled by setIntersectionMode reaches into private state with (trace as unknown as { mode: string }).mode. This is acceptable for now but would be more robust if tested via observable behavior (e.g., checking which y-stack is walked after navigation).

---

Summary

Item	Severity
panY = 0 in COL intersection audio	Bug — wrong position announcement
Mode transition order (transient dual-mode state)	Low-risk but worth fixing
Float equality in point lookups	Robustness concern
No POINT_MODE unit tests	Missing coverage for the larger new feature
instanceof vs interface guard inconsistency	Style / minor
panning.x type widening audit	Due diligence

The implementation is well-structured and the intersection mode is solid. Addressing the panY bug and adding POINT_MODE tests are the priorities before merge.