[Interactive Graph] Add tangent graph option in the Interactive Graph Editor#3358
[Interactive Graph] Add tangent graph option in the Interactive Graph Editor#3358ivyolamit wants to merge 4 commits intoLEMS-3955/tangent-scoringfrom
Conversation
…Add tangent graph option in the Interactive Graph Editor
…h] Add tangent graph option in the Interactive Graph Editor
🗄️ Schema Change: No Changes ✅ |
|
Size Change: +223 B (+0.05%) Total Size: 488 kB
ℹ️ View Unchanged
|
🛠️ Item Splitting: No Changes ✅ |
npm Snapshot: PublishedGood news!! We've packaged up the latest commit from this PR (b4fb7eb) and published it to npm. You Example: pnpm add @khanacademy/perseus@PR3358If you are working in Khan Academy's frontend, you can run the below command. ./dev/tools/bump_perseus_version.ts -t PR3358If you are working in Khan Academy's webapp, you can run the below command. ./dev/tools/bump_perseus_version.js -t PR3358 |
|
Summary of the tangent implementation
|
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
There was a problem hiding this comment.
LGTM — straightforward addition of tangent graph type to the editor, closely mirroring the existing sinusoid pattern and gated behind a feature flag.
Extended reasoning...
Overview
This PR adds tangent as a selectable graph type in the Interactive Graph Editor. It touches 8 files: a new StartCoordsTangent component, a getTangentEquation utility, feature-flag-gated dropdown option in GraphTypeSelector, apiOptions prop threading, export of getTangentCoords from perseus, and updates to shouldShowStartCoordsUI and getDefaultGraphStartCoords to handle the tangent type.
Security risks
None. All changes are editor-only UI code behind a feature flag (interactive-graph-tangent). No auth, crypto, permissions, or user-facing data handling is involved.
Level of scrutiny
Low scrutiny is appropriate. This is the sixth and final PR in a well-structured series. Every change follows the established sinusoid pattern exactly — the new component, equation function, and coord handling are mechanical mirrors of their sinusoid counterparts. The feature flag gating follows the same pattern as other gated features in the codebase.
Other factors
The two bugs found are both minor: (1) a nit about tangent missing from the GraphTypesThatHaveStartCoords type union, which has no runtime or compile-time impact since tangent's startCoords type (Coord[]) is structurally identical to sinusoid's already-included type; (2) a pre-existing formatting issue in equation string building that exists identically in getSinusoidEquation and is explicitly accepted by existing tests. Neither warrants blocking the PR. The CODEOWNERS file only has general team ownership for this path, with no specific individual owners for the interactive graph editor files.
...s/perseus-editor/src/widgets/interactive-graph-editor/start-coords/start-coords-settings.tsx
Show resolved
Hide resolved
| }; | ||
|
|
||
| export const getTangentEquation = (startCoords: [Coord, Coord]) => { | ||
| // Get coefficients | ||
| // It's assumed that p1 is the inflection point and p2 is a quarter-period away | ||
| const p1 = startCoords[0]; | ||
| const p2 = startCoords[1]; | ||
|
|
||
| // Resulting coefficients are canonical for this tangent curve | ||
| const amplitude = p2[1] - p1[1]; | ||
| const angularFrequency = Math.PI / (4 * (p2[0] - p1[0])); | ||
| const phase = p1[0] * angularFrequency; | ||
| const verticalOffset = p1[1]; | ||
|
|
||
| return ( | ||
| "y = " + | ||
| amplitude.toFixed(3) + | ||
| "tan(" + | ||
| angularFrequency.toFixed(3) + | ||
| "x - " + | ||
| phase.toFixed(3) + | ||
| ") + " + | ||
| verticalOffset.toFixed(3) | ||
| ); | ||
| }; | ||
|
|
||
| export const getQuadraticEquation = (startCoords: [Coord, Coord, Coord]) => { | ||
| const p1 = startCoords[0]; | ||
| const p2 = startCoords[1]; |
There was a problem hiding this comment.
🟣 Pre-existing issue: getTangentEquation inherits two formatting quirks from getSinusoidEquation — (1) hardcoded "x - " and ") + " produce ugly strings like "x - -0.785" and "+ -1.000" when phase/offset are negative, and (2) no division-by-zero guard when both points share the same x-coordinate, producing "Infinity" in the equation. Both issues exist identically in getSinusoidEquation (lines 105-123) and are explicitly verified by existing sinusoid tests (line 269). Since this PR intentionally mirrors the sinusoid pattern, this is a pre-existing issue — but worth noting for a future cleanup across both functions.
Extended reasoning...
What the bug is
getTangentEquation (lines 125-153) builds an equation string using hardcoded operators "x - " and ") + ". When phase is negative, the output becomes "x - -0.785" instead of the mathematically cleaner "x + 0.785". Similarly, when verticalOffset is negative, it produces "+ -1.000" instead of "- 1.000". Additionally, if p2[0] === p1[0], the denominator in Math.PI / (4 * (p2[0] - p1[0])) is zero, producing Infinity for angularFrequency and phase, yielding an equation like "y = 0.000tan(Infinityx - Infinity) + 0.000".
Why this is pre-existing
Both issues are direct copies of the identical pattern in getSinusoidEquation (lines 105-123 of the same file). The sinusoid function uses Math.PI / (2 * (p2[0] - p1[0])) with the same division-by-zero vulnerability, and the same hardcoded "x - " and ") + " operators. The PR description explicitly states "StartCoordsTangent mirrors StartCoordsSinusoid exactly," confirming this was an intentional design choice to maintain consistency.
Proof via concrete example
For the formatting issue: given startCoords = [[1, -1], [3, 1]], we get amplitude = 2, angularFrequency = π/8 ≈ 0.393, phase = 0.393, verticalOffset = -1. The output is "y = 2.000tan(0.393x - 0.393) + -1.000" — note the "+ -1.000". For the division-by-zero: given startCoords = [[2, 0], [2, 3]], the denominator 4 * (2 - 2) = 0, so angularFrequency = Infinity, and the equation displays nonsensically.
Existing test evidence
The sinusoid tests in util.test.ts (around line 269) explicitly assert the ugly format: "y = 1.000sin(0.785x - -0.785) + 0.000" and "y = 2.000sin(1.571x - 0.000) + -1.000". This demonstrates the team has accepted this formatting as the current behavior. Changing only getTangentEquation would create an inconsistency between tangent and sinusoid equation displays.
Impact and recommended fix
The impact is purely cosmetic — these equations are only shown in the editor's start coordinates section, not to students. The division-by-zero case requires a content creator to manually type identical x-coordinates into the CoordinatePairInput fields, which is an unusual edge case (interactive dragging prevents it). For comparison, getQuadraticEquation (line 156-159) already has a denom === 0 guard that returns "Division by zero error". A future cleanup could add sign-aware formatting and division-by-zero guards to both getSinusoidEquation and getTangentEquation together for consistency.
| ); | ||
| }; | ||
|
|
||
| const styles = StyleSheet.create({ |
There was a problem hiding this comment.
Move this in packages/perseus-editor/src/widgets/interactive-graph-editor/start-coords/start-coords-tangent.css
…stead of aphrodite
1 participant
Summary:
PR series to add tangent graph support to the Interactive Graph widget:
This is the sixth and final PR in a series to add tangent graph support to the Interactive Graph widget (LEMS-3956). It adds editor support so content creators can create tangent exercises.
Add tangent graph option in the Interactive Graph Editor
interactive-graph-tangentfeature flagStartCoordsTangentcomponent for configuring tangent start coordinates (inflection point + quarter-period point)getTangentCoords()from perseus for use by the editor's start-coords utilitiesImplementation notes
Feature flag gating. The tangent
OptionIteminGraphTypeSelectoronly renders whenisFeatureOn("interactive-graph-tangent")is true. This is the only place the feature flag is checked — once a tangent graph type is persisted in content JSON, it renders and scores regardless of the flag. This follows the pattern used by other gated features (e.g.,image-widget-upgrade-scale).apiOptionsprop threading.GraphTypeSelectorneeded access toapiOptionsforisFeatureOn. AddedapiOptionsas an optional prop and threaded it fromInteractiveGraphEditor.getTangentCoords()export. This function was internal toinitialize-graph-state.ts(as noted in PR 3's plan). Now exported and re-exported from perseus index, used bygetDefaultGraphStartCoordsandStartCoordsSettingsInnerin the editor.StartCoordsTangentmirrorsStartCoordsSinusoidexactly. Two coordinate pair inputs (Point 1 / Point 2) and an equation display usinggetTangentEquation(). The equation helper follows the same pattern asgetSinusoidEquation().shouldShowStartCoordsUIflipped. Changed fromfalse(set in PR 1 as a placeholder) totruefor tangent, now thatStartCoordsSettingsInnerandgetDefaultGraphStartCoordsboth handle the tangent type.References
Co-Authored by Claude Code (Opus)
Issue: LEMS-3956
Test plan:
pnpm tsc— no type errorspnpm lint— no lint errorspnpm prettier . --check— formatting cleanpnpm knip— no unused exports