Interactive Graph Bug Fix: Ensure that we're bounding and snapping BE…

…FORE checking if the new destination results in a valid graph. (#1356)

## Summary:
This is a fix for an issue in our Sinusoid graphs, where moving the points to the edge of the graph would cause the entire browser to crash. I believe that this was due the fact that we were bounding and snapping our points to the graph settings AFTER determining if the newDestination is a valid location, which means that points placed at the edge of the graph could end up in an invalid location. This made it possible to get the graph into a bit of an infinite state. 

By moving the boundAndSnap logic prior to checking whether it is a valid location, we can make sure that this loop is impossible. Repeating this order of operations for the Quadratic Graph actually manages to solve a separate issue that was considered a "Won't Do" earlier! :) 

Issue: LEMS-2064 & LEMS-2066

## Test plan:
- Manual testing in webapp (local dev) to confirm bug is reproducible 
- Manual testing with PR snapshot in webapp to confirm bug is solved.

Author: SonicScrewdriver

Reviewers: nishasy, benchristel, #perseus

Required Reviewers:

Approved By: nishasy

Checks: ✅ codecov/project, ✅ codecov/patch, ✅ Upload Coverage (ubuntu-latest, 20.x), ✅ Publish npm snapshot (ubuntu-latest, 20.x), ✅ Lint, Typecheck, Format, and Test (ubuntu-latest, 20.x), ✅ Jest Coverage (ubuntu-latest, 20.x), ✅ Cypress (ubuntu-latest, 20.x), ✅ Check builds for changes in size (ubuntu-latest, 20.x), ✅ Publish Storybook to Chromatic (ubuntu-latest, 20.x), ✅ Check for .changeset entries for all changed files (ubuntu-latest, 20.x), ✅ gerald

Pull Request URL: #1356

.changeset/smooth-worms-wave.md

@@ -0,0 +1,5 @@
+---
+"@khanacademy/perseus": patch
+---
+
+A fix for performance issues related to Sinusoid and Quadratic graphs

packages/perseus/src/widgets/interactive-graphs/graphs/sinusoid.test.ts

@@ -18,16 +18,29 @@ describe("SinusoidGraph", () => {
         expect(getSinusoidCoefficients(coords)).toEqual(expected);
     });
 
-    it("should accurately calculate the sine wave", () => {
+    it("should accurately calculate the sine wave for a given x-coordinate", () => {
         const coords: SinusoidGraphState["coords"] = [
             [0, 0],
             [2, 2],
         ];
+
+        // Ensure that the coefficients are defined
         const coefficients = getSinusoidCoefficients(coords);
+        expect(coefficients).toBeDefined();
 
+        // Grab a point where the sine wave should be 0
         const pointToTest = coords[0][0] + 4;
 
         // The sine wave should be roughly 0 at this point when accounting for floating point errors
-        expect(Math.round(computeSine(pointToTest, coefficients))).toEqual(0);
+        // We already know that the coefficients are defined from the previous test
+        expect(Math.round(computeSine(pointToTest, coefficients!))).toEqual(0);
+    });
+
+    it("should return undefined when the coefficients are invalid", () => {
+        const coords: SinusoidGraphState["coords"] = [
+            [0, 0],
+            [0, 0],
+        ];
+        expect(getSinusoidCoefficients(coords)).toBe(undefined);
     });
 });

packages/perseus/src/widgets/interactive-graphs/graphs/sinusoid.tsx

@@ -26,12 +26,29 @@ export function SinusoidGraph(props: SinusoidGraphProps) {
     // The coords[0] is the root and the coords[1] is the first peak
     const {coords} = graphState;
 
-    // Get the coefficients for calculating the quadratic equation
-    const coeffs: SineCoefficient = getSinusoidCoefficients(coords);
+    // The coefficients are used to calculate the sinusoid equation, plot the graph, and to indicate
+    // to content creators the currently selected "correct answer" in the Content Editor.
+    // While we should technically never have invalid coordinates, we want to ensure that
+    // we have a fallback so that the graph can still be plotted without crashing.
+    const coeffRef = React.useRef<SineCoefficient>({
+        amplitude: 1,
+        angularFrequency: 1,
+        phase: 1,
+        verticalOffset: 0,
+    });
+    const coeffs = getSinusoidCoefficients(coords);
+
+    // If the coefficients are valid, update the reference
+    if (coeffs !== undefined) {
+        coeffRef.current = coeffs;
+    }
 
     return (
         <>
-            <Plot.OfX y={(x) => computeSine(x, coeffs)} color={color.blue} />
+            <Plot.OfX
+                y={(x) => computeSine(x, coeffRef.current)}
+                color={color.blue}
+            />
             {coords.map((coord, i) => (
                 <StyledMovablePoint
                     key={"point-" + i}
@@ -63,11 +80,16 @@ export const computeSine = function (
 
 export const getSinusoidCoefficients = (
     coords: ReadonlyArray<Coord>,
-): SineCoefficient => {
+): SineCoefficient | undefined => {
     // It's assumed that p1 is the root and p2 is the first peak
     const p1 = coords[0];
     const p2 = coords[1];
 
+    // If the x-coordinates are the same, we are unable to calculate the coefficients
+    if (p2[0] === p1[0]) {
+        return;
+    }
+
     // Resulting coefficients are canonical for this sine curve
     const amplitude = p2[1] - p1[1];
     const angularFrequency = Math.PI / (2 * (p2[0] - p1[0]));

packages/perseus/src/widgets/interactive-graphs/reducer/interactive-graph-reducer.test.ts

@@ -182,6 +182,25 @@ describe("moveControlPoint", () => {
         ]);
     });
 
+    it("does not allow moving an endpoint of a sinusoid if the bounding logic would result in an invalid graph", () => {
+        const state: InteractiveGraphState = {
+            ...baseSinusoidGraphState,
+            coords: [
+                [9, 1],
+                [10, 2],
+            ],
+        };
+
+        const updated = interactiveGraphReducer(state, movePoint(0, [15, 1]));
+
+        invariant(updated.type === "sinusoid");
+        // Assert: the move was canceled
+        expect(updated.coords).toEqual([
+            [9, 1],
+            [10, 2],
+        ]);
+    });
+
     it("snaps points to the snap grid", () => {
         const state: InteractiveGraphState = {
             ...baseSegmentGraphState,

packages/perseus/src/widgets/interactive-graphs/reducer/interactive-graph-reducer.ts

@@ -286,28 +286,38 @@ function doMovePoint(
             };
         }
         case "sinusoid": {
-            // First, we need to verify that the new coordinates are not on the same vertical line
-            // If they are, we don't want to move the point
+            // First, we need to make sure to bound the new coordinates to the graph range
             const destination = action.destination;
+            const boundDestination = boundAndSnapToGrid(destination, state);
+
+            // Then, we need to verify that the new coordinates are not on the same
+            // vertical line. If they are, then we don't want to move the point
             const newCoords: vec.Vector2[] = [...state.coords];
-            newCoords[action.index] = action.destination;
+            newCoords[action.index] = boundDestination;
             if (newCoords[0][0] === newCoords[1][0]) {
                 return state;
             }
+
             return {
                 ...state,
                 hasBeenInteractedWith: true,
                 coords: setAtIndex({
                     array: state.coords,
                     index: action.index,
-                    newValue: boundAndSnapToGrid(destination, state),
+                    newValue: boundDestination,
                 }),
             };
         }
         case "quadratic": {
             // Set up the new coords and check if the quadratic coefficients are valid
             const newCoords: QuadraticCoords = [...state.coords];
-            newCoords[action.index] = action.destination;
+
+            // Bind the new destination to the graph range/snapStep and then get the quadratic coefficients
+            const boundDestination = boundAndSnapToGrid(
+                action.destination,
+                state,
+            );
+            newCoords[action.index] = boundDestination;
             const QuadraticCoefficients = getQuadraticCoefficients(newCoords);
 
             // If the new destination results in an invalid quadratic equation, we don't want to move the point
@@ -321,7 +331,7 @@ function doMovePoint(
                 coords: setAtIndex({
                     array: state.coords,
                     index: action.index,
-                    newValue: boundAndSnapToGrid(action.destination, state),
+                    newValue: boundDestination,
                 }),
             };
         }