[Interactive Graph] Add logarithm graph state management and reducer#3422
[Interactive Graph] Add logarithm graph state management and reducer#3422ivyolamit wants to merge 5 commits intoLEMS-3953/pr2-logarithm-kmathfrom
Conversation
…ithm graph state management and reducer for supporting Logarithm graph in Interactive Graph
…garithm graph state management and reducer
🗄️ Schema Change: No Changes ✅ |
🛠️ Item Splitting: No Changes ✅ |
npm Snapshot: PublishedGood news!! We've packaged up the latest commit from this PR (54a615a) and published it to npm. You Example: pnpm add @khanacademy/perseus@PR3422If you are working in Khan Academy's frontend, you can run the below command. ./dev/tools/bump_perseus_version.ts -t PR3422If you are working in Khan Academy's webapp, you can run the below command. ./dev/tools/bump_perseus_version.js -t PR3422 |
|
Size Change: +403 B (+0.08%) Total Size: 495 kB
ℹ️ View Unchanged
|
|
@claude review |
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
packages/perseus/src/widgets/interactive-graphs/reducer/interactive-graph-reducer.ts
Show resolved
Hide resolved
There was a problem hiding this comment.
Claude Code Review
This repository is configured for manual code reviews. Comment @claude review to trigger a review and subscribe this PR to future pushes, or @claude review once for a one-time review.
Tip: disable this comment in your organization's Code Review settings.
SonicScrewdriver
left a comment
SonicScrewdriver
left a comment
There was a problem hiding this comment.
This looks pretty much perfect to me, aside from the one issue regarding the asymptote bug (same one as the Exponential graph). I think it makes sense to fix it as part of this PR before releasing.
Feel free to grab me if you need help recreating
| describe("movePoint on a logarithm graph", () => { | ||
| it("rejects the move when both points would share the same y-coordinate", () => { | ||
| // Arrange — point 0 at y=-3, point 1 at y=-7; trying to move point 0 to y=-7 | ||
| const state = generateLogarithmGraphState(); |
There was a problem hiding this comment.
I really like these comments, they're honestly very helpful for understanding the tests. :) I might go back and add some clarity for the exponential ones after this
| if (newX === coords[0][X] || newX === coords[1][X]) { | ||
| return state; | ||
| } | ||
|
|
There was a problem hiding this comment.
You have the same bug that I have in the Exponential graph, where the asymptote can be dragged to the graph edge (invalid location) if a point is already at the left or right edge of the graph.
We need to check if the location is within the bounds of the graph here in order to determine if the movement is allowed.
There was a problem hiding this comment.
ooopppsss, i applied the fix in the wrong PR 🙈
https://github.com/Khan/perseus/pull/3423/changes#diff-ed3550c09301a3a9903bb56053e300ccd8339fac2822f32f26e69fc64ce56119R623
let me actually resolve that, move and fix that here.
There was a problem hiding this comment.
oh it's a different one. Ok going to fix that.
There was a problem hiding this comment.
This is now fixed in the latest commit.
ivyolamit
force-pushed
the
LEMS-3953/pr3-logarithm-state-management
branch
from
66a406e to
ce9f761
Compare
packages/perseus/src/widgets/interactive-graphs/reducer/interactive-graph-reducer.ts
Show resolved
Hide resolved
packages/perseus/src/widgets/interactive-graphs/reducer/interactive-graph-reducer.ts
Show resolved
Hide resolved
packages/perseus/src/widgets/interactive-graphs/reducer/interactive-graph-reducer.test.ts
Outdated
Show resolved
Hide resolved
| function getLogarithmCoords( | ||
| graph: PerseusGraphTypeLogarithm, | ||
| range: [x: Interval, y: Interval], | ||
| step: [x: number, y: number], |
There was a problem hiding this comment.
🟡 The getLogarithmCoords function in initialize-graph-state.ts is missing the export keyword, unlike its counterpart getExponentialCoords (line 490) which is exported. The design doc (logarithm.md:307) explicitly lists exporting getLogarithmCoords as a PR 3 deliverable, and PR 6 (editor support) will need to import it exactly the way the editor currently imports getExponentialCoords.
Extended reasoning...
What the bug is
getLogarithmCoords is declared as function getLogarithmCoords (no export keyword) at line 521 of initialize-graph-state.ts. Its counterpart getExponentialCoords is declared export function getExponentialCoords at line 490 of the same file. The PR description explicitly states it follows the exponential pattern throughout, and the design doc at packages/perseus/src/widgets/interactive-graphs/__docs__/notes/logarithm.md line 307 lists packages/perseus/src/index.ts — Export getLogarithmCoords as a PR 3 task.
The code path that will fail
getExponentialCoords is re-exported from packages/perseus/src/index.ts (line 113) and consumed by packages/perseus-editor/src/widgets/interactive-graph-editor/start-coords/util.ts (line 6) to compute default start coordinates in the editor UI. PR 6 (editor support for logarithm) will add logarithm handling to that same util.ts, importing getLogarithmCoords the same way. Without the export keyword here and a corresponding re-export in index.ts, that import will fail to compile.
Why existing code doesn't prevent it
The function is only called internally within initializeGraphState in the same file, so the missing export produces no error today. TypeScript module boundaries are only enforced at import sites, and no import site exists yet for this function.
Addressing the refutation
The refutation argues this is not a bug because the function works for its current purpose and the export can be added in PR 6. This is a reasonable position for a strict "does this PR break anything" review, but the PR description explicitly claims to follow the exponential pattern and the design doc explicitly lists the export as a PR 3 deliverable. Omitting it means this PR does not fully deliver on its stated scope, and the gap will need to be caught before PR 6 merges — exactly the kind of thing a PR review should flag.
Impact
No runtime or compilation failure in this PR or any currently merged code. The impact is forward-compatibility: PR 6 will fail to compile when it attempts to import getLogarithmCoords from @khanacademy/perseus.
How to fix it
- Change
function getLogarithmCoordstoexport function getLogarithmCoordsat line 521 ofinitialize-graph-state.ts. - Add
getLogarithmCoordsto the re-exports inpackages/perseus/src/index.tsalongsidegetExponentialCoords(line 113).
There was a problem hiding this comment.
I'm not too fussed if it's exported during this PR or the later PR, myself.
| // Final safety: asymptote must not land exactly on either point | ||
| if (newX === coords[0][X] || newX === coords[1][X]) { | ||
| return state; | ||
| } |
There was a problem hiding this comment.
🟡 The final safety check at lines 894-897 (if (newX === coords[0][X] || newX === coords[1][X]) return state) is logically unreachable dead code introduced by this PR. Both code paths that reach it guarantee strict inequality between newX and each coord's X value, so this guard can never fire. The identical pattern exists pre-existing in the exponential case at lines 849-852.
Extended reasoning...
What the bug is
The logarithm doMoveCenter function (the case "logarithm": block added entirely by this PR) contains a final safety check at lines 894-897:
// Final safety: asymptote must not land exactly on either point
if (newX === coords[0][X] || newX === coords[1][X]) {
return state;
}This check is unreachable on every code path through the function.
Path 1 — snap-through not needed (allRight || allLeft is true at lines 867-868)
The snap-through block is skipped entirely. If allRight, both coords[i][X] > newX (strict), so newX cannot equal either coord X by definition. The same applies for allLeft with reversed strict inequality. The final check at lines 894-897 therefore can never fire on this path.
Path 2 — snap-through entered (lines 870-892)
After the inset-clamped newX is computed, the code checks stillAllRight (line 886-887, strict >) and stillAllLeft (line 888, strict <). If \!stillAllRight && \!stillAllLeft, the function returns state at line 889-891 — before reaching line 894. If we pass that gate, at least one of stillAllRight or stillAllLeft is true. Either way strict inequality holds between newX and every coord X value, so the final check cannot fire here either.
Why existing code does not prevent the confusion
The guard was presumably copied from the original structure with the intent of being a defensive backstop. But the earlier logic already exhaustively handles every case that could produce an exact match: the snap-through block's own rejection gate (if (\!stillAllRight && \!stillAllLeft)) fires first whenever the clamped value would be on or between the points.
Impact
There is no runtime impact — the check is logically a no-op on every execution path. The harm is purely documentary: the comment "Final safety: asymptote must not land exactly on either point" implies there is a code path the earlier logic does not handle, which misleads future maintainers. The identical dead code exists pre-existing in the exponential case (lines 849-852), but that code was not introduced by this PR.
Step-by-step proof (Path 1 cannot produce an exact match)
- State: range=[-10,10], snapStep=[1,1], coords=[[-4,-3],[-5,-7]], asymptote=-6.
- User drags asymptote to x=-8;
boundAndSnapToGridreturns newX=-8. allRight: -4>-8 && -5>-8 → true. Snap-through block is skipped.- Final check: -8 === -4? No. -8 === -5? No. Guard passes vacuously — correct behavior, but the guard is not needed.
- For the guard to trip on Path 1, we would need newX to equal a coord X value while allRight or allLeft is true — impossible because allRight means both coords strictly exceed newX, and allLeft means both coords are strictly less.
How to fix it
Remove lines 894-897 (the final safety check and its comment). Behavior is identical without it. Alternatively, replace with a comment explaining the invariant is already enforced by the earlier logic.
There was a problem hiding this comment.
I vaguely recall chatting with Claude and finding one really obscure edge case that suggested keeping this, but I can't recall what it was. TBH, I'm fine with either cleaning this up or keeping it.
|
|
||
| // Both points at the same x is invalid for an exponential | ||
| if (updatedCoords[0][X] === updatedCoords[1][X]) { | ||
| return state; | ||
| } | ||
|
|
||
| return { | ||
| ...state, | ||
| hasBeenInteractedWith: true, | ||
| coords: updatedCoords, | ||
| }; |
There was a problem hiding this comment.
🟣 This is a pre-existing issue in the exponential doMovePoint cross-asymptote reflection block: after calling boundAndSnapToGrid on the reflected point, there is no check that updatedCoords[otherIndex][Y] === asymptoteY. If otherPoint[Y] is non-grid-aligned (possible via startCoords, which are stored verbatim) and within snapStep/2 of the asymptote, the reflected Y snaps to exactly asymptoteY, committing an invalid state with a curve point on the horizontal asymptote. The logarithm case added in this PR correctly includes the guard if (updatedCoords[otherIndex][X] === asymptoteX) return state; the fix is to add the symmetric if (updatedCoords[otherIndex][Y] === asymptoteY) return state to the exponential block.
Extended reasoning...
What the bug is and how it manifests
In the exponential doMovePoint cross-asymptote reflection branch (interactive-graph-reducer.ts, lines 581–601), after calling boundAndSnapToGrid([otherPoint[X], reflectedY], state) to produce updatedCoords[otherIndex], there is no guard checking whether updatedCoords[otherIndex][Y] === asymptoteY. If snapping maps reflectedY to exactly the asymptote Y-value, the returned state has a curve point sitting directly on the horizontal asymptote — an invariant violation. The coefficient computation for an exponential curve depends on both points being strictly above or below the asymptote, so placing a point on it produces degenerate or undefined coefficients and silently blanks the rendered curve.
The specific code path that triggers it
getExponentialCoords stores startCoords.coords verbatim without snapping, so a question author can supply a point at a non-grid-aligned Y value. When the user drags the other point across the horizontal asymptote, the cross-asymptote branch fires and computes reflectedY = 2 * asymptoteY - otherPoint[Y]. If otherPoint[Y] is non-grid-aligned and within snapStep/2 of asymptoteY, then Math.round(reflectedY / snapStep) * snapStep resolves to exactly asymptoteY.
Step-by-step proof
- Setup:
asymptoteY = 0,snapStep = [1, 1], coords =[[1, 2], [2, 0.4]](the second point placed via non-grid-alignedstartCoords). - User drags point 0 below the asymptote to
[1, -1]. boundDestination[Y] = -1≠ 0 → on-asymptote guard passes.newCoords[0][X] = 1≠ 2 → same-X guard passes.movedSide = (-1 > 0) = false;otherSide = (0.4 > 0) = true→ sides differ, cross-asymptote branch entered.reflectedY = 2 * 0 - 0.4 = -0.4.boundAndSnapToGrid([2, -0.4], state)→Math.round(-0.4 / 1) * 1 = 0→ reflected point lands at(2, 0)= the asymptote Y.- The only post-reflection guard checks
updatedCoords[0][X] === updatedCoords[1][X](1 ≠ 2) → passes. - State is committed with
coords[1][Y] === asymptoteY— invalid.
Why existing code doesn't prevent it
The pre-reflection same-X guard (newCoords[0][X] === newCoords[1][X]) and the on-asymptote guard (boundDestination[Y] === asymptoteY) both check the dragged point, not the reflected other point. The post-reflection guard added by this PR checks only for same-X between the two curve points, not whether either point lands on the asymptote. None of these guards can catch the reflected-point-onto-asymptote scenario.
Why this is pre-existing and what the PR does
The exponential reflection logic — including the return { ...state, coords: updatedCoords } inside the cross-asymptote block — predated this PR (the return statement has no + prefix in the diff). This PR added the same-X guard and the comment // Mirrors the logarithm reducer, but did not add the analogous asymptote-Y guard. The logarithm case introduced in this very PR does correctly include if (updatedCoords[otherIndex][X] === asymptoteX) return state, demonstrating the author was aware of the pattern and applied it to the new code but left the pre-existing exponential gap unaddressed.
How to fix it
After computing updatedCoords[otherIndex] in the exponential cross-asymptote block, add:
if (updatedCoords[otherIndex][Y] === asymptoteY) {
return state;
}This is directly symmetric with the logarithm guard at line 660 and closes the gap.
There was a problem hiding this comment.
I can make a separate PR for adding this logic, as it doesn't make sense to change exponential logic unless you absolutely have to.
SonicScrewdriver
left a comment
SonicScrewdriver
left a comment
There was a problem hiding this comment.
Happy to approve this now! Claude had a couple comments, but I don't consider them blockers.
3 participants
Summary:
PR series to add logarithm graph support to the Interactive Graph widget:
Add logarithm graph state management and reducer for supporting Logarithm graph in Interactive Graph
LogarithmGraphStateto the internal state type systemmovePoint+moveCenter) with logarithm-specific constraintsDetails
This PR adds the state management layer for logarithm graphs, following the exponential pattern throughout. Logarithm is the vertical-asymptote mirror of exponential's horizontal-asymptote design.
Action registration: Reuses existing
movePointandmoveCenteraction creators (no new action types). Theactionsexport object getslogarithm: { movePoint, moveCenter }, identical to exponential.Reducer —
doMovePoint:reflectedX = 2 * asymptoteX - otherX) so both points end up on the same side — matches Grapher widget behaviorReducer —
doMoveCenter:Initialization:
getLogarithmCoords()followsgetExponentialCoords()pattern — returns{coords, asymptote}. Default coords use normalized fractions[0.55, 0.55]and[0.75, 0.75]to ensure both points are to the right of the default asymptote at x=0 after normalization (x=0.5 would land exactly on the asymptote).Placeholders:
mafs-graph.tsxreturns{graph: null, interactiveElementsDescription: null}for logarithm (replaced in PR 4).mafs-state-to-interactive-graph.tshas the real serialization (not a placeholder).Co-Authored by Claude Code (Opus)
Issue: LEMS-3953
Test plan:
pnpm tscpassespnpm knippassespnpm lintpassespnpm prettier . --checkpassesmovePoint: same-y rejection, bounding-to-same-y rejection, valid move, on-asymptote rejection, cross-asymptote reflectionmoveCenter: valid move, snap-through between points, Y-component ignored, final safety rejection