Interactive Graph Bug Fix: Ensure that we're bounding and snapping BEFORE checking if the new destination results in a valid graph.

[SonicScrewdriver]

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.

[khan-actions-bot]

Gerald

Required Reviewers

• @Khan/perseus for changes to .changeset/smooth-worms-wave.md, packages/perseus/src/widgets/interactive-graphs/graphs/sinusoid.test.ts, packages/perseus/src/widgets/interactive-graphs/graphs/sinusoid.tsx, packages/perseus/src/widgets/interactive-graphs/reducer/interactive-graph-reducer.test.ts, packages/perseus/src/widgets/interactive-graphs/reducer/interactive-graph-reducer.ts

---

Don't want to be involved in this pull request? Comment #removeme and we won't notify you of further changes.

[github-actions]

npm Snapshot: Published

Good news!! We've packaged up the latest commit from this PR (6380fc1) and published it to npm. You
can install it using the tag PR1356.

Example:

yarn add @khanacademy/perseus@PR1356

If you are working in Khan Academy's webapp, you can run:

./dev/tools/bump_perseus_version.sh -t PR1356

[github-actions]

Size Change: +169 B (+0.02%)

Total Size: 845 kB

Filename	Size	Change
packages/perseus/dist/es/index.js	407 kB	+169 B (+0.04%)

ℹ️ View Unchanged

Filename	Size
packages/kas/dist/es/index.js	38.2 kB
packages/kmath/dist/es/index.js	4.26 kB
packages/math-input/dist/es/index.js	80.1 kB
packages/math-input/dist/es/strings.js	1.73 kB
packages/perseus-core/dist/es/index.js	906 B
packages/perseus-editor/dist/es/index.js	271 kB
packages/perseus-error/dist/es/index.js	866 B
packages/perseus-linter/dist/es/index.js	21.8 kB
packages/perseus/dist/es/strings.js	3.21 kB
packages/pure-markdown/dist/es/index.js	3.67 kB
packages/simple-markdown/dist/es/index.js	12.4 kB

_{compressed-size-action}

[codecov]

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 70.91%. Comparing base (e73373f) to head (6380fc1).
Report is 8 commits behind head on main.

Additional details and impacted files

@@            Coverage Diff             @@
##             main    #1356      +/-   ##
==========================================
+ Coverage   69.74%   70.91%   +1.16%     
==========================================
  Files         488      493       +5     
  Lines      103291   103478     +187     
  Branches     7454    10529    +3075     
==========================================
+ Hits        72045    73377    +1332     
+ Misses      31130    30101    -1029     
+ Partials      116        0     -116     

Files	Coverage Δ
...src/widgets/interactive-graphs/graphs/sinusoid.tsx	100.00% <100.00%> (ø)
...active-graphs/reducer/interactive-graph-reducer.ts	94.31% <100.00%> (+7.28%)	⬆️

... and 141 files with indirect coverage changes

---

Continue to review full report in Codecov by Sentry.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update e73373f...6380fc1. Read the comment docs.

[nishasy]

Neat! Good job tracking this down too 🕵🏽

[benchristel]

Can you add a test for the changed behavior? Also, do you know where the infinite loop was occurring? Did it happen because Mafs tried to plot an infinite number of points? I'm not seeing any potential for infinite loops in the old reducer logic itself.

If the infinite loop is in Mafs, then we might want to submit a bugfix upstream. I could take that on.

[SonicScrewdriver]

Can you add a test for the changed behavior? Also, do you know where the infinite loop was occurring? Did it happen because Mafs tried to plot an infinite number of points? I'm not seeing any potential for infinite loops in the old reducer logic itself.

If the infinite loop is in Mafs, then we might want to submit a bugfix upstream. I could take that on.

@benchristel Ooo I absolutely will! I didn't add one initially as I wasn't sure what it would really show 🤔, but I can definitely see two tests that I could add now:

• Add a test to the reducer to ensure that a point cannot be placed at an invalid location, even if it's at the very edge of the graph.
• After updating the logic in the sinusoid component (see notes below), make sure that we fall back to rendering the last "valid" coordinates for our graph when we are provided coordinates that result in an invalid graph / having to calculate by infinity.

Key Investigation notes:

• I was never able to get any kind of error state to appear as the whole browser would freeze and crash (blocking further discovery), so I'm not confident in the full identification of the core issue.
• I have been utterly unable to recreate the issue in Perseus myself - it seems to be something connected with webapp. I tried testing in Flipbook, a new (local) Storybook story for testing Sinusoid+Mafs, and our Storybook Interactive Graph Editor w/ Mafs story.
• It's possible it could be related to how we're always getting the user's current answer for Khanmigo, which would be invalid at this point, but I am unable to verify yet.

What is the issue?

The issue, as far as I can tell, was as follows:

• A sinusoid graph shouldn't be allowed to hit a position that results in an invalid graph that requires dividing by infinity
• We have bounding logic to ensure we keep our points bound to the range of the graph
• We were binding points to the graph AFTER doing the invalid location check, which meant the graph could hit this invalid state when users tried to place a point past the edge of the graph if the other control point is also on the graph (x-axis) edge. For example, if the graph range is [-10, 10], with one point already at [10, 2] and the user tries to drag the second point to [12, 1] - then we would determine this as a valid location based on the current logic. THEN we'd run our bound logic, which would bounce the point back to [10,1], and result in an invalid graph state.
• We have an equation for calculating coefficients in the Sinusoid component that does not have the safety check / fallback that Quadratic does when the coefficients hit "infinity".
• Our computeSine currently logic runs whether the coefficients are valid or not, and the result is provided directly into the mafs graph.

Next Steps for this ticket:

Given this .. I feel like the issue probably isn't with mafs so much as something that was happening between webapp and Perseus when the graph was allowed to hit the invalid state. I feel like I should be able to recreate this issue in Perseus if it was an issue with Mafs specifically.

With the logic added prior to this comment, it should no longer be possible at all to hit this invalid state. That being said, I'll still work on adding the same fallback checks to Sinusoid as we are using in Quadratic just to keep things safe and consistent! I'll also add the two tests I mentioned above.

In terms of completing the investigation:

I'll make a new DRAFT PR on a different branch with some strategic console logs/logic changes to test my theory that it's the fact that the sinusoid graph is allowed to become invalid at all that results in the browser crashing. I'll do these tests tomorrow morning and report back here if I'm able to find the exact root cause.

If I'm still not able to find anything concrete about this exact root cause after these tests, I may simply leave a note on LEMS-2056 about investigating this issue further with a summary of the current discovery. LEMS-2056 is an investigation ticket made to specifically look into performance issues with our Graphs, with a particular focus on Quadratics and Sinusoids.

Hopefully that clearly explains my discoveries so far! I'm happy to jump on a call tomorrow if you want to discuss further or if an alternative approach is desired. :)

[benchristel]

@SonicScrewdriver Thanks for that write-up! Really interesting that it's only reproducible in webapp. I'm going to do a timeboxed investigation into what the issue might be there.