Conversation
🗄️ Schema Change: No Changes ✅ |
🛠️ Item Splitting: No Changes ✅ |
|
Size Change: +1.1 kB (+0.22%) Total Size: 497 kB
ℹ️ View Unchanged
|
npm Snapshot: PublishedGood news!! We've packaged up the latest commit from this PR (0f0d3ae) and published it to npm. You Example: pnpm add @khanacademy/perseus@PR3423If you are working in Khan Academy's frontend, you can run the below command. ./dev/tools/bump_perseus_version.ts -t PR3423If you are working in Khan Academy's webapp, you can run the below command. ./dev/tools/bump_perseus_version.js -t PR3423 |
|
@claude review |
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
packages/perseus/src/widgets/interactive-graphs/interactive-graph.tsx
Outdated
Show resolved
Hide resolved
packages/perseus/src/widgets/interactive-graphs/graphs/logarithm.tsx
Outdated
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.
packages/perseus/src/widgets/interactive-graphs/interactive-graph.tsx
Outdated
Show resolved
Hide resolved
ivyolamit
force-pushed
the
LEMS-3953/pr4-logarithm-graph-component
branch
from
76ec318 to
665cffd
Compare
There was a problem hiding this comment.
LGTM — all substantive bugs from prior review rounds are addressed in the current code.
Extended reasoning...
Overview
This PR adds the logarithm graph visual component to the Interactive Graph widget, following the pattern established by the exponential graph. It touches logarithm.tsx (new), exponential.tsx (refactored to share common constraint logic), utils.ts (new shared helpers), interactive-graph.tsx (equation string + default coords), strings.ts (6 new SR strings), mafs-graph.tsx (routing), and test/story files.
Security risks
No security-sensitive code paths touched. This is UI rendering and keyboard accessibility logic with no auth, data exposure, or injection vectors.
Level of scrutiny
This PR went through five rounds of iterative review. The initial bugs (malformed equation string for negative c, unreachable dead code, missing same-X keyboard guard, cross-asymptote reflection collision, unclamped-coord silent rejection) were all identified and the current code addresses every one of them — including the comprehensive clamped-coord checks in both getLogarithmKeyboardConstraint and getExponentialKeyboardConstraint. The remaining nit (defaultLogarithmCoords calling pointsFromNormalized without noSnap=true) is editor-facing, cosmetic, and reproduces a pre-existing pattern from defaultExponentialCoords.
Other factors
Test coverage is thorough (15 new logarithm tests, updated exponential tests for edge cases, new movable-asymptote blur test). The shared getAsymptoteGraphKeyboardConstraint and constrainAsymptoteKeyboardMovement helpers eliminate duplication between exponential and logarithm. The equation-string c === 0 and c < 0 fixes and the post-drag blur fix for MovableAsymptote are solid incremental improvements.
ivyolamit
force-pushed
the
LEMS-3953/pr4-logarithm-graph-component
branch
from
665cffd to
4574cb7
Compare
| onMove={(newPoint) => | ||
| dispatch(actions.logarithm.moveCenter(newPoint)) | ||
| } | ||
| constrainKeyboardMovement={(p) => | ||
| constrainAsymptoteKeyboard(p, coords, snapStep) | ||
| } |
There was a problem hiding this comment.
🔴 Pressing UP or DOWN while the vertical logarithm asymptote is focused dispatches a move event whose X-coordinate equals the current asymptote position, causing the reducer to set hasBeenInteractedWith: true without actually moving the asymptote. This allows a student to submit the default graph positions as if they had genuinely interacted, bypassing the unanswered-question guard. Fix: add if (newX === state.asymptote) { return state; } before the hasBeenInteractedWith assignment in the logarithm doMoveCenter case in interactive-graph-reducer.ts.
Extended reasoning...
What the bug is and how it manifests
The vertical logarithm asymptote passes point={asymptoteMid} = [asymptoteX, asymptoteMidY] to useDraggable, with constrainKeyboardMovement supplied as a function. When the user presses UP or DOWN while the asymptote is focused, useDraggable takes the function-form keyboard path: direction=[0,±1] (Y-only), span=ySpan. For each candidate dx, constrainAsymptoteKeyboardMovement with orientation='vertical' sets axis=0 (X) and constrains X only — returning [asymptoteX, snap(asymptoteMidY + dx)]. The resulting vec.dist([asymptoteX, snapped_Y], [asymptoteX, asymptoteMidY]) = |snapped_Y - asymptoteMidY| exceeds the minimum threshold (ySpan/(50*2) = 0.2) at dx ≈ 0.8, so onMove([asymptoteX, 1]) fires even though only the irrelevant Y component changed.
The specific code path that triggers it
In the reducer's logarithm doMoveCenter case (interactive-graph-reducer.ts ~lines 860–903), newX = boundAndSnapToGrid([asymptoteX, 1], state)[X] = asymptoteX — identical to state.asymptote. The reducer then checks allRight/allLeft (both pass because coords are still all on the same side), checks newX \!== coords[0][X] and newX \!== coords[1][X] (both pass), and returns {...state, hasBeenInteractedWith: true, asymptote: asymptoteX}. No guard exists for the case newX === state.asymptote.
Why existing code does not prevent it
The asymptote's keyboard movement constraint correctly restricts lateral movement using the X-axis logic, but useDraggable's function-form path measures displacement in the full 2D vector space — including the Y component, which is not semantically meaningful for asymptote positioning. The reducer trusts that any dispatched move represents a real change, and its existing guards only check for collision with curve points, not for no-op moves.
What the impact is
hasBeenInteractedWith is the flag that getGradableGraph inspects to decide whether the student's answer is gradeable or should be treated as unanswered. Once it is spuriously set to true, a student who accidentally presses UP or DOWN on the default graph can immediately hit "Check Answer" and receive grading feedback on the unmodified default positions — bypassing the intended "you haven't interacted yet" invalid-input rejection. This affects grading integrity for the new logarithm graph type introduced in this PR series.
Step-by-step proof
- Default state:
asymptote = -6,coords = [[-4,-3],[-5,-7]],range=[[-10,10],[-10,10]],snapStep=[1,1]. - Student focuses the asymptote and presses UP.
direction=[0,1],span=20. Atdx=0.8:snap([1,1], [-6, 0+0.8]) = [-6, 1];constrainAsymptoteKeyboardMovementwith orientation='vertical' returns[-6, 1]. vec.dist([-6,1], [-6,0]) = 1 > 0.2 = min.onMove([-6, 1])fires.- Reducer:
boundAndSnapToGrid([-6,1])[X] = -6 = state.asymptote. No no-op guard. Returns{...state, hasBeenInteractedWith: true, asymptote: -6}. - Asymptote is visually unchanged. Student clicks "Check Answer" — answer is graded against the default positions.
How to fix it
In interactive-graph-reducer.ts, inside the logarithm doMoveCenter case, add the following early return after computing newX and before the hasBeenInteractedWith assignment:
if (newX === state.asymptote) {
return state;
}This mirrors the analogous guard that the exponential reducer applies for its horizontal asymptote.
There was a problem hiding this comment.
This is a good thing to discuss, but I'm not sure I'd consider it the level of red/stopship that Claude seems to consider it.
IMO the user focusing on the asymptote and attempting to move it is enough interaction for me to consider that they have interacted with the graph. The spirit of the unanswered question guard is mostly to help kids who entirely miss an answerable widget in the question.
That being said, I'm also not against resolving this, as Claude suggests, if we think it's a big enough issue.
There was a problem hiding this comment.
This is now fixed in latest commit, in interactive-graph-reducer.ts
SonicScrewdriver
left a comment
SonicScrewdriver
left a comment
There was a problem hiding this comment.
I had a couple of small comments, but generally this looks good to me.
| ); | ||
| }); | ||
|
|
||
| it("blurs the element when a mouse drag ends to clear the focus ring", () => { |
There was a problem hiding this comment.
Love it! Thank you for fixing the issue
| return [snapped[X], newY]; | ||
| }; | ||
| ): vec.Vector2 => | ||
| constrainAsymptoteKeyboardMovement(p, coords, snapStep, "horizontal"); |
There was a problem hiding this comment.
Love it, thank you again! This is a huge improvement
| onMove={(newPoint) => | ||
| dispatch(actions.logarithm.moveCenter(newPoint)) | ||
| } | ||
| constrainKeyboardMovement={(p) => | ||
| constrainAsymptoteKeyboard(p, coords, snapStep) | ||
| } |
There was a problem hiding this comment.
This is a good thing to discuss, but I'm not sure I'd consider it the level of red/stopship that Claude seems to consider it.
IMO the user focusing on the asymptote and attempting to move it is enough interaction for me to consider that they have interacted with the graph. The spirit of the unanswered question guard is mostly to help kids who entirely miss an answerable widget in the question.
That being said, I'm also not against resolving this, as Claude suggests, if we think it's a big enough issue.
| return strings.srLogarithmInteractiveElements; | ||
| } | ||
|
|
||
| function describeLogarithmGraph( |
There was a problem hiding this comment.
Fully an ignorable nit: It might be nice to have a function description comment here, but perhaps this is straight forward enough to not require it.
| * X vs other point; logarithm checks X vs asymptote and Y vs other point), | ||
| * so they are injected via the callback. | ||
| */ | ||
| export function getAsymptoteGraphKeyboardConstraint( |
There was a problem hiding this comment.
Nit comment: These seems fine here, but I was wondering if it would make more sense to live within movable-asymptote since it directly pertains to that component. 🤔
There was a problem hiding this comment.
i'll leave this as is for now, i'm on the fence on adding util functions within the components make it look clutter (this has been the practice for most of our code) but having it in a util file make sense to me.
There was a problem hiding this comment.
Great! It might help to add a note to a follow up ticket so we don't forget. :)
| @@ -724,7 +725,7 @@ class InteractiveGraph extends React.Component<Props, State> { | |||
| [0.75, 0.75], | |||
| ]; | |||
| // @ts-expect-error - TS2345 - Argument of type 'number[][]' is not assignable to parameter of type 'readonly Coord[]'. | |||
There was a problem hiding this comment.
I'd love to get rid of this (and other) ts-expect-errors, but that seems out of scope for this PR. Just wanted to mention it for our backlog
…ema (#3420) ## Summary: PR series to add logarithm graph support to the Interactive Graph widget: 1.▶️ [Add logarithm graph type definitions and data](#3420) 2. [Add logarithm math utilities to kmath](#3421) 3. [Add logarithm graph state management and reducer](#3422) 4. [Add logarithm graph rendering, SR strings, and equation string](#3423) 5. [Add logarithm graph scoring](#3424) 6. [Add logarithm graph option in the Interactive Graph Editor](#3425) Add logarithm type definitions, this is the initial implementation for supporting Logarithm graph in Interactive Graph widget. - Add `PerseusGraphTypeLogarithm` and `LogarithmGraphCorrect` types to the data schema, following the exponential pattern (coords + asymptote) - Add JSON parser for the new `"logarithm"` graph type - Add `generateIGLogarithmGraph()` test data generator - Add placeholder cases in all exhaustiveness switches so the build stays green ## Details This is the first PR in the logarithm interactive graph series (LEMS-3950). It adds the type definitions with zero runtime behavior change — no graph renders, no scoring, no editor UI. **Data shape:** Logarithm follows the exponential pattern — two curve points plus an asymptote value. For logarithm the asymptote is the x-value of a vertical line (vs. exponential's y-value for a horizontal line). **Placeholder cases** were added to satisfy `UnreachableCaseError` exhaustiveness in: - `interactive-graph-editor.tsx` — graph merging switch - `start-coords/util.ts` — `shouldShowStartCoordsUI` (returns `false`), `getDefaultGraphStartCoords` (returns `undefined`) - `interactive-graph.tsx` — `getEquationString` (returns `""` — safe no-op since this is called unconditionally in the editor render path) - `initialize-graph-state.ts` — `initializeGraphState` (returns `type: "none"`) - `interactive-graph-ai-utils.ts` — `getGraphOptionsForProps` and `getUserInput` These placeholders will be replaced with real implementations in subsequent PRs. **Why `getEquationString` returns `""` instead of throwing:** `getEquationString` is called unconditionally in the editor's `render()` method (`interactive-graph-editor.tsx`). Since this PR adds the parser that accepts `type: "logarithm"`, any content with `correct.type === "logarithm"` (e.g. created via API) would reach the editor and crash the React render if the placeholder threw. All other placeholders in this PR are safe (return no-op values), but `getEquationString` is uniquely on the render path, so it must return a safe value. Future graph types should follow the same pattern: return `""` here, not throw. Issue: LEMS-3953 Co-Authored by Claude Code (Opus) ## Test plan - [ ] `pnpm tsc` passes - [ ] `pnpm knip` passes - [ ] `pnpm lint` passes - [ ] `pnpm prettier . --check` passes - [ ] Generator tests pass (2 new tests: default graph, graph with all props) - [ ] Parser regression tests pass (170 snapshots unchanged) - [ ] Interactive graph tests pass (199 tests) - [ ] Verify no runtime behavior change in Storybook (existing graph types still work) Author: ivyolamit Reviewers: claude[bot], ivyolamit, SonicScrewdriver, handeyeco Required Reviewers: Approved By: SonicScrewdriver Checks: ⏭️ 1 check has been skipped, ✅ 10 checks were successful Pull Request URL: #3420
## Summary: PR series to add logarithm graph support to the Interactive Graph widget: 1. [Add logarithm graph type definitions and data](#3420) 2.▶️ [Add logarithm math utilities to kmath](#3421) 3. [Add logarithm graph state management and reducer](#3422) 4. [Add logarithm graph rendering, SR strings, and equation string](#3423) 5. [Add logarithm graph scoring](#3424) 6. [Add logarithm graph option in the Interactive Graph Editor](#3425) Add the logarithm math utilities to kmath for supporting Logarithm graph in Interactive Graph - Add `LogarithmCoefficient` type and `getLogarithmCoefficients()` to kmath, following the exponential pattern - Compute coefficients `{a, b, c}` for `f(x) = a·ln(b·x + c)` using the inverse exponential approach - No canonical normalization needed (logarithm has no periodic equivalences, same as exponential) ## Details This adds the shared math utility for logarithm coefficient computation to `@khanacademy/kmath`, following the same pattern as `getExponentialCoefficients()`. Both the rendering component (PR 4) and scoring (PR 5) will consume this function. **Mathematical approach (inverse exponential):** 1. Flip each coordinate `(x, y) → (y, x)` — treating the logarithm as the inverse of an exponential 2. Use the asymptote x-value as the flipped exponential's `c` coefficient 4. Compute exponential coefficients `aExp`, `bExp` from the flipped points 5. Invert to get logarithm coefficients: `a = 1/bExp`, `b = 1/aExp`, `c = -cExp/aExp` **Validation guards** (returns `undefined` for invalid inputs): - Same y-coordinate on both points (makes `bExp` undefined) - A point lying on the asymptote - Points on opposite sides of the asymptote - Non-finite or zero intermediate results This matches the reference implementation in `packages/perseus-core/src/utils/grapher-util.ts` (the Grapher widget's `Logarithm` object, lines 449–558). Co-Authored by Claude Code (Opus) Issue: LEMS-3953 ## Test plan - [ ] `pnpm tsc` passes - [ ] `pnpm knip` passes - [ ] `pnpm lint` passes - [ ] `pnpm prettier . --check` passes - [ ] Coefficient tests pass (6 new tests, 17 total in coefficients.test.ts) - Grapher test data: `[-4,-3]`, `[-5,-7]`, asymptote `-6` reproduces correct y-values - Natural log: `[1,0]`, `[e,1]`, asymptote `0` → `a≈1, b≈1, c≈0` - Negative b: points left of asymptote (`y = ln(-x)`) → `b≈-1` - Same-y → `undefined` - Point on asymptote → `undefined` - Opposite sides → `undefined` Author: ivyolamit Reviewers: claude[bot], SonicScrewdriver, handeyeco Required Reviewers: Approved By: SonicScrewdriver Checks: ⏭️ 1 check has been skipped, ✅ 10 checks were successful, ⚪️ 1 check is neutral Pull Request URL: #3421
ivyolamit
force-pushed
the
LEMS-3953/pr3-logarithm-state-management
branch
from
54a615a to
ff4c2f6
Compare
…3422) ## Summary: PR series to add logarithm graph support to the Interactive Graph widget: 1. [Add logarithm graph type definitions and data](#3420) 2. [Add logarithm math utilities to kmath](#3421) 3.▶️ [Add logarithm graph state management and reducer](#3422) 4. [Add logarithm graph rendering, SR strings, and equation string](#3423) 5. [Add logarithm graph scoring](#3424) 6. [Add logarithm graph option in the Interactive Graph Editor](#3425) Add logarithm graph state management and reducer for supporting Logarithm graph in Interactive Graph - Add `LogarithmGraphState` to the internal state type system - Wire up reducer actions (`movePoint` + `moveCenter`) with logarithm-specific constraints - Add graph state initialization with sensible defaults - Add test data fixtures and question builder support ## Details 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 `movePoint` and `moveCenter` action creators (no new action types). The `actions` export object gets `logarithm: { movePoint, moveCenter }`, identical to exponential. **Reducer — `doMovePoint`:** - Point cannot land on the asymptote's x-coordinate - Both points must have different y-values (prevents degenerate coefficient computation) - Cross-asymptote reflection: when a point is dragged past the asymptote, the other point is reflected (`reflectedX = 2 * asymptoteX - otherX`) so both points end up on the same side — matches Grapher widget behavior **Reducer — `doMoveCenter`:** - Asymptote moves horizontally only (X component extracted, Y ignored) - Snap-through logic: when the new position would land between or on the curve points, snaps past all points using the midpoint heuristic for direction detection (prevents oscillation/flicker) - Final safety check: asymptote cannot land exactly on either point's x-coordinate **Initialization:** `getLogarithmCoords()` follows `getExponentialCoords()` 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.tsx` returns `{graph: null, interactiveElementsDescription: null}` for logarithm (replaced in PR 4). `mafs-state-to-interactive-graph.ts` has the real serialization (not a placeholder). Co-Authored by Claude Code (Opus) Issue: LEMS-3953 ## Test plan: - [ ] `pnpm tsc` passes - [ ] `pnpm knip` passes - [ ] `pnpm lint` passes - [ ] `pnpm prettier . --check` passes - [ ] Reducer tests pass (147 total, 14 new logarithm tests): - `movePoint`: same-y rejection, bounding-to-same-y rejection, valid move, on-asymptote rejection, cross-asymptote reflection - `moveCenter`: valid move, snap-through between points, Y-component ignored, final safety rejection - Initialization: given coords, startCoords, defaults - Gradable graph: logarithm state conversion - Serialization: logarithm state to interactive graph - [ ] Interactive graph tests pass (206 total, logarithm added to parameterized test maps) - [ ] Serialization tests pass (14 total) Author: ivyolamit Reviewers: claude[bot], ivyolamit, SonicScrewdriver Required Reviewers: Approved By: SonicScrewdriver Checks: ⏭️ 1 check has been skipped, ✅ 10 checks were successful, ⚪️ 1 check is neutral Pull Request URL: #3422
ivyolamit
changed the base branch from
LEMS-3953/pr3-logarithm-state-management
to
main
… logarithm graph visual component, add Storybook coverage, SR strings, and equation string for supporting Logarithm graph in Interactive Graph
…arithm graph rendering, SR strings, and equation string
…rdConstraint for common logic in logarithm and exponential
…mon logic in logarithm and exponential
… issue when dragged
ivyolamit
force-pushed
the
LEMS-3953/pr4-logarithm-graph-component
branch
from
4574cb7 to
dd49d3a
Compare
## Summary: PR series to add logarithm graph support to the Interactive Graph widget: 1. [Add logarithm graph type definitions and data](#3420) 2. [Add logarithm math utilities to kmath](#3421) 3. [Add logarithm graph state management and reducer](#3422) 4. [Add logarithm graph rendering, SR strings, and equation string](#3423) 5.▶️ [Add logarithm graph scoring](#3424) 6. [Add logarithm graph option in the Interactive Graph Editor](#3425) Add logarithm graph scoring to support the Logarithm graph in Interactive Graph - Add logarithm scoring to `score-interactive-graph.ts` using direct coefficient comparison - No canonical normalization needed (same as exponential) ## Details Follows the exponential scoring pattern exactly. Computes `{a, b, c}` coefficients for both the user's answer and the rubric using `getLogarithmCoefficients` from kmath, then compares with `approximateDeepEqual`. **Scoring logic:** - Returns `invalid` if guess coords or asymptote are missing, or if coefficient computation fails for either side - Returns `earned: 1` if `[a, b, c]` coefficients approximately match - Returns `earned: 0` otherwise This means two different sets of control points that define the same logarithmic curve will score as correct — the comparison is on the mathematical function, not the specific points chosen. Co-Authored by Claude Code (Opus) Issue: LEMS-3953 ## Test plan: - [ ] `pnpm tsc` passes - [ ] `pnpm knip` passes - [ ] `pnpm lint` passes - [ ] `pnpm prettier . --check` passes - [ ] Scoring tests pass (44 total, 6 new logarithm tests): - [ ] Invalid: undefined guess, null coords, null asymptote - [ ] Correct: matching coefficients - [ ] Incorrect: different coefficients - [ ] Equivalent curves: different control points producing same `y = ln(x)` coefficients → correct Author: ivyolamit Reviewers: claude[bot], SonicScrewdriver Required Reviewers: Approved By: SonicScrewdriver Checks: ⏭️ 1 check has been skipped, ✅ 10 checks were successful Pull Request URL: #3424
There was a problem hiding this comment.
All previously raised bugs have been addressed — LGTM.
Extended reasoning...
Overview
This PR adds the logarithm graph visual component (logarithm.tsx), 6 screen reader strings, an equation string, and Storybook coverage, completing the rendering layer of a multi-PR logarithm graph series. It also extracts shared keyboard-constraint logic (getAsymptoteGraphKeyboardConstraint, constrainAsymptoteKeyboardMovement) into utils.ts and applies comprehensive fixes to the exponential graph keyboard constraints as well.
Security risks
No security-sensitive code is touched. The changes are entirely within the interactive-graph widget (client-side rendering, SR strings, equation formatting). No auth, crypto, or data-access paths are involved.
Level of scrutiny
This PR received multiple rounds of iterative review. All five substantive bugs identified across those rounds were addressed by the author: negative-constant notation in the equation string, dead code in the asymptote keyboard constraint, missing same-X guard for keyboard navigation, cross-asymptote reflection collision, and clamped-boundary collision. The one remaining item flagged in this run is a low-severity nit (retry-exhaustion path setting hasBeenInteractedWith without visible movement) that mirrors pre-existing exponential behavior and was explicitly accepted by the team as intentional — a user pressing a key while focused on an interactive element counts as meaningful interaction. The human reviewer concurs.
Other factors
Test coverage is thorough: 15 new logarithm-specific tests plus additional edge-case tests (clamped-boundary collisions) added to the exponential suite during review. The implementation consistently follows the exponential graph pattern, which eases long-term maintenance. The PR is self-contained within a PR series and does not touch scoring or editor logic.
| if (!isValidPosition(movedCoord)) { | ||
| return coordToBeMoved; |
There was a problem hiding this comment.
🟡 When all keyboard-constraint retry attempts are exhausted, getAsymptoteGraphKeyboardConstraint (utils.ts line 416) returns coordToBeMoved — the point's current position — which causes useDraggable's object-form branch to call onMove(currentPosition); the logarithm reducer then sets hasBeenInteractedWith: true with no coordinate change, allowing a student to bypass the unanswered-question guard by pressing an arrow key that cannot move the point. The identical pre-existing behavior exists in the exponential graph; the team has noted that attempting to interact with an element is itself meaningful interaction, so this is a low-severity consistency issue rather than a blocking defect.
Extended reasoning...
What the bug is and how it manifests
getAsymptoteGraphKeyboardConstraint (utils.ts lines 408-419) tries one initial position plus up to 3 retries. If all four attempts fail isValidPosition, it returns coordToBeMoved — the point's current position. The returned value is stored as, say, constrain.up in the object-form KeyboardMovementConstraint. In useDraggable (use-draggable.ts line 103-107), the object-form path reads:
const destination = constrainKeyboardMovement[directionForKey[event.key]];
onMove(destination ?? point);
Because destination is coordToBeMoved (the current position) rather than undefined, the ?? point fallback never fires and onMove(currentPosition) is called unconditionally. The logarithm doMovePoint reducer (interactive-graph-reducer.ts lines 679-687) then processes this as a valid move: boundAndSnapToGrid(currentPosition) === currentPosition; all guards (asymptote, same-X, same-Y) pass because the current position is already valid; the result is {...state, hasBeenInteractedWith: true, coords: unchanged}.
A concrete trigger
Range [-10,10], snapStep [1,1], otherPoint at Y=9, currentPoint at Y=8. Pressing UP:
- Attempt Y=9: coord[Y] === otherPoint[Y] blocked
- Retry Y=10: clamped to Y=9 === otherPoint[Y] blocked (all further retries clamp identically)
All 4 fail -> constraint returns currentPoint. onMove(currentPoint) fires. Reducer sets hasBeenInteractedWith:true with no coordinate change.
Why existing code does not prevent it
The function-form useDraggable path guards against no-op moves using vec.dist(testPoint, point) > min, but the object-form path used by asymptote-based point constraints has no such distance check. The reducer has no identity guard comparing boundDestination to the current state coords.
Addressing the refutation
The main refutation correctly observes: (1) the team explicitly stated that focusing on an element and attempting to move it is sufficient interaction (SonicScrewdriver comment on the asymptote UP/DOWN issue), and (2) the identical behavior already exists in the exponential reducer. Both points are valid. The team's stated intent is that good-faith user engagement — pressing a key while an interactive element has focus — qualifies as interaction even when no visible movement results. The logarithm graph follows the exponential pattern exactly, so this is a pre-existing systemic pattern rather than a new regression introduced by this PR.
The fix (returning null/undefined from the constraint when all retries fail, or adding a no-op position guard in the reducer) would be cleaner and eliminate the asymmetry with the function-form path, but is a cross-cutting change that should be filed as a separate cleanup task consistent with how the exponential graph was shipped.
Impact
A student in an edge-case boundary configuration who presses an arrow key with no visible effect will have hasBeenInteractedWith set to true, enabling submission of the default positions. Because the trigger requires a specific geometry (point near the snap-inset boundary with the other point at the exact clamped value), and submitting in this state still produces a wrong answer that scores zero, the practical harm is minimal.
…ph Editor (#3425) ## Summary: PR series to add logarithm graph support to the Interactive Graph widget: 1. [Add logarithm graph type definitions and data](#3420) 2. [Add logarithm math utilities to kmath](#3421) 3. [Add logarithm graph state management and reducer](#3422) 4. [Add logarithm graph rendering, SR strings, and equation string](#3423) 5. [Add logarithm graph scoring](#3424) 6.▶️ [Add logarithm graph option in the Interactive Graph Editor](#3425) Add logarithm graph option in the Interactive Graph Editor - Add `StartCoordsLogarithm` component for configuring logarithm start coordinates in the editor - Gate the "Logarithm function" option behind the `interactive-graph-logarithm` feature flag - Export `getLogarithmCoords` for use by the editor - Add start asymptote validation ## Details This PR enables content creators to configure logarithm exercises in the Interactive Graph editor, following the exponential editor pattern. **StartCoordsLogarithm component:** - Two coordinate pair inputs (Point 1 and Point 2) - A single number input for the asymptote x-position (labeled "Asymptote x =") - Equation display showing `y = a * ln(b*x + c)` with computed coefficient values - CSS module styling (not Aphrodite), following `start-coords-exponential.module.css` **Feature flag:** "Logarithm function" appears in the graph type selector only when `interactive-graph-logarithm` is enabled, preventing content creators from selecting it until it's ready for launch. **Editor validation:** The start asymptote x-value is validated — it cannot fall between or on the x-coordinates of the curve's start points (mirrors exponential's y-axis validation but for x-axis). **Exports:** `getLogarithmCoords()` is now exported from `initialize-graph-state.ts` and re-exported from `@khanacademy/perseus` for use by the editor's start-coords UI. Co-Authored by Claude Code (Opus) Issue: LEMS-3969 ## Test plan: - [ ] `pnpm tsc` passes - [ ] `pnpm knip` passes - [ ] `pnpm lint` passes - [ ] `pnpm prettier . --check` passes - [ ] Editor tests pass (422 total across 15 suites) - [ ] Verify in Storybook: "Logarithm function" appears in graph type selector when feature flag is on - [ ] Verify start coords UI renders correctly with asymptote input and equation display Author: ivyolamit Reviewers: claude[bot], SonicScrewdriver, ivyolamit Required Reviewers: Approved By: SonicScrewdriver Checks: ⏭️ 1 check has been skipped, ✅ 10 checks were successful Pull Request URL: #3425
3 participants
Summary:
PR series to add logarithm graph support to the Interactive Graph widget:
Create the logarithm graph visual component, add Storybook coverage, SR strings, and equation string for supporting Logarithm graph in Interactive Graph
logarithm.tsx) with curve rendering, draggable asymptote, and movable pointsDetails
This is the largest PR in the series. It creates the visual rendering of the logarithm graph, following the exponential component pattern with the axis swapped (vertical asymptote instead of horizontal).
Curve rendering:
<Plot.OfX>with domain restricted to one side of the asymptote ([asymptoteX + 0.001, xMax]or[xMin, asymptoteX - 0.001])a * ln(b*x + c), returning NaN when outside domain or when y-value exceeds visible range + padding (prevents curve visually touching the asymptote)coeffRefcaches last valid coefficients as fallback during transient invalid statesAsymptote rendering:
MovableAsymptotecomponent withorientation="vertical"actions.logarithm.moveCenter()on dragconstrainAsymptoteKeyboard()implements snap-through logic for keyboard navigation (mirrors exponential's vertical version but on X-axis)Keyboard constraints:
getLogarithmKeyboardConstraint()prevents points from landing on the asymptote's x-coordinate or sharing y-value with the other pointEquation string:
getLogarithmEquationString()displaysy = a*ln(b*x + c)with computed coefficient valuesdefaultLogarithmCoords()provides fallback coords (normalized fractions[0.55, 0.55],[0.75, 0.75])Screen reader strings (6):
srLogarithmGraph— graph container labelsrLogarithmPoint1/srLogarithmPoint2— point position labelssrLogarithmAsymptote— asymptote label with keyboard instructionssrLogarithmDescription— graph state descriptionsrLogarithmInteractiveElements— interactive elements summaryCo-Authored by Claude Code (Opus)
Issue: LEMS-3953
Test plan:
pnpm tscpassespnpm knippassespnpm lintpassespnpm prettier . --checkpasses