feat: add intersecting point rotor mode for multiline plots

[soundaryakp1999]

Pull Request

Description

Adds a new rotor unit, INTERSECTING POINT NAVIGATION, available via Alt+Shift+Up/Down in multiline plots. Left/Right navigate to the previous/next point intersection (data points present in both lines); slope intersections are excluded, reusing the goto feature's classifyIntersectionKind logic for consistency.

Related Issues

Changes Made

• Intersecting point navigation mode appears in the Alt+Shift+Up/Down rotor cycle only for multiline line plots.
• Right arrow → next point intersection (pointIndex > current col). Left arrow → previous.
• Only data points that exist in both line SVGs are navigated to — slope intersections are skipped.

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

Overview

This PR adds a new INTERSECTING POINT NAVIGATION rotor mode for multiline line plots, available via Alt+Shift+Up/Down to cycle into, then Left/Right to step between data-point intersections. The implementation correctly integrates with the existing rotor/extrema infrastructure and reuses findAllIntersectionsForCurrentLine and finalizeExtremaNavigation.

---

What Works Well

• Architecture is correct. Follows the same pattern as GRID_MODE: constant in constant.ts, a capability gate (supportsIntersectionMode()), mode registration in getAvailableModes(), and dispatch guards in movement methods.
• Performance is fine. getPointIntersectionIndices() delegates to findAllIntersectionsForCurrentLine(), which already has per-row caching (intersectionCache). No extra O(n^2) work per keystroke.
• State notifications are correct. finalizeExtremaNavigation() correctly calls notifyStateUpdate() and resets isInitialEntry, so observers receive the right update.
• Clean deduplication in getPointIntersectionIndices() using the seen Set plus sort.

---

Issues

1. Dead try-catch / redundant guard in moveIntersection()

The guard at the top of moveIntersection() checks supportsIntersectionMode() before calling the move methods. But INTERSECTION_MODE is only added to the rotor cycle when that same condition passes, so the catch block can never fire in normal operation — the default throw in AbstractPlot is unreachable. This creates two contradictory defenses that together make the code harder to reason about.

Suggested approaches:

• Option A: Keep the throws in the base class and the try/catch, but drop the explicit supportsIntersectionMode() guard inside moveIntersection() (consistent with how moveUpRotor/moveDownRotor work in rotor.ts).
• Option B: Make the base methods return false instead of throwing, remove the try/catch, and let the boundary condition in each override handle edge cases via notifyRotorBounds().

2. getIntersectionMessage() duplicates getMessage()

The new getIntersectionMessage(direction) re-implements the same isOff() / isTerse() branching that the existing getMessage(navType, direction) already handles. Calling this.getMessage('intersection', direction) would produce near-identical user-facing messages without the duplicated logic. If the slight wording difference ('found above the current point' vs 'found above the current value') is intentional for accessibility reasons, please add a comment explaining why; otherwise prefer reuse.

3. Missing unit tests for new navigation methods

The test/model/line.test.ts file was not updated. The following cases have zero coverage:

• supportsIntersectionMode() returns false for a single-line trace
• supportsIntersectionMode() returns true for a multiline trace
• moveToNextIntersection() advances col to the next point-intersection index
• moveToPrevIntersection() retreats col to the previous index
• Boundary cases: no next / no previous intersection triggers notifyRotorBounds

The PR checklist marks the unit test item as unchecked — please add tests before merge.

4. No E2E coverage for the new rotor mode

e2e_tests/specs/multiLineplot.spec.ts already tests the Go-To intersection dialog but has no test for the Alt+Shift keyboard rotor cycle introduced by this PR. A minimal test cycling into INTERSECTING POINT NAVIGATION and pressing Left/Right would catch regressions.

---

Minor Suggestions

moveToPrevIntersection reverse loop — the manual backwards for loop could be replaced with the cleaner Array.prototype.findLast, available in modern target environments:

const target = indices.findLast(index => index < this.col);

Method placement — the sibling capability methods (moveUpRotor, moveDownRotor, supportsCompareMode) live on AbstractTrace, not AbstractPlot. Moving the three new intersection methods down to AbstractTrace would keep the capability APIs co-located and avoid confusion about which hierarchy level owns intersection semantics.

---

Summary

The core logic is sound, the caching story is correct, and the pattern matches the existing rotor infrastructure. Main blockers before merge: missing unit tests and the dead try-catch / guard inconsistency. The message duplication and findLast suggestions are clean-ups worth addressing but not blocking.

[claude]

Code Review: feat: add intersecting point rotor mode for multiline plots

Overall: This is a well-scoped feature that integrates cleanly with the existing rotor architecture. The logic is correct, the tests cover the main cases, and the MVVC boundaries are respected. A few issues worth addressing before merge.

---

What the PR does

Adds INTERSECTING POINT NAVIGATION as a new rotor mode (cycle with Alt+Shift+Up/Down) for multiline line plots. Left/Right navigate between data points that exist in both lines, skipping slope-only crossings. Reuses findAllIntersectionsForCurrentLine's intersectionKind === 'point' filter for consistency with the goto feature.

---

Issues

1. Throw-based defaults are an antipattern here — prefer return false

src/model/abstract.ts

// current (throws)
public moveToNextIntersection(): boolean {
  throw new Error('Intersection navigation is not defined for this trace');
}

supportsIntersectionMode() gates whether the mode is ever added to the rotor cycle. So if a trace doesn't support intersection mode, isIntersectionMode() can never return true for it and these throw paths are dead code in normal operation — they only fire on a programming mistake (adding the mode without overriding the methods).

The rest of the codebase uses safe no-op defaults (return false, return true). Throws force the caller to use try-catch defensively (which moveIntersection() already does), masking real errors:

// preferred
public moveToNextIntersection(): boolean {
  return false;
}
public moveToPrevIntersection(): boolean {
  return false;
}

Then moveIntersection() in rotor.ts can drop its try-catch entirely and handle the false return directly, matching the pattern used for moveUpRotor, moveLeftRotor, etc.

2. getPointIntersectionIndices() is computed twice per navigation

src/model/line.ts

Both moveToNextIntersection() and moveToPrevIntersection() call getPointIntersectionIndices(), which calls findAllIntersectionsForCurrentLine(). That method scans all line pairs each time. For interactive navigation this is acceptable, but extracting it as a memoized getter (invalidated on row change) would follow the pattern used for highlightValues caching elsewhere in the class.

Not blocking, but worth considering if findAllIntersectionsForCurrentLine is called frequently during autoplay.

3. finalizeExtremaNavigation() is a misleading name for this use

src/model/line.ts lines 1206 and 1218

The method is named for extrema (min/max) navigation but is reused here for intersection navigation. It works correctly (it resets isInitialEntry, updates visual position, calls notifyStateUpdate()), but a future reader will be confused. A one-line comment at each call site is sufficient:

this.col = target;
// Reuse extrema finalization: resets entry state, updates visual, notifies observers
this.finalizeExtremaNavigation();

4. E2E test hardcodes rotor cycle count — fragile

e2e_tests/specs/multiLineplot.spec.ts

for (let i = 0; i < 3; i++) { // cycles to INTERSECTING POINT NAVIGATION

If a new rotor mode is added before INTERSECTING POINT NAVIGATION, this silently cycles to the wrong mode. The assertion on rotorArea text catches it eventually, but the intent is buried. Consider cycling until the target text appears (with a max iteration guard), or at least assert the mode text after each cycle step to make failures easier to diagnose.

5. moveIntersection — silent catch conflates programming errors with expected boundary

src/service/rotor.ts

} catch {
  return this.getMessage('intersection', direction);
}

A genuine bug in moveToNextIntersection (e.g., null dereference) and an intentional "not supported" throw both produce the same user-facing message. Since recommendation #1 removes the throws, this catch can be removed, making failures visible.

---

Minor notes

• Array.prototype.findLast (ES2023) — confirm the project's tsconfig target covers this, or add a polyfill if IE/older Safari is supported.
• The moveUp/moveDown responses for intersection mode (getMessage('intersection', 'above/below')) produce "No intersection value found above/below the current value." This is technically correct but could be more descriptive (e.g., "Use left/right arrows to navigate intersections"). Low priority.
• Unit test moveToPrevIntersection retreats… tests both the true and false return in one case. Consider splitting for clearer failure messages.

---

What's done well

• Clean integration with the rotor cycle — no changes needed to command or keybinding layers.
• getPointIntersectionIndices() correctly deduplicates and sorts; reusing intersectionKind === 'point' is good for consistency.
• supportsIntersectionMode() is false by default — safe for all other trace types.
• Unit tests cover: multiline detection, next/prev navigation with slope-crossing skip, boundary conditions (first/last intersection, no intersections). Good coverage.
• Architecture is correctly respected: model handles navigation and notifies observers, rotor service routes the command, no direct Redux dispatch from the model.

[claude]

Code Review — feat: add intersecting point rotor mode for multiline plots

Overview

This PR adds an INTERSECTING POINT NAVIGATION rotor mode that lets users navigate between data-point intersections (shared x/y values) across lines in a multiline plot, using Left/Right arrows while in the new rotor mode. The implementation correctly skips slope intersections and reuses the existing findAllIntersectionsForCurrentLine / classifyIntersectionKind logic for consistency. Overall the approach is sound; a few design concerns are worth addressing before merge.

---

Issues

1. Base-class methods throw instead of returning false — design inconsistency

src/model/abstract.ts

public moveToNextIntersection(): boolean {
  throw new Error('Intersection navigation is not defined for this trace');
}

supportsIntersectionMode() is the declared guard: callers are expected to check it before invoking the navigation methods. Given that guard exists, the default implementation should simply return false (a safe no-op) rather than throw. Throwing forces every call site to wrap in a try/catch even when the guard is respected, and it silently suppresses real programming errors in the rotor.ts catch block:

try {
  const moved = direction === 'right'
    ? activeTrace.moveToNextIntersection()
    : activeTrace.moveToPrevIntersection();
  ...
} catch {
  return this.getMessage('intersection', direction); // swallows unexpected errors too
}

Suggested fix: Change the base defaults to return false;. The try/catch in rotor.ts can then be removed, which also makes the control flow easier to reason about.

---

2. finalizeExtremaNavigation() name is misleading for intersection use

src/model/line.ts — moveToNextIntersection / moveToPrevIntersection

The method does three things that are correct for intersection navigation too (clear initial-entry flag, update visual position, notify observers), but its name implies it is specific to extrema/compare mode. Future maintainers may hesitate to call it or may accidentally break intersection navigation while "refactoring extrema logic."

Suggested fix: Rename to finalizeNavigation() (or introduce a thin finalizeIntersectionNavigation() that calls notifyStateUpdate() and updateVisualPointPosition() explicitly) so the intent is unambiguous.

---

3. Up/Down in intersection mode returns a directional bound message — potentially confusing

src/service/rotor.ts

if (this.isIntersectionMode()) {
  return this.getMessage('intersection', 'above'); // up arrow
}
if (this.isIntersectionMode()) {
  return this.getMessage('intersection', 'below'); // down arrow
}

The 'above' / 'below' strings suggest vertical bounds, while intersection mode is horizontal-only. A user pressing Up or Down might hear a message implying they reached a vertical boundary, which is incorrect. Please verify what text getMessage('intersection', 'above') produces and whether it makes sense in this context. If the intent is simply "this direction is unavailable in intersection mode," returning null (no-op) or a dedicated message like "Up/Down not available in intersection mode" would be clearer.

---

4. instanceof AbstractTrace guard is redundant given the mode check

src/service/rotor.ts — moveIntersection()

if (!(activeTrace instanceof AbstractTrace)) {
  return this.getMessage('intersection', direction);
}

isIntersectionMode() is only entered when getAvailableModes() included INTERSECTION_MODE, which only happens when activeTrace.supportsIntersectionMode() returns true. Since all traces are already AbstractTrace subclasses and the guard already ensures the mode is supported, this instanceof check adds noise without providing safety. Remove it and rely on the supportsIntersectionMode() contract.

---

5. getPointIntersectionIndices() is recomputed on every keystroke

src/model/line.ts

Every call to moveToNextIntersection / moveToPrevIntersection calls getPointIntersectionIndices(), which calls findAllIntersectionsForCurrentLine(). For typical multiline plots this is negligible, but if that method is O(n²) or walks the DOM, rapid key presses could be sluggish. Consider caching the result (invalidated on row/col change) or at least profiling against a large dataset.

---

6. findLast() is ES2023

indices.findLast(...) is ES2023. Please confirm the project's tsconfig.json / build target covers this, or add a polyfill. A safe alternative is:

const target = [...indices].reverse().find(index => index < this.col);

---

Minor / Positive Notes

• Constant naming (INTERSECTION_MODE = 'INTERSECTING POINT NAVIGATION') is consistent with the existing LOWER_VALUE_MODE / HIGHER_VALUE_MODE style. ✅
• Unit tests in test/model/line.test.ts are thorough: cover single-line guard, point/slope mixed data, boundary conditions in both directions, and the no-intersection case. ✅
• AbstractTrace import in rotor.ts was already present — no missing import. ✅
• MVVC compliance: the model calls notifyStateUpdate() after moving, services/viewmodels react correctly. ✅
• E2E test cycles the rotor correctly and asserts the label. However, it does not verify that ArrowRight/Left actually moved the cursor to a known intersection column. Strengthening the assertion (e.g. checking the text-output area for the expected point description) would give higher confidence.

---

Summary

Category	Status
Code correctness	Mostly correct; base-class throw vs. return-false is the main concern
Architecture (MVVC)	Compliant
Test coverage	Unit tests solid; E2E could be stronger
Security	No concerns
Performance	Low risk; intersection index recomputation worth documenting
Breaking changes	None

The feature is a good addition. Addressing issues 1–3 above would improve robustness and maintainability before merge.

[claude]

Code Review — feat: add intersecting point rotor mode for multiline plots

Overview

Adds INTERSECTING POINT NAVIGATION as a new rotor mode (Alt+Shift+Up/Down to cycle in, Left/Right to step between intersections) for multiline line plots, filtering to intersectionKind === 'point' to skip slope-only crossings. The architecture is correct, the rename from finalizeExtremaNavigation → finalizeNavigation is a welcome cleanup, and the base-class defaults correctly return false rather than throwing. A few things worth addressing before merge:

---

Issues

1. Capability methods belong on AbstractTrace, not AbstractPlot

src/model/abstract.ts

supportsCompareMode(), moveToNextHigherValue(), moveToNextLowerValue(), and all the grid-mode methods live on AbstractTrace (not AbstractPlot). The three new intersection methods are placed on AbstractPlot, which also covers Subplot and Figure — neither of which could meaningfully intersect. Moving them down to AbstractTrace keeps the capability API co-located and avoids the impression that subplots or figures support intersection navigation.

---

2. Boundary message uses "intersection value" — awkward for screen reader users

src/service/rotor.ts — moveIntersection()

return moved ? null : this.getMessage('intersection', direction);

getMessage('intersection', 'left') produces:

• Terse: "No intersection value found on the left"
• Verbose: "No intersection value found to the left of the current value."

Intersections are not values — the word is inherited from the compare-mode message and reads oddly out of context. A dedicated message like "No intersection found to the left" / "No intersection found to the right" would be clearer for screen reader users, which is the primary audience.

---

3. getIntersectionVerticalUnavailableMessage() duplicates the isOff/isTerse pattern

src/service/rotor.ts

private getIntersectionVerticalUnavailableMessage(): string {
  if (this.text.isOff()) { return ''; }
  if (this.text.isTerse()) { return 'Up/down unavailable in intersection mode'; }
  return 'Up and down navigation is not available in intersection point mode.';
}

The isOff/isTerse branching is the same pattern used throughout getMessage(). Introducing a helper like this.buildMessage(terse, verbose) (or folding this into getMessage with a special direction key) would reduce duplication. Minor, but the same pattern will be copied by the next person adding a mode.

---

4. E2E test only verifies no error is thrown, not actual navigation

e2e_tests/specs/multiLineplot.spec.ts

await page.keyboard.press('ArrowRight');
await page.keyboard.press('ArrowLeft');
// No assertion that the cursor moved or text output changed

The test confirms the rotor label appears and key presses don't crash, but does not verify that navigation happened. Consider asserting the MAIDR text output area (or the rotor area) after arrow presses to give real confidence. The comment "boundary feedback is expected" suggests the test data has no point intersections, which means ArrowRight/Left always returns false — so even a broken moveToNextIntersection would pass this test.

---

Minor Notes

• findLast() — ES2023, tsconfig.json targets ESNext so this is fine. ✅
• The test case moveToPrevIntersection retreats to the previous point intersection asserts both the true and false branches in one test, which makes failure messages harder to diagnose. Consider splitting into two cases.
• The getPointIntersectionIndices() sort at the end is redundant if findAllIntersectionsForCurrentLine() already returns points in column order — worth a quick check to avoid the extra allocation.

---

What's Done Well

• Base-class defaults return false — no defensive try/catch needed, and moveIntersection() correctly uses the boolean return. ✅
• finalizeNavigation rename is cleaner and more accurate. ✅
• getPointIntersectionIndices() deduplicates with a Set and delegates to the cached findAllIntersectionsForCurrentLine() — no per-keystroke O(n²) cost. ✅
• Unit tests cover: single-line guard, multiline detection, next/prev navigation with slope-crossing skipped, boundary conditions, and no-intersection case. ✅
• supportsIntersectionMode() defaults to false — other trace types are unaffected. ✅
• MVVC compliance: model navigates and calls notifyStateUpdate(), rotor service routes the command, no Redux from the model layer. ✅

---

Summary

Category	Status
Architecture (MVVC)	Mostly compliant — AbstractPlot placement is a concern
Accessibility / UX	"Intersection value" message wording should be improved
Unit test coverage	Solid
E2E coverage	Weak — smoke only
Performance	Fine (caching already in place)
Security	No concerns
Breaking changes	None

Issues 1 and 2 are the most impactful before merge.

[claude]

Code Review — feat: add intersecting point rotor mode for multiline plots

Overview

This PR adds a new rotor navigation mode (INTERSECTING POINT NAVIGATION) for multiline line traces. Users cycle to it via Alt+Shift+Up/Down; Left/Right navigate between data-point intersections while Up/Down announce that vertical movement is unavailable. The implementation reuses the existing findAllIntersectionsForCurrentLine / classifyIntersectionKind logic and the refactoring rename (finalizeExtremaNavigation → finalizeNavigation) is a welcome cleanup.

---

Issues

1. Method placement creates an inconsistent API contract

supportsIntersectionMode(), moveToNextIntersection(), and moveToPrevIntersection() are added to AbstractPlot, but moveIntersection() in rotor.ts only operates on AbstractTrace instances:

// rotor.ts
if (!(activeTrace instanceof AbstractTrace)) {
  return null;
}

If these methods are only meaningful for trace-level plots, they should live on AbstractTrace, not on the higher-level AbstractPlot. The runtime guard then becomes unnecessary and the API is self-documenting.

2. buildMessage() duplicates the existing getMessage() pattern

getMessage(navType, direction) already handles isOff() / isTerse() branching. The new buildMessage(terse, verbose) duplicates that logic with a different signature. Consider reusing getMessage or, if the intersection phrasing needs to differ, passing custom strings as overloads to keep messaging centralised.

3. INTERSECTION_MODE can appear before GRID_MODE is stable

getAvailableModes() pushes INTERSECTION_MODE after GRID_MODE, but the block is inside if (activeTrace instanceof AbstractTrace). For a future multiline scatter trace that supports both grid and intersection navigation the order would be DATA → LOWER → HIGHER → GRID → INTERSECTING. The comment on getAvailableModes now only mentions the partial ordering; update it to include the new mode so future contributors don't add modes in the wrong position.

4. rotorIndex can become stale when context changes

If the user navigates to a single-line plot while rotorIndex points to the slot that held INTERSECTION_MODE, the index is taken % modes.length against the shorter mode list. That correctly wraps, but the user would land on a different mode than expected (e.g. LOWER_VALUE_MODE) with no announcement. This is a pre-existing concern with all rotor modes, but adding a new mode makes it more likely. Consider resetting rotorIndex to 0 when the active trace changes.

---

Suggestions

5. Array.prototype.findLast — browser compatibility

findLast was introduced in ES2023 (Chrome 97, Firefox 104, Safari 15.4). If the project needs to support older browsers, replace it with:

const target = [...indices].reverse().find(index => index < this.col);

or check the tsconfig.json lib targets to confirm findLast is acceptable.

6. E2E test relies on implicit data fixture

// The example data has no point intersections, so arrow keys must route
// through intersection mode and land on the boundary message

This is fragile — if the test fixture ever gains coincident y-values the test still passes structurally but stops verifying what the comment claims. Pin the assertion more tightly, e.g. by asserting the exact message text, or add a brief inline note about which fixture file is used.

7. Missing unit test: position unchanged at boundary for moveToNextIntersection

The test 'moveToNextIntersection returns false at the last intersection' asserts moved === false but does not assert trace.col is unchanged. The analogous moveToPrevIntersection test does check col remains at 1. Add the equivalent assertion for the next-intersection boundary.

8. Performance comment is slightly misleading

The JSDoc for getPointIntersectionIndices says:

"repeated calls while the cursor stays on the same line are O(n) over the cached intersection list"

The cache key is currentGroup (the active row/line index), so "line" is correct. However, the O(n) note refers to the for…of loop over cached results, not to the full findAllIntersectionsForCurrentLine algorithm. The comment is not wrong but could be clearer that the cache avoids the expensive recomputation; the O(n) is only the filter step over an already-computed list.

---

Positive highlights

• Reusing classifyIntersectionKind via findAllIntersectionsForCurrentLine rather than duplicating the slope/point discrimination logic is exactly the right approach.
• The finalizeExtremaNavigation → finalizeNavigation rename is correct and all call sites are updated.
• Unit tests cover forward/backward navigation, boundary returns, no-intersection traces, and single-line traces (mode not advertised).
• The getIntersectionVerticalUnavailableMessage message is meaningfully different from a directional bound — good UX distinction.

---

Verdict

The feature works as described and the core logic is sound. The main items to address before merging are #1 (method placement), #2 (message helper duplication), and #3 (docs for mode ordering). Items #4–#8 are lower-priority but worth a look before the PR lands.

[claude]

Code Review — PR #604: Add intersecting point rotor mode for multiline plots

Overview

This PR adds a new rotor mode (INTERSECTING POINT NAVIGATION) to multiline line plots, accessible via Alt+Shift+Up/Down. Right/Left navigate to the next/previous point-type intersection while skipping slope intersections. The implementation reuses the existing findAllIntersectionsForCurrentLine() / classifyIntersectionKind logic from the goto feature, which is a sound design choice.

---

Positive Highlights

• Good reuse: Correctly reuses the existing intersection-classification logic (findAllIntersectionsForCurrentLine) rather than reimplementing it — keeping slope-vs-point discrimination consistent with the goto feature.
• Clean extension point: supportsIntersectionMode() / moveToNextIntersection() / moveToPrevIntersection() on AbstractTrace is a clean opt-in API. The safe no-op defaults mean non-line traces are unaffected.
• finalizeNavigation rename: Renaming from finalizeExtremaNavigation is a welcome generalisation; all callers are consistently updated.
• buildMessage refactor: Extracting buildMessage(terse, verbose) eliminates duplicated terse/verbose branching and makes getMessage() cleaner.
• Test coverage: Both unit tests (test/model/line.test.ts) and an E2E test (e2e_tests/specs/multiLineplot.spec.ts) are included.

---

Issues & Suggestions

1. Array.prototype.findLast — browser compatibility (minor risk)

// src/model/line.ts
const target = indices.findLast(index => index < this.col);

findLast is ES2023 and broadly available in modern browsers (Chrome 97+, Safari 15.4+), but since MAIDR targets accessibility users who may run assistive technology on older system browsers, it is worth calling out. The project has "target": "ESNext" and no explicit browserslist, which is fine, but consider whether a simple reversed find is safer for defensive compatibility:

// Alternative — no new APIs
const target = [...indices].reverse().find(index => index < this.col);

This is a low-priority suggestion given the ESNext target, but worth considering for an accessibility-first library.

---

2. E2E test only exercises the boundary (no actual navigation)

The E2E test comment acknowledges "the example data has no point intersections," so the test only verifies the boundary message, not real intersection traversal. This is better than no test, but a complementary test with data that contains actual intersections would give stronger confidence that moveToNextIntersection / moveToPrevIntersection work end-to-end in the browser. Consider a follow-up ticket or a second test.describe block if the test fixture can be extended.

---

3. Missing unit tests for RotorNavigationService

The rotor-service additions (isIntersectionMode, moveIntersection, getIntersectionBoundMessage, getIntersectionVerticalUnavailableMessage) are only exercised by the E2E test. A unit test for the service (mocking the context / text service) would catch regressions faster. Worth adding in a follow-up or this PR if time allows.

---

4. getPointIntersectionIndices() redundant sort

// src/model/line.ts
indices.sort((a, b) => a - b);

findAllIntersectionsForCurrentLine() may already return results in pointIndex order (since it iterates columns sequentially). If so, the .sort() is a no-op. If it is not guaranteed to be sorted, the sort is necessary — worth a brief comment confirming which case holds, or checking whether the upstream method guarantees ordering.

---

5. supportsIntersectionMode() — three-line check worth simplifying

public override supportsIntersectionMode(): boolean {
  return this.points.length > 1;
}

This is correct. Consider documenting why > 1 (i.e., "at least two lines are required for an intersection to exist") for future readers, since points is a 2-D array where the outer dimension is lines.

---

6. Vertical key presses in intersection mode — audio behaviour

When Up/Down is pressed in intersection mode, getIntersectionVerticalUnavailableMessage() returns a message and the method exits early — the model is not moved and no observer is notified. This means audio will not play, which is the right behaviour. But it may be worth a comment in the rotor service confirming this is intentional (rather than an accidental omission of notifyStateUpdate).

---

Summary

Category	Status
Architecture (MVVC)	✅ Correct
Reuse of existing logic	✅ Good
Test coverage	⚠️ Unit tests for rotor service missing; E2E only covers boundary
Browser compatibility	⚠️ findLast (ES2023) — low risk given ESNext target
Code style / KISS	✅ Clean and readable
Observer notifications	✅ finalizeNavigation() correctly called
Accessibility text modes	✅ Terse / verbose / off all handled

Overall this is a well-implemented, focused feature. The main asks before merge are: (1) verify or document the findAllIntersectionsForCurrentLine sort guarantee, and (2) consider adding a rotor-service unit test or an E2E data fixture with actual intersections to strengthen confidence.

[claude]

Code Review: feat: add intersecting point rotor mode for multiline plots

Overview

This PR adds a new INTERSECTING POINT NAVIGATION rotor mode that allows users to jump between data-point intersections across lines in multiline plots via Left/Right arrows. The implementation is well-structured and largely follows existing project conventions.

---

What Works Well

• Consistent pattern extension. The supportsIntersectionMode() / moveToNextIntersection() / moveToPrevIntersection() triple mirrors the existing supportsGridMode() / supportsCompareMode() pattern. Adding these to AbstractTrace with false defaults is the right approach.
• finalizeExtremaNavigation rename. Renaming to finalizeNavigation is correct — the old name was misleading for non-extrema navigation. All four call-site updates are consistent.
• buildMessage helper. Extracting the off/terse/verbose branching into one place is a clean DRY improvement. The refactored getMessage preserves the existing behavior.
• Deduplication + explicit sort in getPointIntersectionIndices. The comment explaining why an explicit sort is needed (non-monotonic nearest-point mapping) is valuable context.
• Unit test coverage. The four unit tests in line.test.ts cover the key branches: advance, boundary, retreat, and no-intersection cases.

---

Issues and Suggestions

1. Missing HTML fixture (potential CI failure)

The E2E test should navigate to a real point intersection with ArrowRight references examples/multiline_plot_intersection.html, but that file does not appear in this diff. If it is not present in the repo, the test will consistently fail in CI. Please confirm it exists or add it.

2. Array.prototype.findLast compatibility

const target = indices.findLast(index => index < this.col);

findLast (ES2023) is available in Node >= 18 and modern browsers, but if the project must support older targets this could fail silently after transpilation. Please verify the tsconfig lib/target settings, or use a safe alternative:

const target = [...indices].reverse().find(index => index < this.col);

3. instanceof AbstractTrace check in the service layer

In moveIntersection(), the guard if (!(activeTrace instanceof AbstractTrace)) couples the service to a concrete model class rather than an interface. If it fires unexpectedly, the user gets no feedback at all (null is returned silently). The guard is safe in practice since intersection mode is only enabled for AbstractTrace subclasses, but consider logging a warning on the unexpected branch, or ensuring the capability check is fully handled through the supportsIntersectionMode contract on the Plot interface.

4. Vertical navigation gives no audio feedback

The Up/Down handlers in intersection mode return the unavailable message but intentionally skip notifyStateUpdate(), so no audio tone plays. Other rotor modes do emit audio on boundary hits. If silence-on-vertical is the intended UX, add a note in the PR description so future contributors do not treat it as a bug.

5. Brittle E2E assertion for successful navigation

await expect(textContainer).toContainText('3', { timeout: 2000 });
await expect(textContainer).toContainText('4', { timeout: 2000 });

Checking bare digits will match any text containing those characters. A more robust assertion would use a regex like /x.*3.*y.*4/i or match the full announced description.

6. E2E rotor cycle is tied to absolute mode count

Using a hardcoded loop of 3 presses to reach INTERSECTING POINT NAVIGATION means inserting any future mode before it will silently break the test (it will land on the wrong mode). Consider cycling until the rotor area displays the expected string, rather than counting presses.

7. Nit: getPointIntersectionIndices can skip the parallel array

seen (a Set<number>) already holds all unique indices, making the parallel indices array redundant. Simplify to:

return [...seen].sort((a, b) => a - b);

---

Test Coverage Summary

Area	Unit	E2E
supportsIntersectionMode single vs. multiline	yes	-
moveToNextIntersection advance + boundary	yes	yes
moveToPrevIntersection retreat + boundary	yes	-
No point intersections exist	yes	yes (boundary message)
Vertical arrows in intersection mode	-	yes
Rotor mode cycling	-	yes
Real intersection fixture navigation	-	yes (pending fixture file)

---

Summary

The feature is well-conceived and the implementation is clean. The main blockers before merge:

1. Confirm/add examples/multiline_plot_intersection.html — otherwise CI will fail.
2. Verify findLast is safe under the project's browser/Node targets.

The instanceof AbstractTrace check in the service layer and the brittle E2E assertions are lower-priority but worth addressing for long-term maintainability.

[claude]

Code Review — feat: add intersecting point rotor mode for multiline plots

Overall, this is a well-structured addition. The implementation is clean, correctly layered in the MVVC architecture, and the reuse of the existing findAllIntersectionsForCurrentLine cache is a good design choice. Below are observations from most to least important.

---

Bugs / Correctness

1. moveIntersection() returns null in the defensive branch — semantically incorrect

// src/service/rotor.ts ~line 387-393
if (!(activeTrace instanceof AbstractTrace)) {
  console.warn('[RotorNavigation] Active plot is not an AbstractTrace; ...');
  return null;  // ← null means "navigation succeeded" to callers
}

In this service, null return from moveLeft()/moveRight() means "movement succeeded, no error to display." Returning null in a path where nothing moved is misleading — the rotor area stays silent while the model didn't change. A bound message or a distinct no-op return would be more honest. Since this branch is theoretically unreachable, a thrown error (in dev) or a terse warning message (in production) would also be reasonable alternatives.

---

Architecture / Design

2. supportsIntersectionMode() belongs on the interface, not the base class

The existing supportsGridMode() is defined on a type-guard interface (isGridNavigable), which keeps the capability-check at the type level and avoids runtime instanceof casts elsewhere. supportsIntersectionMode() is added directly to AbstractTrace instead, requiring an instanceof AbstractTrace check in moveIntersection(). For consistency with how supportsGridMode is handled, consider a type-guard function isIntersectionNavigable(trace) so the caller can narrow the type without instanceof.

That said, the current approach is functionally correct and simpler, so this is a style point rather than a blocking concern.

3. getAvailableModes() is computed on every getMode() call

getMode() calls getAvailableModes() each time it runs. getAvailableModes() in turn calls context.active, instanceof AbstractTrace, supportsCompareMode(), isGridNavigable(), and now supportsIntersectionMode(). This is pre-existing, but the new mode makes it slightly more expensive. A cached modes array invalidated when rotorIndex changes or context updates would be cleaner. Low priority unless traces have expensive supports* implementations.

---

Minor Issues

4. getPointIntersectionIndices() reconstructs the filtered set on every navigation call

The underlying findAllIntersectionsForCurrentLine() is cached per row, so repeated calls while the cursor stays on the same line are O(n) over the intersection list. However, getPointIntersectionIndices() creates a new Set and sorted array on every moveToNextIntersection / moveToPrevIntersection call. For typical intersection counts this is negligible, but the result could equally be cached per (row, col) or just per row (since col doesn't affect which intersections exist).

5. rotorIndex is not reset when context switches from multiline → single-line

If a user cycles to INTERSECTING POINT NAVIGATION on a multiline trace and then focus moves to a single-line trace, the rotorIndex % modes.length clamping in getMode() silently wraps the index to a different mode with no announcement. This is identical to the pre-existing behavior for GRID_MODE and is documented nowhere as a known limitation. Worth a comment in getMode() or in the keyboard documentation.

---

Test Coverage

6. Missing unit test: moveToPrevIntersection from position before all intersections

The unit suite covers:

• advancing forward through all intersections ✓
• hitting the right boundary ✓
• retreating from the last intersection ✓
• no intersections at all ✓

Missing: col = 0 when intersections exist at cols 1 and 4 → moveToPrevIntersection() should return false and leave col unchanged. Easy to add alongside the existing suite.

7. E2E: Up/Down in intersection mode only tested on the non-intersection fixture

The second E2E test (using multiline_plot_intersection.html) verifies ArrowRight moves to a real intersection, but doesn't test ArrowUp/ArrowDown on that fixture. Confirming that vertical navigation is also blocked (returns the "unavailable" message) when real intersections are present would round out coverage.

---

Positive Observations

• finalizeNavigation rename is correct and well-applied — the method is no longer used exclusively for extrema, so the rename removes a misleading implication. All callers updated consistently.
• buildMessage extraction is a clean DRY win; the getMessage refactor is correct and maintains prior behavior.
• ES2015 findLast avoidance: The [...indices].reverse().find() idiom is justified by the comment and avoids a potential runtime gap.
• cycleRotorTo E2E helper that polls the rotor area rather than using a fixed keypress count is the right approach — it won't silently break if the rotor order changes.
• Constant added to Constant class follows the existing pattern. The name INTERSECTING POINT NAVIGATION matches the PR description and test assertions.

---

Summary

Area	Finding
Correctness	moveIntersection defensive branch returns wrong sentinel
Architecture	supportsIntersectionMode could use a type-guard pattern
Performance	Minor allocation in getPointIntersectionIndices per call
Tests	One missing unit case; one missing E2E direction
Style	All good; comments are appropriate and non-redundant

Nothing here is blocking for a user-facing bug, but item 1 is worth a quick fix before merge.

[claude]

Code Review

Overview

This PR adds INTERSECTING POINT NAVIGATION as a new rotor mode for multiline line plots. Users can cycle into the mode via Alt+Shift+Up/Down and then use Left/Right arrows to jump between data points that are shared (same coordinates) across two or more lines. Slope-only crossings are explicitly excluded. The implementation reuses findAllIntersectionsForCurrentLine + classifyIntersectionKind already present in LineTrace, which is a good consistency decision.

---

Strengths

• Clean layering. The capability check (supportsIntersectionMode) and navigation methods (moveToNextIntersection/moveToPrevIntersection) live on AbstractTrace, exactly matching the existing pattern for supportsGridMode/supportsCompareMode. Adding a new modality required zero changes to the core observer chain.
• finalizeExtremaNavigation → finalizeNavigation rename. The scope of this method now correctly covers intersection navigation, and all 6 call sites were updated consistently.
• buildMessage extraction. Consolidating the off/terse/verbose branching into one private helper eliminates duplication and improves maintainability.
• Known limitation documented in-code. The comment about rotorIndex not resetting on context change is honest and self-contained — appreciated.
• Test coverage. Both unit tests (6 cases in line.test.ts) and E2E tests (2 scenarios in multiLineplot.spec.ts) are included, covering forward/backward navigation, slope-skipping, and boundary conditions.

---

Issues & Suggestions

1. getPointIntersectionIndices — missing deduplication across lines (potential bug)

// src/model/line.ts – getPointIntersectionIndices
const seen = new Set<number>();
for (const intersection of intersections) {
  if (intersection.intersectionKind === 'point') {
    seen.add(intersection.pointIndex);
  }
}

findAllIntersectionsForCurrentLine returns one entry per crossing pair — a three-way intersection at the same column produces two entries (line 0↔1 and line 0↔2), both with the same pointIndex. The Set deduplicates correctly here, but this relies on intersectionKind === 'point' being assigned consistently for multi-way crossings. Worth adding a unit test with a 3-line fixture where all three lines share a data point to confirm.

2. E2E test path may break depending on server base URL

// e2e_tests/specs/multiLineplot.spec.ts
await page.goto('examples/multiline_plot_intersection.html', { waitUntil: 'load' });

All other tests in this file use setupMultiLineplotPage(page), which presumably resolves the fixture relative to the dev server's base URL. This raw relative path could fail if the Playwright baseURL config doesn't point at the repo root. Suggest using the same helper pattern or checking playwright.config.ts to confirm this path resolves correctly in CI.

3. moveToPrevIntersection — unnecessary array copy

const target = [...indices].reverse().find(index => index < this.col);

[...indices].reverse() copies the entire sorted array just to walk it backwards. A simple reverse loop avoids the allocation:

let target: number | undefined;
for (let i = indices.length - 1; i >= 0; i--) {
  if (indices[i] < this.col) { target = indices[i]; break; }
}

The current code is correct and readable, but the copy is wasteful for large datasets. Low priority.

4. moveIntersection returns null for success — non-obvious contract

// rotor.ts
return moved ? null : this.getIntersectionBoundMessage(direction);

The null = success convention is documented in the JSDoc, but it differs from what callers might expect. The existing moveGrid function follows the same contract (established pattern), so this is consistent. However, the JSDoc on moveIntersection says @returns Error message if at bounds, null otherwise — adding a note that null signals a successful move (matching the calling convention in moveUp/moveDown/moveLeft/moveRight) would help future maintainers.

5. No unit tests for RotorNavigationService intersection integration

The rotor-level logic (isIntersectionMode, moveIntersection, getIntersectionBoundMessage, getIntersectionVerticalUnavailableMessage) is exercised only via E2E tests. E2E tests are valuable but slower and more brittle (DOM-dependent). Suggest adding a unit test for RotorNavigationService that mocks a LineTrace and verifies:

• moveLeft/moveRight dispatch to moveToPrevIntersection/moveToNextIntersection
• moveUp/moveDown return the vertical-unavailable message
• Boundary reached → correct bound message returned

6. getMessage is public and its behavior changed silently

// Before (from diff):
public getMessage(navType: string, direction: string): string {
  if (this.text.isOff()) { return ''; }  // ← explicit early exit
  ...
}

// After:
public getMessage(navType: string, direction: string): string {
  return this.buildMessage(...);  // ← off check inside buildMessage
}

The observable behavior is identical, but getMessage is public, so downstream callers (or tests) that checked getMessage directly without mocking text.isOff() may now see different behaviour if buildMessage introduces any edge cases. This is low risk but worth a quick scan to confirm there are no direct call sites outside rotor.ts.

---

Minor Nits

• cycleRotorTo is defined as a bare function inside the test.describe block rather than a beforeEach/beforeAll fixture — fine for readability, just non-standard for Playwright.
• JSDoc on supportsIntersectionMode in abstract.ts says "currently only multiline line traces" — consider phrasing as "opt-in per trace type" to avoid needing to update the comment when a second trace type gains this capability.

---

Summary

The feature is well-scoped, consistent with existing patterns, and thoughtfully documented. The main actionable items before merge are:

1. Verify the E2E fixture path resolves correctly in CI (risk: silent test failure in non-local environments).
2. Add a 3-line point-intersection unit test to confirm the Set-deduplication path is correct for multi-way crossings.
3. Consider adding rotor-service unit tests for the intersection dispatch/message logic.

Everything else is minor polish. Great work overall — the buildMessage refactor and the acknowledged known-limitation comment are both solid additions beyond the feature itself.

[claude]

Code Review – feat: add intersecting point rotor mode for multiline plots

Overview

This PR adds INTERSECTING POINT NAVIGATION as a new rotor mode accessible via Alt+Shift+Up/Down on multiline line plots. Left/Right arrows then jump to the previous/next data-point intersection (exact coordinate match between lines), skipping slope-only crossings. The implementation is clean, well-documented, and follows the MVVC architecture throughout.

---

✅ Strengths

• Architecture is correct. Model handles navigation (line.ts), service handles routing (rotor.ts) — clean MVVC separation with no layer violations.
• Opt-in extension pattern is well-designed. supportsIntersectionMode() / moveToNextIntersection() / moveToPrevIntersection() on AbstractTrace are safe no-ops by default; LineTrace overrides only what it needs. Easy to extend to other trace types later.
• finalizeNavigation() rename is a clear improvement — the name finalizeExtremaNavigation was misleading since the method now applies to any navigation finalization. All callers are updated correctly.
• buildMessage() helper nicely eliminates the terse/verbose branching duplicated across message helpers.
• Deduplication via Set correctly handles multi-way intersections (e.g. 3 lines sharing one data point). The unit test for this case is particularly valuable.
• Test coverage is solid: unit tests cover all directional variants, boundary conditions, and deduplication; a new test/service/rotor.test.ts validates the service dispatch layer independently; E2E tests confirm end-to-end behavior including the vertical-unavailable path.
• Honest documentation of the known limitation around rotorIndex not resetting on context change — appropriate since that's a pre-existing broader issue.

---

🐛 Issues / Concerns

1. getAvailableModes() called twice per navigation (minor perf)

Each moveUp() / moveDown() / moveLeft() / moveRight() now calls getAvailableModes() twice — once through isGridMode() and once through isIntersectionMode(). Both methods delegate to getMode() → getAvailableModes(). This was already a one-call-per-nav pattern for the grid check; the PR adds a second call. For typical chart interactions this is negligible, but worth noting. Consider extracting getMode() into a local variable at the top of each move*() method:

public moveLeft(): string | null {
  const mode = this.getMode(); // single call
  if (mode === Constant.GRID_MODE) return this.moveGrid('left');
  if (mode === Constant.INTERSECTION_MODE) return this.moveIntersection('left');
  // ...
}

2. getPointIntersectionIndices() allocates on every navigation call

The method constructs a Set and then a sorted Array on every call to moveToNextIntersection() / moveToPrevIntersection(). While findAllIntersectionsForCurrentLine() caches its results per row, getPointIntersectionIndices() itself is not memoized. For charts with many intersection points this is fine, but if performance ever becomes a concern, caching the result alongside the row index would avoid the allocation.

3. Intersection navigation is row-scoped — worth a code comment

moveToNextIntersection() only navigates intersections that the current line (this.row) participates in. If a user switches lines and then presses Right in intersection mode, they'll see intersections for the new line, not the previous one. This is correct and intuitive, but a short comment near the method explaining "intersections are relative to the active row" would help future readers avoid confusion.

4. PR checklist inconsistency

The "I have added appropriate unit tests" checkbox is unchecked, but the PR includes comprehensive unit and E2E tests. This is a minor documentation nit but worth fixing to accurately reflect the work done.

---

💡 Suggestions

E2E cycleRotorTo helper – The helper is defined as a standalone function inside test.describe, which works but makes it slightly harder to discover. Consider moving it to a shared test utility file if other describe blocks ever need to cycle the rotor.

moveIntersection guard wording – The console.warn message reads 'Active plot is not an AbstractTrace; intersection move ignored'. Consider adding the mode name for easier grep: '[RotorNavigation][INTERSECTION_MODE] Active plot is not an AbstractTrace...' — consistent with how other logs in the file are prefixed.

Vertical message phrasing – The verbose vertical-unavailable message is 'Up and down navigation is not available in intersection point mode.' The mode name in the constant is 'INTERSECTING POINT NAVIGATION'. For screen reader consistency it might read better as 'Up and down navigation is not available in intersecting point navigation mode.' — matching the announced mode name exactly. Small point, user-preference-level.

---

✅ Summary

The implementation is correct, well-architected, and meaningfully tested. The two substantive notes (double getAvailableModes() call and undocumented row-scoping) are minor and non-blocking. This is a solid contribution — approve after addressing the checklist nit and optionally the getMode() local-variable suggestion.

[claude]

Code Review

PR: feat: add intersecting point rotor mode for multiline plots
Files changed: 11 (+651 / -22)

---

Overview

This PR adds an INTERSECTING POINT NAVIGATION rotor mode to multiline line plots, accessible via Alt+Shift+Up/Down cycling. Left/Right arrow keys navigate to previous/next data-point intersections between lines, while Up/Down are explicitly blocked with a user-facing message. Slope intersections are excluded by reusing the existing classifyIntersectionKind logic.

The implementation follows MVVC architecture well: the Model layer gains opt-in capability methods, the Service layer dispatches to them, and existing tests are extended alongside new unit and E2E tests. The design choices are generally sound.

---

Bugs / Blockers

1. Missing example HTML fixture — E2E test will fail

The E2E test at e2e_tests/specs/multiLineplot.spec.ts (line ~65) does:

await page.goto('examples/multiline_plot_intersection.html', { waitUntil: 'load' });

But examples/multiline_plot_intersection.html is not included in this PR's diff. The file must be committed, or the test will fail in CI with a navigation/404 error.

2. isIntersectionMode() is dead code

src/service/rotor.ts defines:

private isIntersectionMode(): boolean {
  return this.getMode() === Constant.INTERSECTION_MODE;
}

But none of the four move methods (moveUp, moveDown, moveLeft, moveRight) call it — they all cache const mode = this.getMode() and compare directly. The private method is never referenced and should be removed to avoid confusion.

---

Code Quality

3. finalizeExtremaNavigation rename is sensible but large in scope

Renaming to finalizeNavigation() across 6 files (bar, candlestick, heatmap, segmented, line, abstract) is the right call — the old name was misleading for non-extrema paths — but this is a pure refactor bundled into a feature PR. Consider noting it explicitly in the PR description, or splitting it, so bisect and blame remain clean.

4. getPointIntersectionIndices() allocates on every call

private getPointIntersectionIndices(): number[] {
  const intersections = this.findAllIntersectionsForCurrentLine();
  const seen = new Set<number>();
  for (const intersection of intersections) {
    if (intersection.intersectionKind === 'point') {
      seen.add(intersection.pointIndex);
    }
  }
  return [...seen].sort((a, b) => a - b);
}

Both moveToNextIntersection and moveToPrevIntersection call this — so each user keypress builds the Set, spreads it into an array, and sorts it twice. The underlying findAllIntersectionsForCurrentLine already caches per row, so the per-call overhead is just the filter+dedupe+sort pass. For realistic multiline data sizes this is negligible, but a simple cache keyed on this.row (invalidated on row change) would make the intent explicit.

5. console.warn in moveIntersection — use the project logger

src/service/rotor.ts includes:

console.warn('[RotorNavigation] Active plot is not an AbstractTrace; intersection move ignored');

Per the codebase conventions (see DEBUGGING.md), direct console.* calls should not be left in production code. Either use the project's logger.warn() utility or remove the log entirely (the defensive return value already communicates the failure to the caller).

6. Known-limitation comment in getMode() is acknowledged but untracked

The comment documents that rotorIndex is not reset when the active trace changes, which can cause a silent mode-wrap. This is called out fairly, but it would be better to open a follow-up issue and reference it (e.g. // TODO: see #XXX) so the UX debt doesn't get lost.

---

Architecture

7. supportsIntersectionMode() / moveToNextIntersection() / moveToPrevIntersection() placement

Adding three public methods to AbstractTrace is a clean opt-in design that follows the same pattern as supportsGridMode(). The default return false base implementation is correct. No concerns here.

8. moveIntersection defensive instanceof AbstractTrace guard

The guard is well-motivated (race condition between mode list build and key press), and returning a user-facing message instead of null is the right convention. The comment clearly explains why this branch should never be reached under normal operation. Good defensive coding.

---

Test Coverage

The unit tests in test/model/line.test.ts and test/service/rotor.test.ts are thorough and well-structured:

• ✅ supportsIntersectionMode only for multiline
• ✅ moveToNextIntersection advances correctly, skips slope crossings
• ✅ moveToPrevIntersection retreats correctly
• ✅ Bounds conditions (returns false, col unchanged)
• ✅ Three-line deduplication test
• ✅ No-intersection data set
• ✅ Rotor service dispatch: moveRight/Left/Up/Down in intersection mode
• ✅ Terse vs. verbose vs. off message variants
• ✅ cycleRotorTo helper avoids brittle fixed press counts

9. Missing unit test: intersection navigation on a non-zero row

All unit tests set trace.col = N but leave trace.row at its default (0). A test navigating when row = 1 (cursor on the second line) would confirm that findAllIntersectionsForCurrentLine() correctly scopes to the active line, not always line 0.

10. E2E test uses page.goto() directly without the BasePage helper

The second E2E test bypasses setupMultiLineplotPage and calls page.goto('examples/multiline_plot_intersection.html', ...) directly. This is pragmatic for a new fixture, but make sure the path resolves correctly under the Playwright baseURL configured in e2e_tests/config/test-config.ts.

---

Summary

Category	Status
Architecture (MVVC)	✅ Follows existing patterns
Feature correctness	✅ Logic is sound
Missing fixture file	❌ Blocker — E2E will fail
Dead code (isIntersectionMode)	⚠️ Remove unused private method
console.warn in production path	⚠️ Use project logger or remove
Test coverage	✅ Good; add row > 0 test
Rename scope	ℹ️ Fine, but note in PR description

The core logic and design are solid. Please add the missing examples/multiline_plot_intersection.html fixture and remove the dead isIntersectionMode() method before merging. The other items are lower priority but worth addressing.

[claude]

Code Review: feat: add intersecting point rotor mode for multiline plots

Overview

This PR adds a new INTERSECTING POINT NAVIGATION rotor mode (Alt+Shift+Up/Down to cycle in) for multiline plots. Left/Right arrows navigate between data-point intersections (shared sampled coordinates), skipping slope-only crossings. The implementation reuses the existing findAllIntersectionsForCurrentLine logic and its row-keyed cache, and cleanly extends the opt-in rotor capability pattern already established by supportsGridMode.

---

Architecture and Design

• Opt-in via supportsIntersectionMode() follows the same pattern as supportsGridMode() and supportsCompareMode(). Defaulting to false on AbstractTrace is exactly right.
• buildMessage() refactoring cleanly eliminates the duplicated terse/verbose/off branching that existed in the old getMessage(). Nice DRY improvement.
• finalizeExtremaNavigation → finalizeNavigation rename is a good improvement, though ideally a separate commit for clean history.
• Caching: The new navigation methods rely on findAllIntersectionsForCurrentLine(), which is cached per row. Moving horizontally (changing col) does not invalidate the cache. This is correct.
• setMode() behavior: When cycling into INTERSECTION_MODE, setRotorEnabled(true) and notifyGridMode(false) are correctly called, consistent with the existing non-grid rotor modes.

---

Issues and Suggestions

1. getPointIntersectionIndices() rebuilds on every keypress

The underlying findAllIntersectionsForCurrentLine() is cached per row, but the Set construction and sort run on every arrow keypress. For typical multiline plots this is negligible, but consider caching the filtered indices under the same row key if this becomes a hot path with large datasets.

2. console.warn in production code

In moveIntersection() there is a console.warn('[RotorNavigation] Active plot is not an AbstractTrace; intersection move ignored'). Per the project debugging guide, logs should be removed before commit. This defensive guard for an unlikely race condition is worth keeping as logic, but the warn call itself should be removed or wrapped in a debug flag consistent with how other services handle unexpected states.

3. formatModeDisplay() returns plain mode name for INTERSECTION_MODE

Grid mode enriches its display string with dimension info (e.g., "GRID NAVIGATION: 5×4 GRID"). Intersection mode just returns "INTERSECTING POINT NAVIGATION". Functionally fine, but a count like "INTERSECTING POINT NAVIGATION: 3 intersections" could give users immediate context before they start navigating. Not blocking, but worth considering for accessibility.

4. moveIntersection vs callMoveToNextCompareMethod guard behavior

callMoveToNextCompareMethod logs console.error and returns null (the success signal) when the active trace is not an AbstractTrace. moveIntersection correctly returns a non-null message in the same guard case. The new behavior is correct and is an improvement over the existing compare method pattern.

---

Test Coverage

• test/model/line.test.ts: Covers multi-way intersections, deduplication via Set, row-scoping, edge cases (no intersections, at boundary). The createMixedIntersectionTrace fixture is well-designed to catch slope vs. point confusion.
• test/service/rotor.test.ts: Tests dispatch through the rotor service (mode cycling, actual trace movement), terse/verbose/off text modes, and boundary messages. Right scope for a service-level test.
• e2e_tests/specs/multiLineplot.spec.ts: The cycleRotorTo helper correctly avoids relying on a fixed press count. The second test depends on examples/multiline_plot_intersection.html, which is confirmed present in the repo.

---

Security

No external input is rendered as HTML, no dynamic code execution, no new dependencies introduced.

---

Summary

This is a solid, well-tested feature that fits cleanly into the existing rotor architecture. Actionable items:

1. Remove the console.warn (or guard it) to align with project conventions.
2. Consider caching getPointIntersectionIndices() under the same row key as the intersection cache if profiling shows it matters.
3. Optional: Enrich formatModeDisplay() for INTERSECTION_MODE with an intersection count for richer screen-reader context.

The buildMessage() refactoring and the documented known limitation about rotorIndex not resetting on context change are both handled well. LGTM with the console.warn nit addressed.

[claude]

Code Review — PR #604: Intersecting Point Rotor Mode for Multiline Plots

Overall: This is a well-structured feature addition that correctly follows the project's MVVC architecture. The intersection logic builds cleanly on existing infrastructure, test coverage is solid across all three layers (unit, service, E2E), and the UX messages are thoughtfully designed for screen reader users. A few items below warrant attention before merging.

---

What This PR Does

• Adds INTERSECTING POINT NAVIGATION as a new rotor unit (Alt+Shift+Up/Down cycle), available only for multiline line traces.
• Left/Right navigate to previous/next point intersections between lines; slope intersections are explicitly excluded by reusing findAllIntersectionsForCurrentLine.
• Up/Down in this mode return a distinct "unavailable" message rather than silently doing nothing.
• Renames finalizeExtremaNavigation() → finalizeNavigation() across all callers (good generalization).
• Extracts buildMessage(terse, verbose) utility to eliminate per-mode off/terse/verbose branching duplication.

---

Issues

1. Documented Known Limitation Is a Real Accessibility Risk

src/service/rotor.ts (the new comment in getMode()):

"rotorIndex is not reset when the active plot/trace changes. If the user cycles to a capability-gated mode (GRID_MODE or INTERSECTION_MODE) and focus then moves to a trace that does not advertise that capability, the modulo below silently wraps the index onto a different mode without announcing the switch."

For a screen reader–first tool this is a meaningful UX regression, not just a known limitation. A user who cycles into INTERSECTION_MODE on a multiline chart, then tabs to a single-line chart, will end up in an undisclosed mode. The fix (reset rotorIndex on context change) could be a follow-up PR, but the issue should be tracked in an issue rather than only in a comment that will drift away from the bug.

Suggestion: Open a tracking issue for this and link it from the comment so it doesn't get lost.

2. getAvailableModes() Called on Every Key Press

Before this PR, isGridMode() called getMode() which called getAvailableModes() — so one call per directional key. This PR caches getMode() per method (good), but getAvailableModes() still runs every time a direction key is pressed. For the typical case (a few lines with a few hundred points) this is negligible, but the intersection filter in getAvailableModes() calls supportsIntersectionMode() which touches this.points.length — O(1), so no real concern. No action needed, just noting.

3. Minor: cycleRotorTo / cycleTo Duplicated Across Test Files

The E2E file defines cycleRotorTo(page, targetMode) and the service unit test defines cycleTo(service, target). These serve different layers so they can't be directly unified, but the algorithm is identical. If a new mode changes the cycle order, both files need updating. Consider extracting the unit-test version into a shared test helper in test/helpers/ to reduce future drift.

---

Code Quality Observations

Positives:

• supportsIntersectionMode() as an opt-in override on AbstractTrace is the right extension point — it keeps multiline logic out of the base class and avoids fat switches in the service.
• getPointIntersectionIndices() correctly deduplicates via a Set before sorting, which handles 3-way intersections cleanly (verified by the "multi-way point intersection shared across 3 lines" unit test — nice edge case to cover).
• moveToPrevIntersection() avoids the Array.prototype.findLast (ES2023) dependency and the allocation cost of [...indices].reverse() — the reverse-loop comment is exactly the kind of non-obvious rationale that warrants a comment.
• The distinction between getIntersectionBoundMessage and getIntersectionVerticalUnavailableMessage is UX-correct: "no intersection to the right" vs "up/down unavailable in intersection mode" prevents screen reader users from inferring a vertical bound that doesn't exist.
• Refactoring getMessage() to use buildMessage() is a clean improvement and the behavior is equivalent (verified by inspection).

Minor style note:

The JSDoc comment density in this PR is noticeably higher than the project norm (CLAUDE.md: "Default to writing no comments. Only add one when the WHY is non-obvious"). The @param and @returns tags on private helpers like createLineLayer and createMockContext in test files add noise without aiding readers. Consider trimming those to just the non-obvious why comments (e.g. the reverse-loop note, the dedup rationale, the caching note) and dropping the mechanical parameter descriptions.

---

Test Coverage

Layer	Tests Added	Quality
Unit (test/model/line.test.ts)	7 new tests	Good coverage of next/prev, boundary, dedup, row scoping, no-intersections case
Service (test/service/rotor.test.ts)	6 new tests	Covers all four directions, terse/off text modes, bound messages
E2E (e2e_tests/specs/multiLineplot.spec.ts)	2 new tests	Covers the "no intersections" boundary path and a real intersection fixture

The cycleRotorTo helper in E2E is a good pattern — avoids hardcoded press counts that break silently when the rotor order changes.

One gap: there's no test verifying that INTERSECTION_MODE does not appear in the rotor cycle for a single-line trace. Adding that to rotor.test.ts would close the loop on the supportsIntersectionMode → getAvailableModes wiring.

---

Architecture Check ✅

• Navigation logic lives in LineTrace (model layer) — correct.
• RotorNavigationService handles mode routing and user messages (service layer) — correct.
• finalizeNavigation() calls notifyStateUpdate(), keeping the observer chain intact — correct.
• No direct Redux dispatch or UI manipulation from service layer — correct.

---

Summary

This is a solid, well-tested feature. The two things I'd address before merging:

1. Open a tracking issue for the rotorIndex-not-reset limitation so it doesn't stay as an in-file comment indefinitely.
2. Add one more rotor unit test asserting that INTERSECTION_MODE is absent from the available modes for a single-line trace.

Everything else is either pre-existing behavior or low-priority cleanup.

[claude]

Code Review

Overview

This PR adds an INTERSECTING POINT NAVIGATION rotor mode for multiline line plots, accessible via Alt+Shift+Up/Down. Users can then press Left/Right to jump between data points where two or more lines share the same sampled value ("point intersections"), explicitly excluding slope-only crossings. The implementation is well-structured and follows the MVVC architecture closely.

---

Architecture & Code Quality ✅

Strengths:

• MVVC compliance is solid. Navigation logic lives in the model (LineTrace), routing lives in the service (RotorNavigationService), and no layer bleeds into another.
• Opt-in pattern via supportsIntersectionMode() mirrors the existing supportsGridMode() / supportsCompareMode() pattern — a natural, consistent extension point.
• finalizeExtremaNavigation → finalizeNavigation rename is well-executed. The old name was misleading once intersection navigation reused the method. All 7 call sites are updated.
• buildMessage() refactor cleanly eliminates the duplicated isOff/isTerse/verbose branching that was in getMessage(). No behavior change — good hygiene.
• getMode() cached once per move method — the old isGridMode() helper called getMode() each time it was used; caching it at the top of moveUp/Down/Left/Right avoids redundant work. Noted correctly in the comment.

---

Potential Issues

1. setMode() does not notify when entering INTERSECTION_MODE (minor UX gap)

When cycling into INTERSECTION_MODE, setMode() calls notifyGridMode(currMode === GRID_MODE) → notifyGridMode(false). This disables grid mode, which is correct, but there is no corresponding "notify intersection mode entered" event. Compare to GRID_MODE which calls setGridMode(true) on the active trace via notifyGridMode. Intersection mode has no equivalent activation side effect today, which may be intentional, but it means any service listening for mode-entry won't know the user is now in intersection mode. If the audio or highlight service ever needs to react to entering intersection mode, a hook will be needed.

2. getPointIntersectionIndices() re-sorted on every moveToNext/Prev call

Each navigation call constructs a new Set, iterates over the cached intersection list, converts to a sorted array, and then discards it. For the typical intersection list size (≪100 items) this is negligible. But if the list were ever large (e.g. a 1 000-point multiline trace with many crossings), repeated sorting on every keypress could be noticeable. A memoized getter invalidated on row/col change would future-proof this. Not a blocker — just something to keep in mind.

3. Known limitation (acknowledged in code, but worth surfacing here)

The comment in getMode() notes:

rotorIndex is not reset when the active plot/trace changes. If the user cycles to INTERSECTION_MODE and focus moves to a trace that does not advertise that capability, the modulo silently wraps the index onto a different mode without announcing the switch.

This pre-existed the PR but is now more impactful since a third capability-gated mode exists. A follow-up issue to reset rotorIndex on context change would improve robustness.

---

Test Coverage ✅

Coverage is comprehensive and well-designed:

Unit tests (test/model/line.test.ts additions):

• supportsIntersectionMode() for single- vs. multi-line traces
• moveToNextIntersection(): normal advance, slope-crossing skip, boundary at last intersection
• moveToPrevIntersection(): backward advance, boundary before first intersection
• 3-way intersection deduplication via Set (this is the non-obvious case — great that it's tested)
• Row-scoping (active row changes which intersections are visible)
• No-intersection trace returns false for both directions

Unit tests (test/service/rotor.test.ts — new file):

• Right/left dispatch in intersection mode
• Bound message on right at last intersection
• Vertical unavailability messages for up/down
• Terse vs. verbose message formatting
• Text-off suppression
• INTERSECTION_MODE absent from single-line rotor cycle

E2E tests (e2e_tests/specs/multiLineplot.spec.ts additions):

• cycleRotorTo helper avoids fixed press counts — good defensive practice
• Tests bound messages for right/left and vertical unavailability
• Second test exercises a real intersection in examples/multiline_plot_intersection.html (file confirmed present in the repo)

---

Nits / Minor Suggestions

1. Verbose comments — several comments (especially in line.ts and rotor.ts) are more detailed than the project's "only comment the non-obvious WHY" convention in CLAUDE.md. The getPointIntersectionIndices docblock, for instance, explains what the code does as well as why. Trimming to just the non-obvious parts (the sort rationale and the performance note) would tighten things up.

2. Duplicated createLineLayer helper — it appears in both test/model/line.test.ts and test/service/rotor.test.ts. If there's a shared test utilities directory, this could be extracted. Minor given test isolation benefits.

3. cycleRotorTo helper duplicated between E2E and unit tests — same idea; a shared test helper or a comment pointing to the parallel would keep things DRY. Again minor.

4. moveToPrevIntersection uses a manual reverse loop to avoid Array.prototype.findLast — the comment explains this is for ES2023 compatibility. If the project's tsconfig already targets ES2023+ or findLast is polyfilled elsewhere, the loop can be simplified. If not, the approach is correct.

---

Summary

This is a well-implemented, architecturally compliant feature. The test coverage is thorough — particularly the 3-way deduplication and row-scoping unit tests, and the cycleRotorTo E2E helper. The buildMessage() refactor is a nice improvement. The two items worth addressing before merge are:

1. (Consider) Whether setMode() needs an intersection-mode activation side effect analogous to notifyGridMode.
2. (Track) A follow-up issue for resetting rotorIndex on context change, now that a third capability-gated mode exists.

Neither is a blocker. Nice work!

[nk1408]

hello @soundaryakp1999 , i found a minor bug while navigating, there is only one intersection point at X=2, Y=4. but still in intersecting point rotor mode it moves to another point at X=8, Y=8 (or sometimes X=8, Y=7 (highlighting is flaky)). Could you please fix this issue?

[nk1408]

looking good to me now!

[claude]

Code Review PR 604 Intersecting Point Rotor Mode

Overview

This PR adds INTERSECTING POINT NAVIGATION to the rotor cycle for multiline plots. Right/Left in this mode jump between data points shared across lines (point intersections only; slope crossings excluded). The mode is scoped to LineTrace with more than 1 line via opt-in supportsIntersectionMode() on AbstractTrace. Implementation follows MVVC and has solid unit and E2E tests.

---

Issues

Unrelated file committed: .claude/scheduled_tasks.lock

The diff includes .claude/scheduled_tasks.lock, a runtime lock file unrelated to this feature. Remove it before merging.

---

Suggestions

getMessage() computes strings even when text is off. The refactored version always evaluates preposition and position even when isOff() would suppress output. Not a performance concern, but the old early-return was more explicit about intent.

Caching comment in moveUp/Down/Left/Right overstates the optimization. const mode = this.getMode() is a local variable assignment, not memoization. getMode() still recomputes getAvailableModes() on each call. The real benefit is clarity (one call, two comparisons). The cache framing in the comment should be simplified.

Known limitation: rotor index not reset on context change. The getMode() comment correctly notes this. If a user cycles into INTERSECTION_MODE on a multiline trace and focus moves to a single-line trace, the modulo wrapping silently changes modes without announcing it. Track as a follow-up issue.

dodged_barplot_large.html and multiline_plot_large.html are not referenced by any new test and appear unrelated to the feature. If they serve a purpose, note it in the PR description.

---

Positives

Opt-in design on AbstractTrace: supportsIntersectionMode(), moveToNextIntersection(), moveToPrevIntersection() all default to no-op/false so existing traces are completely unaffected.

finalizeExtremaNavigation() to finalizeNavigation() rename is correct and consistent now that the method covers intersection navigation too.

buildMessage() helper nicely reduces duplication of off/terse/verbose branching across all intersection message helpers.

Defensive instanceof AbstractTrace check in moveIntersection() correctly handles the edge case where context changes between the mode list build and the key press.

Set-based dedup in getPointIntersectionIndices() correctly handles n-way intersections (e.g., 3 lines sharing one point counted as one stop).

E2E cycleRotorTo helper avoids fragile fixed press counts and stays correct if rotor order changes.

Test coverage is thorough: boundary conditions, slope vs. point filtering, multi-way dedup, row scoping, terse/off text modes, and INTERSECTION_MODE exclusion for single-line traces.

Architecture is correct: model methods live on LineTrace, dispatch in RotorNavigationService, no direct UI manipulation. Properly follows MVVC.

---

Summary

The feature implementation is solid. The only required change before merging is removing .claude/scheduled_tasks.lock from the commit. All other points are minor. The accessibility design (boundary announcements, vertical navigation blocked with a clear message) is appropriate for screen reader users.

[claude]

Code Review – PR #604: Intersecting Point Rotor Mode for Multiline Plots

Overall this is a clean, well-scoped feature. The extension model (opt-in via supportsIntersectionMode()), the deduplication logic for multi-way crossings, and the test coverage are all solid. A few issues and suggestions below.

---

Summary of Changes

• src/model/abstract.ts – Adds supportsIntersectionMode(), moveToNextIntersection(), and moveToPrevIntersection() as opt-in no-ops on AbstractTrace. Also renames finalizeExtremaNavigation() → finalizeNavigation().
• src/model/line.ts – Overrides all three methods; getPointIntersectionIndices() collects only 'point'-kind intersections, deduplicates them, and sorts by column.
• src/service/rotor.ts – Adds INTERSECTION_MODE to getAvailableModes(), routes moveLeft/Right to the new moveIntersection() helper, blocks moveUp/Down with a user-friendly message, and extracts buildMessage() to de-duplicate terse/verbose/off branching.
• src/util/constant.ts – Adds INTERSECTION_MODE = 'INTERSECTING POINT NAVIGATION'.
• Tests – New unit tests in test/model/line.test.ts (boundary cases, multi-way dedup, row scoping) and a new test/service/rotor.test.ts (service dispatch layer). New Playwright tests in e2e_tests/specs/multiLineplot.spec.ts.

---

Issues

1. Unrelated example file committed (examples/dodged_barplot_large.html) 🟡

The dodged_barplot_large.html example (1,347 lines) has no connection to intersection navigation. If this was generated for manual testing, it should be removed before merging to keep the diff reviewable and the repo lean. If it's intentional please explain why in the PR description.

2. buildMessage() evaluates both template strings before the isOff() short-circuit 🟢

In the refactored getMessage():

return this.buildMessage(
  `No ${navType} value found ${preposition}${direction}`,  // always evaluated
  `No ${navType} value found ${position}the current value.`,  // always evaluated
);

The original code returned '' immediately when text mode is off, without building either string. The new path constructs both strings and then discards them. This is negligible for short messages, but it's a behavioral change worth noting — especially for callers that may eventually pass expensive expressions as arguments.

Consider lazy evaluation if this pattern is used more widely:

private buildMessage(terse: () => string, verbose: () => string): string {
  if (this.text.isOff()) return '';
  return this.text.isTerse() ? terse() : verbose();
}

3. Known limitation (rotor index not reset on context change) is documented but creates a silent UX bug 🟡

The getMode() comment correctly identifies the issue:

If the user cycles to INTERSECTION_MODE and focus then moves to a trace that does not advertise that capability, the modulo below silently wraps the index onto a different mode without announcing the switch.

This pre-dates this PR (GRID_MODE has the same problem), but adding a second capability-gated mode makes this more likely to be hit. Since it's acknowledged and tracked separately, it's fine to leave for now — just worth highlighting for follow-up.

4. moveIntersection() uses instanceof AbstractTrace guard 🟢

The defensive check is correct and the comment explains why it could be reached. However, this means RotorNavigationService now has a concrete dependency on AbstractTrace. The existing pattern for isGridNavigable uses a type-guard function instead (isGridNavigable(activeTrace)), which is more loosely coupled. Consider whether moveIntersection should follow the same approach — or whether that's out of scope for this PR.

---

Positive Highlights

• finalizeExtremaNavigation() → finalizeNavigation() – A good rename that reflects the method's actual use (it's not exclusively for extrema navigation). All call sites updated correctly across bar, candlestick, heatmap, segmented, and line traces.

• Set-based deduplication in getPointIntersectionIndices() – Correctly handles the case where multiple line pairs share the same pointIndex (e.g., a three-way intersection). The unit test at 'deduplicates a multi-way point intersection shared across 3 lines' confirms this.

• moveToPrevIntersection() backward loop – The comment explaining why findLast was avoided is exactly the kind of non-obvious WHY the project style guide calls for.

• cycleRotorTo in both unit and E2E tests – Using the rotor's announced text as the stop condition (rather than a fixed press count) correctly future-proofs the test against rotor cycle order changes.

• Vertical navigation UX in intersection mode – Returning a distinct "Up/down unavailable in intersection mode" message (rather than a direction bound message) is the right accessibility call — it communicates why the key did nothing, not just that it did nothing.

• buildMessage() helper – The extraction removes the repeated isOff/isTerse branching that was duplicated across message helpers. Clean improvement.

---

Minor Nits

• rotor.ts moveUp() has a comment block explaining the const mode = this.getMode() caching optimization. Per the project's comment style, this is borderline — it explains a non-obvious performance micro-optimization, which qualifies, but the same comment is omitted from moveDown/Left/Right which does the same thing. Either add it everywhere or remove it (the optimization is straightforward enough to not need explanation).

• line.ts getPointIntersectionIndices() has a long "Sort rationale" and "Performance note" block. This is more doc than the project style guide typically recommends. The sort rationale is the useful part; consider trimming the performance note to a single sentence.

---

Verdict: Ready to merge after addressing item #1 (dodged_barplot_large.html). Items #2 and #4 are worth a quick decision but are not blockers.

[claude]

Code Review — PR #604: Intersecting Point Rotor Mode for Multiline Plots

Overview

This PR adds a new INTERSECTING POINT NAVIGATION rotor unit that lets users jump directly to data-point intersections between lines in a multiline plot using Left/Right arrows. Up/Down in this mode explicitly surface an "unavailable" message. The implementation follows the existing GRID_MODE rotor pattern cleanly.

---

Positives

• Good architecture adherence. The opt-in supportsIntersectionMode() / moveToNextIntersection() / moveToPrevIntersection() triple on AbstractTrace is the correct extension point — new modes should be transparent to traces that don't need them, and they are.
• finalizeExtremaNavigation → finalizeNavigation rename is a small, correct improvement that removes misleading specificity from a general-purpose helper.
• buildMessage() helper removes duplicated terse/verbose/off branching across message helpers — nice refactor.
• Test coverage is thorough. Unit tests cover: multiline/single-line capability guard, next/prev navigation, bound conditions, dedup of multi-way intersections, row-scoping, and the single-line exclusion guard. The E2E suite adds a realistic fixture with true point intersections (multiline_plot_intersection.html).
• Known limitation documented — the comment in getMode() about rotorIndex not resetting on context change is honest and flags the right follow-up work.

---

Issues and Suggestions

1. getPointIntersectionIndices() is called twice per move (minor perf)

// line.ts
public override moveToNextIntersection(): boolean {
    const indices = this.getPointIntersectionIndices();   // call 1
    ...
}
public override moveToPrevIntersection(): boolean {
    const indices = this.getPointIntersectionIndices();   // call 2
    ...
}

The comment says findAllIntersectionsForCurrentLine caches per-row, so this is not a correctness issue, but calling getPointIntersectionIndices (which iterates the result and builds a Set) twice in a single move is redundant. Could be extracted to a shared accessor if moveToPrev ever needs to call moveToNext internally, but it's minor as-is.

2. moveIntersection instanceof guard can silently fail if class hierarchy changes

// rotor.ts
if (!(activeTrace instanceof AbstractTrace)) {
    return this.buildMessage(...);
}

This guard is correct today, but INTERSECTION_MODE is only added to getAvailableModes() when activeTrace.supportsIntersectionMode() returns true, and supportsIntersectionMode() is defined on AbstractTrace. So if the active plot is not an AbstractTrace, this branch is already unreachable from getAvailableModes(). The guard is a useful defensive belt-and-suspenders, but the comment could be clearer: it's not protecting against a race condition in general — it's protecting against the specific case where the active context changes between the mode-list build and the keypress (already noted, just suggest tightening the comment phrasing).

3. The fixture HTML mixes string and numeric x values

// multiline_plot_intersection.html (old fixture — still present in the line layer data)
{ "x": "2", "y": 3.0 }   // string
{ "x": 3.0, "y": 5.0 }   // number

The old fixture had this inconsistency and it was carried forward. If intersection detection compares x values across lines for the "same point" check, a "3" vs 3.0 comparison would silently fail. The new fixture (10-point 3-series data) also mixes string/numeric x. Confirm that classifyIntersectionKind / the point-matching logic coerces types before comparing, or normalize to one type in the fixture data.

4. E2E test navigates using page.goto with a relative path — fragile baseline

await page.goto('examples/multiline_plot_intersection.html', { waitUntil: 'load' });

The comment explains the baseURL makes this work. That's fine, but the test has no fallback if the file doesn't exist. Consider adding a expect(page.url()).toContain('multiline_plot_intersection') assertion after the goto to surface a clear failure message rather than a confusing waitForSelector timeout if the fixture path ever changes.

5. Missing unit test: supportsIntersectionMode with exactly one line

The existing test covers length > 1 (multi) and the single-line case implicitly via the rotor exclusion test. A direct unit test asserting supportsIntersectionMode() === false for a single-line LineTrace would be a cleaner, more explicit contract check. (The rotor exclusion test does verify the observable behavior; this is a minor style note.)

6. moveToPrevIntersection col is not reset on failure

public override moveToPrevIntersection(): boolean {
    ...
    if (target === undefined) {
        return false;   // col is unchanged — correct
    }
    this.col = target;
    this.finalizeNavigation();
    return true;
}

This is actually correct — col is not mutated when returning false. Confirming this is intentional and matches moveToNextIntersection's pattern. ✅

---

Minor Nits

• The cycleRotorTo helper in the E2E spec is well-designed (avoids hard-coded press counts). Worth a brief mention in the PR that it was intentionally written this way so reviewers don't flag it as "magic loop".
• The createLineLayer function is duplicated in both test/model/line.test.ts and test/service/rotor.test.ts. Consider moving it to a shared test fixture file if test helpers grow further.

---

Summary

The implementation is solid and follows MAIDR's architectural patterns well. The main thing to verify before merging is the string/numeric x value mixing in fixture data (point 3) — if the intersection-detection code does not normalize types, this could cause silent misses in the real-world scenario the E2E test is trying to exercise. The rest of the notes are minor polish items.

[claude]

Code Review — feat: add intersecting point rotor mode for multiline plots

Overview

This PR adds INTERSECTING POINT NAVIGATION as a new rotor mode (Alt+Shift+Up/Down cycle) for multiline line plots. Left/Right navigate between point-type intersections on the active line, skipping slope intersections. The feature uses an opt-in capability pattern on AbstractTrace, mirrors the existing supportsGridMode() / supportsCompareMode() extension points, and includes unit, service, and E2E tests.

---

Architecture and Design

Strengths

• Good extensibility: The supportsIntersectionMode() / moveToNextIntersection() / moveToPrevIntersection() triple follows exactly the same opt-in contract as supportsGridMode() on scatter, making it easy to apply to future trace types.
• Correct MVVC boundary: The model (LineTrace) handles data navigation; the service (RotorNavigationService) handles orchestration and messaging; the view layer is untouched.
• buildMessage() refactor: Centralising the off/terse/verbose branching that was previously duplicated in getMessage() is a clean improvement. getMessage() retains its public signature with no behavioral regression.
• Caching getMode() once per keypress: Replacing four independent isGridMode() calls with a single const mode = this.getMode() at the top of each handler avoids redundant getAvailableModes() walks.

Potential issue — constructor is a breaking change

RotorNavigationService now requires a third NotificationService argument. The controller has been updated, but worth a grep to confirm new RotorNavigationService( appears only in controller.ts and the new test file — any missed call site will fail silently at runtime.

---

Correctness

Intersection logic

• getPointIntersectionIndices() filters for intersectionKind === 'point', deduplicates with a Set, and re-sorts by column index. The comment correctly identifies why the re-sort is needed (the upstream cache sorts by continuous x, not by column), and deduplication is verified by the 3-line test case.
• moveToNextIntersection / moveToPrevIntersection only mutate this.col, leaving this.row unchanged — correct since we navigate within the current line, not across lines.
• Both methods call finalizeNavigation(), which calls notifyStateUpdate(), so audio/text/braille all update correctly on a successful move.

Notification path for screen readers

The announceIntersectionMessage() to notification.notify() path for bound and vertical-unavailable messages is necessary and well-explained. One subtle point: when text mode is OFF, buildMessage() returns '', and announceIntersectionMessage('') still calls notification.notify(''). The code comments say NotificationService no-ops on empty input — worth either confirming that contract is documented on NotificationService, or adding a guard to make the intent explicit:

private announceIntersectionMessage(message: string): string {
  if (message) {
    this.notification.notify(message);
  }
  return message;
}

Known limitation documented

The rotorIndex not resetting on context change is clearly noted in a JSDoc comment. Documenting it rather than leaving it undocumented is the right call.

---

Test Coverage

The test suite is thorough:

Area	Covered
supportsIntersectionMode — multi vs. single line	yes
moveToNextIntersection — advance, boundary, no intersections	yes
moveToPrevIntersection — retreat, boundary, no intersections	yes
3-line deduplication	yes
Row-scoped intersections (row > 0)	yes
Service routing — all four directions	yes
Terse and off text modes	yes
notification.notify called for SR re-announce	yes
Repeat presses each call notify	yes
Single-line trace excluded from cycle	yes
E2E — cycle to mode, boundary messages, real fixture	yes

---

Minor Issues

1. Missing newline at end of examples/multiline_plot_intersection.html — the diff shows no newline at end of file. Easy fix.

2. notify('') defensive check — as above, a small guard in announceIntersectionMessage would make the empty-string no-op contract explicit rather than relying on NotificationService's internal behavior.

3. getMessage() refactored implicitly — getMessage() is public. Its behavior is identical to before, but the internal plumbing now delegates to buildMessage(). This is safe, but worth noting so reviewers don't have to trace it manually.

4. Example HTML attribute change — the old fixture used maidr-data="..." while the new one uses maidr="...". Not a bug if both are supported, but worth confirming this switch is intentional.

---

Summary

The implementation is solid, well-tested, and follows project conventions. Main items before merge:

• Verify no other new RotorNavigationService( call sites are missing the new NotificationService argument.
• Add newline at end of examples/multiline_plot_intersection.html.
• Consider adding the if (message) guard in announceIntersectionMessage, or confirm NotificationService.notify('') is a documented no-op.

Overall this is a well-crafted, accessibility-thoughtful feature. The notification/re-announce pattern for repeated identical messages is a subtle but important accessibility detail — nice work.

Review generated by Claude Sonnet 4.6

[xabilitylab]

🎉 This PR is included in version 3.66.0 🎉

The release is available on GitHub release

Your semantic-release bot 📦🚀