[AbsoluteValue] PR5 - editor support#3352
[AbsoluteValue] PR5 - editor support#3352handeyeco wants to merge 4 commits intoLEMS-3347/absolute-value-pr4from
Conversation
🗄️ Schema Change: No Changes ✅ |
npm Snapshot: PublishedGood news!! We've packaged up the latest commit from this PR (23ab60e) and published it to npm. You Example: pnpm add @khanacademy/perseus@PR3352If you are working in Khan Academy's frontend, you can run the below command. ./dev/tools/bump_perseus_version.ts -t PR3352If you are working in Khan Academy's webapp, you can run the below command. ./dev/tools/bump_perseus_version.js -t PR3352 |
🛠️ Item Splitting: No Changes ✅ |
|
Size Change: +68 B (+0.01%) Total Size: 488 kB
ℹ️ View Unchanged
|
| case "absolute-value": | ||
| const absoluteValueCoords = getAbsoluteValueCoords( | ||
| props, | ||
| range, | ||
| step, | ||
| ); | ||
| return ( | ||
| <StartCoordsPoint | ||
| startCoords={absoluteValueCoords} | ||
| onChange={onChange} | ||
| /> | ||
| ); |
There was a problem hiding this comment.
🟡 The absolute-value case uses StartCoordsPoint (which expects Coord[]) instead of StartCoordsLine (which expects CollinearTuple / [Coord, Coord]). Since getAbsoluteValueCoords returns [Coord, Coord], using StartCoordsLine would be more type-safe and consistent with how linear/ray handle the same structure.
Extended reasoning...
What the bug is
The new absolute-value case in StartCoordsSettingsInner (line 46-57) renders StartCoordsPoint to handle start coordinate editing. However, getAbsoluteValueCoords returns [Coord, Coord] (a fixed-length tuple), which is the same shape as CollinearTuple used by linear and ray graph types. StartCoordsPoint expects Coord[] (variable-length array) and is designed for point/polygon graphs.
How the type widening happens
StartCoordsPoint declares its props as { startCoords: Coord[]; onChange: (startCoords: Coord[]) => void }. Its onChange handler uses const newStartCoords = [...startCoords] (spread into a new array), then calls onChange(newStartCoords). This spread widens the TypeScript type from [Coord, Coord] to Coord[]. In contrast, StartCoordsLine declares { startCoords: CollinearTuple; onChange: (startCoords: CollinearTuple) => void } and its onChange constructs the tuple explicitly: onChange([value, startCoords[1]]), preserving the [Coord, Coord] type.
Step-by-step proof
- User opens the interactive graph editor and selects "Absolute value" as the graph type.
StartCoordsSettingsInnermatchescase "absolute-value"(line 46).getAbsoluteValueCoordsis called, returning a[Coord, Coord]value.- This is passed to
<StartCoordsPoint startCoords={absoluteValueCoords} onChange={onChange} />. - User edits Point 1 —
StartCoordsPoint.onChangefires with[...startCoords](line 31 of start-coords-point.tsx), producingCoord[]. - The
onChangeprop propagates thisCoord[]value upward, where the absolute-value graph expects[Coord, Coord].
For comparison, linear and ray (which have the identical [Coord, Coord] type) correctly use StartCoordsLine (lines 62-68), and sinusoid (also [Coord, Coord]) uses StartCoordsSinusoid.
Impact
This is not a runtime bug — at JavaScript runtime, a 2-element array is indistinguishable whether typed as [Coord, Coord] or Coord[], and the UI renders identically (both show "Point 1" / "Point 2" inputs). The onChange handler at line 170 is untyped, so the widening passes silently. However, it is a type-safety and consistency concern: downstream TypeScript code expecting [Coord, Coord] could receive Coord[], and this breaks the established pattern in the codebase.
How to fix
Replace StartCoordsPoint with StartCoordsLine in the absolute-value case, matching the pattern used by linear/ray:
case "absolute-value":
const absoluteValueCoords = getAbsoluteValueCoords(props, range, step);
return (
<StartCoordsLine
startCoords={absoluteValueCoords}
onChange={onChange}
/>
);…-pr4' into LEMS-3347/absolute-value-pr5
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
Add editor support for absolute-value graph type
Surfaces the absolute-value graph in the content creator UI. Depends on PR
1 + PR 2 (for getAbsoluteValueCoords). No new components needed — the
two-point shape fits existing patterns.
Changes:
file-private) so the editor can import it via @khanacademy/perseus.
other getXxxCoords helpers.
GraphTypesThatHaveStartCoords so the StartCoords type includes its
startCoords field.
getAbsoluteValueCoords + StartCoordsPoint (the two-point shape maps
directly onto the existing generic point coordinate UI).
getDefaultGraphStartCoords so the "Use default start coordinates" reset
button returns the correct defaults.