feat: go to intersection

[soundaryakp1999]

Pull Request

Description

Implemented intersections in the go to feature, accessible with the g key. In a multiline plot, the g key in addition to the extremas, now lists all the points where the currently selected line intersects another line.

Related Issues

Changes Made

• Only in multiline plot: when g is pressed on a line, all points are listed where other line(s) intersect.
• This scans and displays floating point values, as the exact intersection was often skipped by just considering integers.

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

Thanks for adding intersection navigation to the Go To feature - this is a meaningful accessibility enhancement for multiline plots. The geometric intersection algorithm is correct and well-documented. Below are issues that should be addressed before merging.

---

Bugs

1. Non-unique React key props (GoToExtrema.tsx)

The key built from segment-type-pointIndex is not guaranteed to be unique. Two distinct intersections can map to the same nearest pointIndex (e.g., two intersections on the same segment both closest to endpoint 3). This causes silent React rendering issues. Consider including the label field in the key to ensure uniqueness.

2. x: number | string return type is always number (line.ts ~L633)

findAllIntersectionsForCurrentLine declares x: number | string in its return type, but getSegmentIntersection always returns a numeric x. This misleads callers and adds unnecessary type widening. Remove string from the union.

3. intersectingLines always includes the current group (line.ts ~L698-705)

The current group index is always added to the intersectingLines set, so getIntersectionLabel returns something like "Line A (current), Line B". Since the user is already on Line A, only the other line(s) should appear in the label. Filter out currentGroup before building the return array.

---

Design / Fragility

4. String parsing for display logic (GoToExtrema.tsx ~L348-358)

Display labels are constructed in getExtremaTargets with a specific string format, then re-parsed in the UI component using replace/split. If the label format ever changes, this breaks silently. Consider adding structured display fields to ExtremaTarget (e.g., displayCoords?: string, displayLines?: string) instead of parsing strings in the component.

5. Semantic mismatch: intersections in ExtremaNavigable

The existing interface, type field (max | min), and dialog title "Go To Extrema" are specifically about statistical extrema. Intersections are not extrema. At minimum the dialog title/description should be updated (e.g., "Go To Points of Interest"). Longer-term, a separate intersectionTargets field rather than mixing types in one array would be cleaner.

---

Performance

6. O(n x m^2) intersection scan on every dialog open (line.ts ~L633)

findAllIntersectionsForCurrentLine iterates over every segment pair each time getExtremaTargets is called. For large datasets with many lines this can be expensive. Results should be cached and invalidated when the active row changes.

7. Intersection grouping by rounded x may be imprecise (line.ts ~L709-713)

Rounding to 2 decimal places can merge nearby but distinct intersections, or separate intersections that are genuinely close. A tolerance-based deduplication (comparing |xi - xj| < epsilon) would be more robust.

---

Missing Tests

The PR checklist marks "I have added appropriate unit tests" as unchecked. The new algorithmic logic (getSegmentIntersection, findAllIntersectionsForCurrentLine) is non-trivial and should have unit tests covering:

• Parallel lines (no intersection)
• Lines that cross within the segment bounds
• Lines whose extensions cross but the actual segments do not
• A 3-line plot where all three lines cross at the same point

---

Minor

• getIntersectionLabel accesses this.points[lineIndex][0]?.fill - the optional chain prevents crashes, but a brief comment explaining the fallback to Line N would help.
• The version bump in package-lock.json (3.50.0 to 3.51.0) is unusual for a feature PR; confirm this is intentional.

[claude]

PR Review: feat: go to intersection

Great feature addition! The intersection detection for multiline plots is a meaningful accessibility improvement. Below is a detailed review covering architecture, correctness, and test quality.

---

Architecture Concerns

1. Presentation logic in model types (Medium)

The new display field added to ExtremaTarget in src/type/extrema.ts contains pre-formatted strings:

display?: {
  coords?: string;    // e.g., "x=1.50, y=2.50"
  otherLines?: string; // e.g., "Line A, Line B"
};

Per MVVC architecture, formatting strings for display belongs in the ViewModel or View layer, not in model types. The model should expose raw data (numbers, indices), and let higher layers format it. Consider removing display from ExtremaTarget and computing these strings in GoToExtrema.tsx directly from target.x, target.y, and target.intersectingLines.

Similarly, the intersectingLines?: number[] field is fine as raw data in the model type, but the formatted label (getIntersectionLabel) should ideally live in the ViewModel, not in LineTrace.

---

Correctness Issues

2. Distance calculation uses only x-axis distance (Bug)

When choosing the nearest point index to an intersection, only the x-distance is compared:

// src/model/line.ts
const distToStart = Math.abs(intersection.x - seg1Start.x);
const distToEnd = Math.abs(intersection.x - seg1End.x);
const nearestPointIndex = distToStart <= distToEnd ? segIndex : segIndex + 1;

This is incorrect for nearly vertical line segments where x-coordinates of both endpoints are very similar but y-coordinates differ significantly. Use Euclidean distance instead:

const distToStart = Math.hypot(intersection.x - seg1Start.x, intersection.y - seg1Start.y);
const distToEnd = Math.hypot(intersection.x - seg1End.x, intersection.y - seg1End.y);

3. getIntersectionLabel relies on fill for line name (Minor)

// src/model/line.ts
const firstPoint = this.points[lineIndex][0];
return firstPoint?.fill || `Line ${lineIndex + 1}`;

Using firstPoint?.fill to get a line's display name is informal — fill is a visual styling property, not a semantic label. If there's a dedicated title or series name property on the layer/trace, prefer that. If fill is the established convention in this codebase, add a comment explaining why.

4. Deduplication epsilon may be too strict (Minor)

INTERSECTION_EPSILON = 1e-6 works for coordinates in the range [-1000, 1000], but will silently fail to deduplicate intersections when data uses large values (e.g., timestamps in milliseconds, dollar amounts in millions). Consider using a relative epsilon or one derived from the data range.

5. Cache never cleared on disposal (Minor)

The intersectionCache is a Map that grows with each unique row accessed. It is not cleared in any dispose() method. For normal chart sizes this is fine, but it should be cleaned up to avoid memory leaks if the trace is long-lived. Add this.intersectionCache.clear() in a disposal step.

---

View Layer Issues

6. labelParts[1] can be undefined (Bug)

In GoToExtrema.tsx:

const labelParts = target.label.split(' at ');
displayLabel = `${labelParts[0]} Value: ${target.value.toFixed(2)} at ${labelParts[1]}`;

If the label does not contain " at ", labelParts[1] is undefined, and the displayed string will literally read "... at undefined". Add a guard or use the display field (once it's available for min/max targets too) to avoid this fragile string parsing.

---

Test Quality

7. Unused variable in new test cases

All five new tests assign multiLineplotPage but never use it:

const multiLineplotPage = await setupMultiLineplotPage(page);
// multiLineplotPage is never referenced below

This will likely trigger a lint error (no-unused-vars). Either remove the assignment and just await setupMultiLineplotPage(page), or use the variable if it provides useful helpers.

8. Conditional test assertion provides false confidence

if (await intersectionTarget.count() > 0) {
  await intersectionTarget.click();
  await expect(dialog).not.toBeVisible({ timeout: 5000 });
}

If no intersection target is found, this test passes silently without actually testing anything. Replace the if guard with an explicit assertion that intersections are present, or use test.skip() with a clear condition:

// Either:
await expect(intersectionTarget).toHaveCount({ min: 1 }); // fail fast
await intersectionTarget.click();

// Or more explicit:
const count = await intersectionTarget.count();
if (count === 0) {
  test.skip(true, 'No intersection targets found in test data');
}

---

Suggestions

• The description change from "Navigate to statistical extremes within the current ${traceType}" → "Navigate to points of interest..." is a good improvement for accessibility clarity.
• Renaming the dialog title from "Go To Extrema" to "Go To" is appropriate since the feature now covers more than extrema.
• The caching strategy (per-row, computed once) is sound for static chart data.
• The use of Set<number> for deduplicating intersecting lines is a clean approach.

---

Summary

Severity	Count	Notes
Bug	2	Euclidean distance (#2), undefined in string (#6)
Architecture	1	Presentation logic in model types (#1)
Minor	3	fill for naming (#3), epsilon scale (#4), cache leak (#5)
Test	2	Unused variable (#7), conditional assertion (#8)

The core intersection algorithm (getSegmentIntersection) is mathematically correct and the feature concept is solid. Addressing the distance calculation bug and the architecture concern around display fields would put this in good shape for merge.

[claude]

Code Review — Follow-up

Thanks for addressing the issues from the previous review — the Euclidean distance fix, caching, epsilon-based deduplication, the display field, key uniqueness with index, and the labelParts[1] guard are all good improvements. A few remaining concerns:

---

Bugs

1. fill is a color value, not a line name (line.ts ~L838)

const firstPoint = this.points[lineIndex][0];
return firstPoint?.fill || `Line ${lineIndex + 1}`;

fill is a visual styling attribute (e.g. "#1f77b4"). When it is defined, the intersection label will read "Intersection with #1f77b4 at x=..., y=...", which is meaningless to a user. The fallback Line N is correct and should be the primary path, not the fallback. Check whether the trace data carries a dedicated name or label field for the series and prefer that, or always use the ordinal fallback.

2. Inconsistent fallback format between getIntersectionLabel and moveOnce (line.ts)

The new getIntersectionLabel falls back to Line N (capital L, word "Line"), while the existing moveOnce uses lN (lowercase letter l). These represent the same concept and should be consistent. Decide on one format and use it everywhere.

---

Minor Issues

3. intersectionCache not cleared in dispose() (line.ts ~L67)

The dispose() method clears points, min, and max but not intersectionCache. Add this.intersectionCache.clear(); to avoid a minor memory leak.

4. Misleading cache comment (line.ts ~L44)

// Invalidated when active row changes

The cache is never invalidated — it accumulates per-row results throughout the trace's lifetime. Since chart data is static this is fine in practice, but the comment is incorrect and could mislead future maintainers. Change it to something like: "Computed lazily once per row; static data ensures results remain valid."

5. Absolute INTERSECTION_EPSILON scale (line.ts ~L638)

INTERSECTION_EPSILON = 1e-6 is fine for datasets in the range 0-1, but for typical chart values (e.g. 0-1000), two floating-point computations of the same intersection can easily differ by more than 1e-6, causing the same crossing to appear twice in the list. Consider scaling by the data range or using a relative tolerance.

6. display field not populated for min/max targets

The new display field on ExtremaTarget is only populated for intersection targets. The UI component still string-splits target.label for min/max display. Populating display for min/max targets too would let the UI drop the string parsing entirely, making both paths consistent and robust.

---

Tests

7. Intersection tests are data-dependent without documentation

Several tests assert expect(intersectionTargets.length).toBeGreaterThan(0) without documenting that the shared fixture actually contains crossing lines. If the fixture is updated to use non-intersecting data, these tests will fail with an opaque message. Add a comment like // The default multiline fixture has 3 lines with known crossings to make the assumption explicit.

---

Summary

Severity	#	Issue
Bug	2	fill displays raw color (#1), inconsistent fallback format (#2)
Minor	4	Cache not cleared (#3), misleading comment (#4), absolute epsilon (#5), display field gap (#6)
Test	1	Undocumented data dependency (#7)

The algorithm and overall structure are sound. The two bugs around line naming (items 1 & 2) are the most user-visible — a screen reader user hearing "Intersection with #1f77b4" gets no meaningful information. Everything else is minor polish.

[nk1408]

I tested the feature, It found the intersaction points (same x & y value) but the issue is we dont have that exact point as svg element in the image. So goto doesn't exactly land at the intersection point as specified in the options (it lands at the closest available svg point). I think we may have to re-think a bit of how we have to design this. We can discuss further in tomorrow's call.

[nk1408]

The navigation model is discrete (index-based: this.col = N), so it can only land on actual data points. Geometric intersections occur between data points and there's no mechanism to represent or navigate to an intermediate position.

We can have two possible solutions:

1. Remove geometric intersections from "Go To" — Only show intersections that fall exactly on data points (use the existing findIntersections() at line.ts:300). This means fewer (possibly zero) intersection targets, but each one would be accurate.
2. Add interpolated navigation support — Create a virtual data point at the intersection coordinates so the cursor can actually land there. This is a larger change requiring updates to the highlight system, text/audio state, and the navigation model.

[claude]

Code Review - PR 563 (feat: go to intersection)

Great feature addition! Adding intersection navigation to multiline plots is a meaningful accessibility improvement. Here are my observations.

BUGS AND CORRECTNESS ISSUES

1. Silent failure with categorical x-values
   LinePoint.x is typed as number|string in grammar.ts. In findAllIntersectionsForCurrentLine, x values are converted with Number(p1.x). When x is categorical (e.g., month names like Jan, Feb), Number() returns NaN and no intersections are found silently. Consider adding an early-exit guard and documenting that this feature only works with numeric x-axes.

2. Deduplication tolerance may be too strict
   INTERSECTION_EPSILON = 1e-6 is very small. Two computations of the same geometric intersection arriving from different segment pairs can yield slightly different floating-point results (e.g., 0.5000001 vs 0.5000003) that fall outside this epsilon and will not be deduplicated. A slightly larger tolerance such as 1e-4 would be more robust for typical coordinate ranges.

3. Misleading cache comment
   The field is annotated 'Invalidated when active row changes' but no invalidation code exists anywhere. The cache is retained for the lifetime of the object. That is actually correct since this.points is immutable, but the comment is wrong and will confuse future maintainers. Change it to something like: 'Cache is permanent since point data is immutable.'

DESIGN AND ARCHITECTURE CONCERNS

4. Navigation snaps to nearest data point, not the actual intersection
   The dialog displays computed floating-point coordinates such as x=1.50, y=2.50, but navigateToExtrema sets this.col = target.pointIndex which is the nearest existing data point. If the intersection falls between two data points, the user sees one coordinate in the dialog but lands at a different position. The aria-label or visible text should reflect where navigation actually goes, not the exact geometric intersection.

5. Display formatting in the Model layer
   getExtremaTargets() builds display.coords with .toFixed(2) hardcoded in the model. Per the MVVC architecture in this project, formatting decisions belong in the View or ViewModel layers, not the Model. Consider passing raw numbers and letting the UI format them.

6. Two separate intersection algorithms with no cross-reference
   There is already a findIntersections() method used for audio state. The new findAllIntersectionsForCurrentLine() uses a different approach (segment math vs. exact point matching). This is intentional and fine, but a brief comment cross-referencing them would help future readers understand why both exist.

MINOR ISSUES

7. getIntersectionLabel lacks a bounds check - this.points[lineIndex][0] is accessed without first checking that this.points[lineIndex] exists. Use this.points[lineIndex]?.[0] to be safe.

8. Spurious blank line in package-lock.json - an unnecessary blank line was added after the opening brace and should be reverted to keep the diff clean.

9. Weak e2e assertion - expect(text).toContain('with') only verifies the word 'with' appears. A stronger assertion like toMatch(/Intersection with .+ at x=/) would better validate the actual format.

WHAT IS DONE WELL

• The caching strategy (Map keyed by row index) avoids recomputing on every getExtremaTargets() call - good performance design.
• The display field on ExtremaTarget cleanly avoids string-parsing in the UI layer.
• The getSegmentIntersection algorithm is mathematically correct and properly handles the parallel-lines edge case.
• Good e2e test coverage for the new dialog behavior.
• The title rename from 'Go To Extrema' to 'Go To' is the right UX call given the expanded scope.

[xabilitylab]

🎉 This PR is included in version 3.55.0 🎉

The release is available on GitHub release

Your semantic-release bot 📦🚀