Skip to content

Commit 3b0b1c7

Browse files
nishasynishasy
authored
[LX] Stop locked functions from memory leaking (#2313)
## Summary: It looks like locked functions - specifically locked exponential functions (y = e^x) - cause a memory leak and completely brick the webpage. One solution is to add bounds behind the scenes. Instead of -Infinity to Infinity, we can clamp the bounds to the range of the graph. Issue: https://khanacademy.atlassian.net/browse/LEMS-2899 ## Test plan: `yarn jest packages/perseus/src/widgets/interactive-graphs/locked-figures/locked-function.test.ts` Storybook - All locked function stories: http://localhost:6006/?path=/docs/perseus-widgets-interactive-graph-locked-functions--docs - Story that was causing storybook to freeze: http://localhost:6006/?path=/story/perseus-widgets-interactive-graph-locked-functions--exponent Author: nishasy Reviewers: mark-fitzgerald, nishasy, benchristel, SonicScrewdriver, catandthemachines Required Reviewers: Approved By: benchristel, mark-fitzgerald Checks: ✅ 8 checks were successful Pull Request URL: #2313
1 parent 87420d7 commit 3b0b1c7

File tree

7 files changed

+246
-4
lines changed

7 files changed

+246
-4
lines changed

‎.changeset/thirty-pumpkins-smash.md

Lines changed: 5 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,5 @@
1+
---
2+
"@khanacademy/perseus": patch
3+
---
4+
5+
[LX] Stop locked functions from memory leaking

‎packages/perseus/src/widgets/interactive-graphs/interactive-graph.testdata.ts

Lines changed: 11 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -866,6 +866,17 @@ export const segmentWithLockedFunction = (
866866
.build();
867867
};
868868

869+
export const segmentWithLockedFunctionAndAsymmetricRange = (
870+
equation: string = "x^2",
871+
options?: LockedFunctionOptions,
872+
): PerseusRenderer => {
873+
return interactiveGraphQuestionBuilder()
874+
.withXRange(-5, 5)
875+
.withYRange(-10, 10)
876+
.addLockedFunction(equation, options)
877+
.build();
878+
};
879+
869880
export const segmentWithLockedLabels: PerseusRenderer =
870881
interactiveGraphQuestionBuilder()
871882
.addLockedLabel("small $\\frac{1}{2}$", [-6, 2], {

‎packages/perseus/src/widgets/interactive-graphs/locked-figures/locked-function.test.tsx

Lines changed: 137 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,137 @@
1+
import {render} from "@testing-library/react";
2+
import * as React from "react";
3+
4+
import {Dependencies} from "@khanacademy/perseus";
5+
6+
import {testDependencies} from "../../../../../../testing/test-dependencies";
7+
import {MafsGraph} from "../mafs-graph";
8+
import {getBaseMafsGraphPropsForTests} from "../utils";
9+
10+
import * as Utils from "./utils";
11+
12+
import type {NoneGraphState} from "../types";
13+
import type {LockedFunctionType} from "@khanacademy/perseus-core";
14+
15+
const baseMafsGraphProps = getBaseMafsGraphPropsForTests();
16+
const baseLockedFunctionProps = {
17+
type: "function",
18+
color: "grayH",
19+
strokeStyle: "solid",
20+
equation: "x/2",
21+
directionalAxis: "x",
22+
domain: [-10, 10],
23+
} satisfies LockedFunctionType;
24+
25+
describe("LockedFunction", () => {
26+
beforeEach(() => {
27+
jest.spyOn(Dependencies, "getDependencies").mockReturnValue(
28+
testDependencies,
29+
);
30+
});
31+
32+
test("calls clampedDomain with the x range correct arguments when in x direction", () => {
33+
// Arrange
34+
const clampSpy = jest.spyOn(Utils, "clampDomain");
35+
36+
// Act
37+
const graphStateWithRange = {
38+
type: "none",
39+
range: [
40+
[-10, 10],
41+
[-5, 5],
42+
],
43+
hasBeenInteractedWith: false,
44+
snapStep: [1, 1],
45+
} satisfies NoneGraphState;
46+
47+
const {container} = render(
48+
<MafsGraph
49+
{...baseMafsGraphProps}
50+
state={graphStateWithRange}
51+
lockedFigures={[
52+
{...baseLockedFunctionProps, directionalAxis: "x"},
53+
]}
54+
/>,
55+
);
56+
57+
// eslint-disable-next-line testing-library/no-container, testing-library/no-node-access
58+
const lockedFunction = container.querySelector(
59+
".locked-function",
60+
) as SVGGElement;
61+
62+
// Assert
63+
expect(lockedFunction).toBeInTheDocument();
64+
expect(clampSpy).toHaveBeenCalledWith(
65+
baseLockedFunctionProps.domain,
66+
graphStateWithRange.range[0], // x range
67+
);
68+
});
69+
70+
test("calls clampedDomain with the y range correct arguments when in y direction", () => {
71+
// Arrange
72+
const clampSpy = jest.spyOn(Utils, "clampDomain");
73+
74+
// Act
75+
const graphStateWithRange = {
76+
type: "none",
77+
range: [
78+
[-5, 5],
79+
[-10, 10],
80+
],
81+
hasBeenInteractedWith: false,
82+
snapStep: [1, 1],
83+
} satisfies NoneGraphState;
84+
85+
const {container} = render(
86+
<MafsGraph
87+
{...baseMafsGraphProps}
88+
state={graphStateWithRange}
89+
lockedFigures={[
90+
{...baseLockedFunctionProps, directionalAxis: "y"},
91+
]}
92+
/>,
93+
);
94+
95+
// eslint-disable-next-line testing-library/no-container, testing-library/no-node-access
96+
const lockedFunction = container.querySelector(
97+
".locked-function",
98+
) as SVGGElement;
99+
100+
// Assert
101+
expect(lockedFunction).toBeInTheDocument();
102+
expect(clampSpy).toHaveBeenCalledWith(
103+
baseLockedFunctionProps.domain,
104+
graphStateWithRange.range[1], // y range
105+
);
106+
});
107+
108+
test("does not render when the domain is outside the graph bounds", () => {
109+
// Arrange
110+
const graphStateWithRange = {
111+
type: "none",
112+
range: [
113+
[-10, 10],
114+
[-5, 5],
115+
],
116+
hasBeenInteractedWith: false,
117+
snapStep: [1, 1],
118+
} satisfies NoneGraphState;
119+
120+
// Act
121+
const {container} = render(
122+
<MafsGraph
123+
{...baseMafsGraphProps}
124+
state={graphStateWithRange}
125+
lockedFigures={[
126+
{...baseLockedFunctionProps, domain: [-20, -15]},
127+
]}
128+
/>,
129+
);
130+
131+
// eslint-disable-next-line testing-library/no-container, testing-library/no-node-access
132+
const lockedFunction = container.querySelector(".locked-function");
133+
134+
// Assert
135+
expect(lockedFunction).not.toBeInTheDocument();
136+
});
137+
});

‎packages/perseus/src/widgets/interactive-graphs/locked-figures/locked-function.tsx

Lines changed: 25 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -4,9 +4,14 @@ import {Plot} from "mafs";
44
import * as React from "react";
55
import {useState, useEffect} from "react";
66

7+
import useGraphConfig from "../reducer/use-graph-config";
8+
9+
import {clampDomain} from "./utils";
10+
711
import type {LockedFunctionType} from "@khanacademy/perseus-core";
812

913
const LockedFunction = (props: LockedFunctionType) => {
14+
const {range} = useGraphConfig();
1015
type Equation = {
1116
[k: string]: any;
1217
eval: (number) => number;
@@ -19,7 +24,6 @@ const LockedFunction = (props: LockedFunctionType) => {
1924
const plotProps = {
2025
color: lockedFigureColors[color],
2126
style: strokeStyle,
22-
domain,
2327
};
2428

2529
const hasAria = !!props.ariaLabel;
@@ -34,6 +38,16 @@ const LockedFunction = (props: LockedFunctionType) => {
3438
return null;
3539
}
3640

41+
const clampedDomain =
42+
directionalAxis === "x"
43+
? clampDomain(domain, range[0])
44+
: clampDomain(domain, range[1]);
45+
46+
// The domain entirely is outside the bounds of the graph. Don't render.
47+
if (clampedDomain === null) {
48+
return null;
49+
}
50+
3751
return (
3852
<g
3953
className="locked-function"
@@ -42,10 +56,18 @@ const LockedFunction = (props: LockedFunctionType) => {
4256
role="img"
4357
>
4458
{directionalAxis === "x" && (
45-
<Plot.OfX y={(x) => equation.eval({x})} {...plotProps} />
59+
<Plot.OfX
60+
y={(x) => equation.eval({x})}
61+
domain={clampedDomain}
62+
{...plotProps}
63+
/>
4664
)}
4765
{directionalAxis === "y" && (
48-
<Plot.OfY x={(y) => equation.eval({y})} {...plotProps} />
66+
<Plot.OfY
67+
x={(y) => equation.eval({y})}
68+
domain={clampedDomain}
69+
{...plotProps}
70+
/>
4971
)}
5072
</g>
5173
);

‎packages/perseus/src/widgets/interactive-graphs/locked-figures/locked-functions.stories.tsx

Lines changed: 15 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -2,7 +2,10 @@ import * as React from "react";
22

33
import {RendererWithDebugUI} from "../../../../../../testing/renderer-with-debug-ui";
44
import {ApiOptions} from "../../../perseus-api";
5-
import {segmentWithLockedFunction} from "../interactive-graph.testdata";
5+
import {
6+
segmentWithLockedFunction,
7+
segmentWithLockedFunctionAndAsymmetricRange,
8+
} from "../interactive-graph.testdata";
69

710
export default {
811
title: "Perseus/Widgets/Interactive Graph/Locked Functions",
@@ -38,6 +41,17 @@ export const FunctionOfY = (args: StoryArgs): React.ReactElement => (
3841
/>
3942
);
4043

44+
export const FunctionOfYAsymmetricRange = (
45+
args: StoryArgs,
46+
): React.ReactElement => (
47+
<RendererWithDebugUI
48+
{...defaultApiOptions}
49+
question={segmentWithLockedFunctionAndAsymmetricRange("y/2", {
50+
directionalAxis: "y",
51+
})}
52+
/>
53+
);
54+
4155
export const DomainRestrictedMin = (args: StoryArgs): React.ReactElement => (
4256
<RendererWithDebugUI
4357
{...defaultApiOptions}

‎packages/perseus/src/widgets/interactive-graphs/locked-figures/utils.test.ts

Lines changed: 29 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,29 @@
1+
import {clampDomain} from "./utils";
2+
3+
// TODO: Change/handle the last case here
4+
describe("clampDomain", () => {
5+
test.each`
6+
domain | graphXBounds | expected
7+
${[-Infinity, Infinity]} | ${[-10, 10]} | ${[-10, 10]}
8+
${[-20, Infinity]} | ${[-10, 10]} | ${[-10, 10]}
9+
${[-10, Infinity]} | ${[-10, 10]} | ${[-10, 10]}
10+
${[-5, Infinity]} | ${[-10, 10]} | ${[-5, 10]}
11+
${[-Infinity, 20]} | ${[-10, 10]} | ${[-10, 10]}
12+
${[-Infinity, 10]} | ${[-10, 10]} | ${[-10, 10]}
13+
${[-Infinity, 5]} | ${[-10, 10]} | ${[-10, 5]}
14+
${[-Infinity, -5]} | ${[-10, 10]} | ${[-10, -5]}
15+
${[-Infinity, -10]} | ${[-10, 10]} | ${[-10, -10]}
16+
${[10, -10]} | ${[-10, 10]} | ${[-10, 10]}
17+
${[-10, -20]} | ${[-10, 10]} | ${[-10, 10]}
18+
${[10, 10]} | ${[-10, 10]} | ${[10, 10]}
19+
${[-10, 10]} | ${[-10, 10]} | ${[-10, 10]}
20+
${[-Infinity, -9]} | ${[-10, 10]} | ${[-10, -9]}
21+
${[-Infinity, -11]} | ${[-10, 10]} | ${null}
22+
${[11, Infinity]} | ${[-10, 10]} | ${null}
23+
`(
24+
"clampedDomain($domain, $graphXBounds) = $expected",
25+
({domain, graphXBounds, expected}) => {
26+
expect(clampDomain(domain, graphXBounds)).toEqual(expected);
27+
},
28+
);
29+
});

‎packages/perseus/src/widgets/interactive-graphs/locked-figures/utils.ts

Lines changed: 24 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,24 @@
1+
export function clampDomain(
2+
domain: [number, number],
3+
graphBounds: [number, number],
4+
): [number, number] | null {
5+
// If the domain is invalid, return the graph bounds
6+
if (domain[0] > domain[1]) {
7+
return graphBounds;
8+
}
9+
10+
// If the domain is outside the graph bounds, return null
11+
if (
12+
(domain[0] < graphBounds[0] && domain[1] < graphBounds[0]) ||
13+
(domain[0] > graphBounds[1] && domain[1] > graphBounds[1])
14+
) {
15+
return null;
16+
}
17+
18+
// Clamp the function to the bounds of the graph to prevent memory
19+
// leaks when the domain is set to something like [-Infinity, Infinity].
20+
const min = Math.max(domain[0], graphBounds[0]);
21+
const max = Math.min(domain[1], graphBounds[1]);
22+
23+
return [min, max];
24+
}

0 commit comments

Comments
 (0)