[AbsoluteValue] PR2 - state management#3349
[AbsoluteValue] PR2 - state management#3349handeyeco wants to merge 4 commits intoLEMS-3347/absolute-value-pr1from
Conversation
🗄️ Schema Change: No Changes ✅ |
handeyeco
changed the title
github-actions
bot
added
the
schema-change
npm Snapshot: PublishedGood news!! We've packaged up the latest commit from this PR (89adf57) and published it to npm. You Example: pnpm add @khanacademy/perseus@PR3349If you are working in Khan Academy's frontend, you can run the below command. ./dev/tools/bump_perseus_version.ts -t PR3349If you are working in Khan Academy's webapp, you can run the below command. ./dev/tools/bump_perseus_version.js -t PR3349 |
github-actions
bot
removed
schema-change
🛠️ Item Splitting: No Changes ✅ |
|
Size Change: +45 B (+0.01%) Total Size: 486 kB
ℹ️ View Unchanged
|
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
There was a problem hiding this comment.
LGTM — straightforward, pattern-following implementation of the absolute-value graph type across existing switch statements and builders.
Extended reasoning...
Overview
This PR fills in the absolute-value graph type implementation across 7 files in the interactive-graphs widget. Every change follows the exact same pattern as existing graph types (sinusoid, quadratic, angle, etc.): replacing throw new Error("Not implemented") stubs with actual case handlers that mirror sibling graph types. The new getAbsoluteValueCoords function, AbsoluteValueGraphConfig builder class, doMovePoint reducer case, getGradableGraph handler, and mafsStateToInteractiveGraph case all follow established conventions precisely.
Security risks
None. The changes are entirely internal graph state management — no user input parsing, no network calls, no auth code, no data exposure vectors.
Level of scrutiny
Low scrutiny is appropriate. This is mechanical, pattern-following code in a widget subsystem. Each new code block is structurally identical to existing cases for other graph types (e.g., the sinusoid case). The feature is also behind a feature flag (per PR #3344). The only filed bug is a nit about a missing package-level re-export, which is explicitly deferred to a later PR in this multi-PR series and has no functional impact (the graph initializes correctly via fallback defaults).
Other factors
The PR is part of a planned multi-PR series (LEMS-3347). PR 1 set up the scaffolding and editor support; this PR 2 implements the core graph reducer/state logic. Test data is included. The PR description is sparse but the code changes are self-explanatory given the established patterns.
| export function getAbsoluteValueCoords( | ||
| graph: PerseusGraphTypeAbsoluteValue, | ||
| range: [x: Interval, y: Interval], | ||
| step: [x: number, y: number], | ||
| ): [Coord, Coord] { | ||
| if (graph.coords) { | ||
| return graph.coords; | ||
| } | ||
|
|
||
| if (graph.startCoords) { | ||
| return graph.startCoords; | ||
| } | ||
|
|
||
| const defaultCoords: [Coord, Coord] = [ | ||
| [0.5, 0.5], | ||
| [0.75, 0.75], | ||
| ]; | ||
|
|
||
| return normalizePoints(range, step, defaultCoords, true); | ||
| } |
There was a problem hiding this comment.
🟡 Nit: getAbsoluteValueCoords is exported from initialize-graph-state.ts but not re-exported from packages/perseus/src/index.ts, unlike every other coord initialization function (getCircleCoords, getLineCoords, etc.). This means the editor's getDefaultGraphStartCoords in util.ts has no absolute-value case and returns undefined. Since this is clearly a multi-PR effort and the editor integration was set up in PR 1, this is likely planned for a later PR — but worth tracking to ensure the export and editor case are added before the feature ships.
Extended reasoning...
What the bug is
getAbsoluteValueCoords is a new function introduced in this PR, exported from initialize-graph-state.ts. However, it is NOT re-exported from packages/perseus/src/index.ts (line 111-121), where all 9 other coord initialization functions (getCircleCoords, getLineCoords, getLinearSystemCoords, getPointCoords, getPolygonCoords, getSegmentCoords, getSinusoidCoords, getQuadraticCoords, getAngleCoords) are exported. This breaks the established pattern.
Downstream impact
The editor utility getDefaultGraphStartCoords in packages/perseus-editor/src/widgets/interactive-graph-editor/start-coords/util.ts computes default start coordinates for each graph type by calling the corresponding coord function. Without the getAbsoluteValueCoords export, there is no "absolute-value" case in the switch statement, so it falls through to default: return undefined (line 91-92). Meanwhile, shouldShowStartCoordsUI at line 195 returns true for absolute-value, so the start coords editor UI IS shown.
Step-by-step proof
- User opens the editor for an absolute-value graph type
shouldShowStartCoordsUIreturnstrue(line 195 of util.ts, added in PR 1)- The start coords UI is displayed with a "Use default start coordinates" button
- User clicks the button, which calls
getDefaultGraphStartCoords - The switch statement has no
"absolute-value"case, falls through todefault: return undefined undefinedis set asstartCoordson the graph config- During graph initialization,
getAbsoluteValueCoordsreceivesgraph.startCoords === undefined, skips the startCoords branch, and computes defaults from scratch - The graph still initializes correctly — the bug is silent
Addressing refutations
Multiple verifiers noted this is intentionally deferred work in a multi-PR series. I agree. The git history confirms: shouldShowStartCoordsUI for absolute-value was added in PR 1 (commit aae4026), not this PR. This PR does not modify util.ts at all. The practical impact is negligible since undefined startCoords triggers the correct default fallback in getAbsoluteValueCoords. The feature is also behind a feature flag (PR #3344).
Why this is a nit, not a bug
This is a pattern inconsistency in an incremental implementation, not a functional bug. The graph initializes correctly either way. However, the export should be added to the package index and the editor case should be completed before the feature flag is removed, to ensure the "Use default start coordinates" button returns explicit computed defaults rather than relying on the accidental undefined fallback.
…-pr1' into LEMS-3347/absolute-value-pr2
…for AbsoluteValue
1 participant
Everything is AI
Summary:
This is part of a series of PRs as defined in the first PR:
Initial implementation experiment is Perseus#3304.
Issue: LEMS-3347
Implements the full state pipeline for the absolute-value graph:
initialization, reducer transitions, serialization, and test
infrastructure. Depends on PR 1. No rendering yet — the renderGraphElements
stub from PR 1 remains in place.
Changes:
initializeGraphState case. Adds getAbsoluteValueCoords(graph, range, step):
returns graph.coords if set, then graph.startCoords, then normalized
defaults [[0.5, 0.5], [0.75, 0.75]] (same pattern as sinusoid/quadratic).
Note: currently unexported — PR 5 exports it for editor use.
reusing the shared movePoint action factory (same approach as sinusoid).
doMovePoint that snaps/bounds the destination, then rejects the move if the
two points would share the same x-coordinate (slope would be undefined).
serializes AbsoluteValueGraphState back to PerseusGraphTypeAbsoluteValue
for scoring.
case that maps state coords back to the graph type for live interaction
(e.g. equation display).
builder method and AbsoluteValueGraphConfig class so test fixtures can
construct absolute-value questions.