[Interactive Graph] Add tangent graph state management and reducer#3353
[Interactive Graph] Add tangent graph state management and reducer#3353ivyolamit wants to merge 3 commits intoLEMS-3955/tangent-kmathfrom
Conversation
…ph state management and reducer for supporting Tangent graph in Interactive Graph
…graph state management and reducer
🗄️ Schema Change: No Changes ✅ |
🛠️ Item Splitting: No Changes ✅ |
|
Summary of the tangent implementation
|
|
Size Change: +63 B (+0.01%) Total Size: 486 kB
ℹ️ View Unchanged
|
npm Snapshot: PublishedGood news!! We've packaged up the latest commit from this PR (fa61e49) and published it to npm. You Example: pnpm add @khanacademy/perseus@PR3353If you are working in Khan Academy's frontend, you can run the below command. ./dev/tools/bump_perseus_version.ts -t PR3353If you are working in Khan Academy's webapp, you can run the below command. ./dev/tools/bump_perseus_version.js -t PR3353 |
There was a problem hiding this comment.
LGTM — mechanical extension of the sinusoid pattern to tangent across all state management layers, with good test coverage.
Extended reasoning...
Overview
This PR adds tangent graph state management as part 3 of a 6-PR series (LEMS-3955). It touches 15 files across the interactive graph widget: type definitions (types.ts), actions (interactive-graph-action.ts), reducer (interactive-graph-reducer.ts), initialization (initialize-graph-state.ts), gradable graph serialization (interactive-graph-state.ts), state-to-graph conversion (mafs-state-to-interactive-graph.ts), a placeholder rendering case (mafs-graph.tsx), test data builders (interactive-graph-question-builder.ts, interactive-graph.testdata.ts), and 5 test files with 13 new tests.
Security risks
None. This is UI graph widget code with no auth, network, or data-handling changes. The same-x guard prevents a division-by-zero in getTangentCoefficients, which is a correctness concern rather than a security one.
Level of scrutiny
Low scrutiny is appropriate. Every tangent addition is a direct copy of the existing sinusoid pattern — TangentGraphConfig mirrors SinusoidGraphConfig, the reducer tangent case mirrors the sinusoid case, getTangentCoords mirrors getSinusoidCoords, etc. The PR makes no novel architectural decisions; it extends an established, well-tested system.
Other factors
The one bug found (bounding-rejection test passing for the wrong reason) is a minor test quality nit — it does not affect production behavior, and the direct same-x test at line 280 properly covers the guard. The identical issue pre-exists in the sinusoid tests. The bundle size impact is negligible (+63 bytes). All CI checks (schema, item splitting, size) passed. No CODEOWNERS file appears to gate these paths.
packages/perseus/src/widgets/interactive-graphs/reducer/interactive-graph-reducer.test.ts
Outdated
Show resolved
Hide resolved
| "@khanacademy/kmath": minor | ||
| "@khanacademy/perseus": minor | ||
| "@khanacademy/perseus-core": minor | ||
| "@khanacademy/perseus-editor": minor |
There was a problem hiding this comment.
| "@khanacademy/kmath": minor | |
| "@khanacademy/perseus": minor | |
| "@khanacademy/perseus-core": minor | |
| "@khanacademy/perseus-editor": minor | |
| "@khanacademy/perseus": minor |
Only @khanacademy/perseus changes in this PR.
| expect(result.coords).toEqual([[5, 0]]); | ||
| }); | ||
|
|
||
| it("returns tangent graph coords when interacted with", () => { |
There was a problem hiding this comment.
This doesn't really seem like a super useful test, but, even if it is, its name is confusing: it makes it sound like this is some kind of UI test that's interacting with the graph but really it's just testing a getter function. Maybe it would be better:
- If the name was more specific to what we're trying to accomplish
- There was a test for when
hasBeenInteractedWithisfalse- is the behavior any different?
| ], | ||
| }; | ||
|
|
||
| const baseTangentGraphState: InteractiveGraphState = { |
There was a problem hiding this comment.
Seems like we should be using generateIGTangentGraph instead here. The other tests in these files should similarly be using their generator functions.
| }); | ||
|
|
||
| it("does not allow moving an endpoint of a sinusoid if the bounding logic would result in an invalid graph", () => { | ||
| it("rejects a sinusoid move when bounding clamps the point to the same x as the other point", () => { |
There was a problem hiding this comment.
Why did this test need to change?
| interactiveGraphQuestionBuilder().withSinusoid().build(); | ||
|
|
||
| export const tangentQuestion: PerseusRenderer = | ||
| interactiveGraphQuestionBuilder() |
There was a problem hiding this comment.
Through these PRs, this is the third way I've seen to generate tangent test data:
- baseTangentGraphState
- generateIGTangentGraph
- interactiveGraphQuestionBuilder
Feels like we need to land on one.
Summary:
PR series to add tangent graph support to the Interactive Graph widget:
This is the third PR in a series to add tangent graph support to the Interactive Graph widget (LEMS-3955). It wires up the state management layer — the reducer, initialization, actions, and gradable graph serialization.
Add tangent graph state management and reducer for supporting Tangent graph in Interactive Graph
TangentGraphStateand adds it to theInteractiveGraphStateuniontangent.movePointaction and reducer case with same-x constraint (prevents division by zero ingetTangentCoefficients)initializeGraphStateviagetTangentCoords()(default coords[[0.5, 0.5], [0.75, 0.75]])getGradableGraphfor scoring serializationwithTangent()builder method andTangentGraphConfigclass for test data constructiontangentQuestionandtangentQuestionWithDefaultCorrecttest datamafsStateToInteractiveGraphImplementation notes
InteractiveGraphState union update. Adding
TangentGraphStateto the union triggersUnreachableCaseErrorinrenderGraphElements(mafs-graph.tsx). A placeholder case returnsnull(no rendering) — replaced with real rendering in PR 4.Same-x constraint. The
doMovePointtangent case rejects moves that would place both control points on the same vertical line. This mirrors the sinusoid constraint and preventsgetTangentCoefficientsfrom producingInfinityforangularFrequency(since it divides byp2[0] - p1[0]).getTangentCoords()is not exported. It's only called withininitialize-graph-state.ts. It will be exported in PR 6 (editor) whenstart-coords/util.tsneeds it.mafsStateToInteractiveGraphtangent case is the real implementation (not a placeholder) — it simply returns{ ...originalGraph, coords: state.coords }, same as sinusoid.References
Co-Authored by Claude Code (Opus)
Issue: LEMS-3955
Test plan:
pnpm tsc— no type errorspnpm lint— no lint errorspnpm prettier . --check— formatting cleanpnpm knip— no unused exportsgetGradableGraphtangent test passesmafsStateToInteractiveGraphtangent serialization test passes