[AbsoluteValue] update guide into multiple PRs#3346
Conversation
handeyeco
changed the title
github-actions
bot
added
schema-change
🗄️ Schema Change: No Changes ✅ |
🛠️ Item Splitting: No Changes ✅ |
npm Snapshot: PublishedGood news!! We've packaged up the latest commit from this PR (dd931e5) and published it to npm. You Example: pnpm add @khanacademy/perseus@PR3346If you are working in Khan Academy's frontend, you can run the below command. ./dev/tools/bump_perseus_version.ts -t PR3346If you are working in Khan Academy's webapp, you can run the below command. ./dev/tools/bump_perseus_version.js -t PR3346 |
|
Size Change: 0 B 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 — documentation-only change reorganizing a planning markdown file into a PR breakdown structure.
Extended reasoning...
Overview
This PR modifies a single file: packages/perseus/src/widgets/interactive-graphs/__docs__/notes/absolute-value.md. The change reorganizes an existing implementation planning document from a flat file/checklist structure into a 5-PR stacked breakdown. No code, configuration, tests, or runtime behavior is affected.
Security risks
None. This is a markdown planning document in a __docs__/notes/ directory with zero runtime impact.
Level of scrutiny
Minimal scrutiny warranted. This is a documentation reorganization with no code changes. The three bug reports (all filed as nits) identify valid issues with the planning document's accuracy — incomplete file coverage in the PR breakdown, missing feature flag mention, and incorrect scoring pseudocode pattern — but these are guide-level concerns that will be caught during actual implementation when TypeScript compilation and tests enforce correctness. The inline comments already convey this feedback to the author.
Other factors
The PR description is sparse (summary placeholder not filled in, no test plan), which is consistent with a docs-only change. The content being reorganized was already reviewed and merged in PR #3334. The restructuring into PRs is a reasonable organizational improvement for the implementation work ahead.
| The implementation is split into five stacked PRs. Each PR can pass all checks independently (types, lint, tests). They are intended to land together. | ||
|
|
||
| - `packages/perseus/src/widgets/interactive-graphs/graphs/absolute-value.tsx` — Main component | ||
| ### PR 1 — Type definitions & schema | ||
|
|
||
| Pure additive type changes. No runtime behavior. All existing tests continue to pass unmodified. |
There was a problem hiding this comment.
🟡 The PR breakdown claims "Each PR can pass all checks independently (types, lint, tests)" but this is false. PR 1 adds to the PerseusGraphType and InteractiveGraphState unions, which would break exhaustive UnreachableCaseError switches in 4+ files only handled in later PRs. Additionally, several files are missing from all 5 PRs entirely (interactive-graph-ai-utils.ts, start-coords/util.ts, index.ts), the AbsoluteValueGraphCorrect type for PerseusGraphCorrectType is omitted, and editor-package duplicates of the question builder and testdata are not listed.
Extended reasoning...
The core claim is false
The PR breakdown introduces the claim: "Each PR can pass all checks independently (types, lint, tests). They are intended to land together." This is provably incorrect for PR 1. PR 1 adds PerseusGraphTypeAbsoluteValue to the PerseusGraphType union and AbsoluteValueGraphState to the InteractiveGraphState union, but multiple files in the codebase have exhaustive switch statements on these unions with UnreachableCaseError (which takes a never parameter, enforcing compile-time exhaustiveness). The affected files — initialize-graph-state.ts:129, mafs-state-to-interactive-graph.ts:95, interactive-graph.tsx:603, and mafs-graph.tsx:772 — are only modified in PR 2 or PR 3. TypeScript would refuse to compile after PR 1 lands alone. The project's own new-graph-type.md guide (line 566) explicitly warns about this: "TypeScript will report a compile error if you add a type to the union without handling it in those switches."
Files completely missing from all 5 PRs
Two additional files with exhaustive UnreachableCaseError switches on PerseusGraphType are not listed in ANY of the 5 PRs:
interactive-graph-ai-utils.ts— has TWO exhaustive switches (lines 226 and 279)start-coords/util.ts— has one exhaustive switch (line 197)
Even after all 5 PRs land, these files would still fail to compile. This is distinct from the PR ordering issue above — those files at least appear in the plan. These are completely omitted.
Missing index.ts re-export
packages/perseus/src/index.ts (lines 115-121) re-exports getSinusoidCoords, getQuadraticCoords, etc. from initialize-graph-state.ts. The editor package imports these from @khanacademy/perseus. PR 5 plans to use getAbsoluteValueCoords in start-coords-settings.tsx, but index.ts is not listed in any PR. Without adding getAbsoluteValueCoords to this re-export, PR 5 cannot compile. (Note: one verifier flagged this as a duplicate of bug_010, but bug_010 is not part of this review — the issue stands on its own.)
Missing AbsoluteValueGraphCorrect type
PerseusGraphCorrectType (data-schema.ts:1087-1098) is a separate union from PerseusGraphType. It is used by PerseusInteractiveGraphRubric as the type of rubric.correct. Every existing graph type has a corresponding *GraphCorrect variant (e.g., SinusoidGraphCorrect, QuadraticGraphCorrect). PR 1's checklist only mentions adding to PerseusGraphType and never mentions PerseusGraphCorrectType. Without an AbsoluteValueGraphCorrect type, PR 4's scoring code (rubric.correct.type === "absolute-value") would be a TypeScript error because "absolute-value" is not in the discriminator.
Missing editor-package test infrastructure duplicates
The editor package has explicitly duplicated copies of interactive-graph-question-builder.ts and interactive-graph.testdata.ts (each file's line 1 says "Duplicated from packages/perseus/..."). These are used by interactive-graph-editor.stories.tsx for Storybook and editor tests. Neither duplicate is mentioned in any of the 5 PRs.
Concrete proof for PR 1 compilation failure
Step-by-step: (1) PR 1 adds PerseusGraphTypeAbsoluteValue to the PerseusGraphType union in data-schema.ts. (2) initialize-graph-state.ts imports PerseusGraphType and has a switch at line 129: default: throw new UnreachableCaseError(graph). (3) UnreachableCaseError constructor expects never. (4) After adding the new union member, graph in the default branch is no longer never — it's PerseusGraphTypeAbsoluteValue. (5) TypeScript emits: "Argument of type 'PerseusGraphTypeAbsoluteValue' is not assignable to parameter of type 'never'". (6) This file is only modified in PR 2, not PR 1. (7) Therefore PR 1 cannot pass type checks independently.
Suggested fix
Either: (a) add placeholder case "absolute-value": throw new Error("Not yet implemented") branches in all exhaustive switches in PR 1, or (b) restructure the PR breakdown so type union additions and their corresponding switch cases are in the same PR. Also add the missing files (interactive-graph-ai-utils.ts, start-coords/util.ts, index.ts, editor-package duplicates) and the AbsoluteValueGraphCorrect type to the appropriate PRs.
| |------|--------| | ||
| | `packages/perseus-editor/.../graph-type-selector.tsx` | Add `<OptionItem value="absolute-value" label="Absolute value" />` | | ||
| | `packages/perseus-editor/.../interactive-graph-editor.tsx` | Add editor support | | ||
| | `packages/perseus-editor/.../start-coords/start-coords-settings.tsx` | Add `case "absolute-value"` using `StartCoordsPoint` with `getAbsoluteValueCoords` | | ||
|
|
||
| **Checklist:** | ||
|
|
||
| - [ ] **`graph-type-selector.tsx`** — Add `<OptionItem value="absolute-value" label="Absolute value" />`. | ||
| - [ ] **`interactive-graph-editor.tsx`** — Add editor support for absolute value type. | ||
| - [ ] **`start-coords/start-coords-settings.tsx`** — Add `case "absolute-value"` using | ||
| `StartCoordsPoint` with `getAbsoluteValueCoords`. |
There was a problem hiding this comment.
🟡 The PR 5 checklist adds <OptionItem value="absolute-value" .../> to graph-type-selector.tsx unconditionally, but the feature flag "interactive-graph-absolute-value" was explicitly added in PR #3344 (feature-flags.ts:16) for controlled rollout. The plan should mention gating this option behind the feature flag using isFeatureOn, so the implementer doesn't ship the option to all content creators immediately.
Extended reasoning...
What the issue is
The preceding PR #3344 (commit 3289699) added the feature flag "interactive-graph-absolute-value" to packages/perseus-core/src/feature-flags.ts at line 16, along with flags for other phase-2 graph types (tangent, logarithm, exponent, vector). These flags all default to false in both production and test configurations, establishing a clear intent for controlled rollout.
However, the PR 5 section of this planning document instructs the implementer to add <OptionItem value="absolute-value" label="Absolute value" /> to graph-type-selector.tsx with no mention of feature flag gating. None of the five PR descriptions reference the feature flag at all.
Why this matters
The GraphTypeSelector component currently accepts only graphType and onChange props — it has no apiOptions prop and no isFeatureOn check. All existing graph types (linear, quadratic, sinusoid, circle, etc.) are listed unconditionally. To gate the new option behind the feature flag, the component would need to accept apiOptions and conditionally render the <OptionItem> using isFeatureOn("interactive-graph-absolute-value", apiOptions). This is a non-trivial interface change that the implementer should plan for.
Concrete walkthrough
If an implementer follows the PR 5 checklist literally: they add <OptionItem value="absolute-value" label="Absolute value" /> as a new line after the existing options in graph-type-selector.tsx. On merge, every content creator using the interactive graph editor would immediately see "Absolute value" in the dropdown, even though the feature flag defaults to false. The flag would have no effect because nothing checks it.
Severity assessment
This is filed as a nit because this PR only modifies a markdown planning document — no code is being shipped, and no runtime behavior is affected. The observation is valid, but the plan is a guide, not a specification. Additionally, none of the other similar planning documents (tangent.md, logarithm.md) mention feature flags either, so this is consistent with the established pattern for these docs. The implementer could address feature flag gating during actual implementation regardless of what this document says.
Suggested fix
Add a note to the PR 5 checklist: pass apiOptions to GraphTypeSelector and conditionally render the new <OptionItem> only when isFeatureOn("interactive-graph-absolute-value", apiOptions) returns true. This follows the pattern used by image-settings.tsx for the image widget feature flags.
| ### PR 4 — Scoring | ||
|
|
||
| ### Scoring | ||
| Isolated from rendering. Can be developed in parallel with PR 3 but must stack after PR 1. | ||
|
|
||
| **New files:** _(none)_ | ||
|
|
||
| **Modified files:** | ||
|
|
||
| | File | Change | | ||
| |------|--------| | ||
| | `packages/perseus-score/src/widgets/interactive-graph/score-interactive-graph.ts` | Add scoring branch | | ||
| | `packages/perseus-score/src/widgets/interactive-graph/score-interactive-graph.test.ts` | Add scoring unit tests | | ||
|
|
||
| **Checklist:** | ||
|
|
||
| - [ ] **`score-interactive-graph.ts`** — Add `else if` branch: | ||
| ```ts |
There was a problem hiding this comment.
🟡 The PR 4 scoring pseudocode (lines 298-305) has two issues: (1) it calls getAbsoluteValueCoefficients() which no PR creates in kmath/coefficients.ts (where the scoring package imports coefficient helpers from), making the dependency claim "must stack after PR 1" incomplete; and (2) it unconditionally returns for both correct and incorrect answers (earned: correct ? 1 : 0), breaking the fall-through pattern every other graph type uses — where only correct answers return early and incorrect ones fall through to shared logic (line 315) that returns type: "invalid" for unmodified submissions. Following this pseudocode literally would mark unmodified graphs as wrong instead of prompting the user to try.
Extended reasoning...
Issue 1: Missing getAbsoluteValueCoefficients in PR breakdown
The scoring pseudocode in PR 4 calls getAbsoluteValueCoefficients(userInput.coords) and getAbsoluteValueCoefficients(rubric.correct.coords). Looking at the existing codebase pattern, getSinusoidCoefficients and getQuadraticCoefficients both live in packages/kmath/src/coefficients.ts and are imported by the scoring package at score-interactive-graph.ts:23 via @khanacademy/kmath. The document's own Decision #6 recommends adding getAbsoluteValueCoefficients to kmath/coefficients.ts for scoring reuse.
However, no PR in the breakdown includes modifications to packages/kmath/. PR 4 claims it "must stack after PR 1" and can parallel PR 3, but PR 1 only touches type definitions and schema — it does not create the coefficient helper. PR 3 defines a getAbsoluteValueCoefficients in absolute-value.tsx (in packages/perseus), but the scoring package (packages/perseus-score) cannot import from packages/perseus due to cross-package import restrictions. This is an internal inconsistency in the planning document that would block PR 4 implementation until resolved.
Issue 2: Scoring pseudocode breaks the fall-through pattern
The pseudocode returns immediately for both correct and incorrect answers:
const correct = approximateDeepEqual(userCoeffs, rubricCoeffs);
return { type: "points", earned: correct ? 1 : 0, total: 1, message: null };This breaks the pattern used by every existing graph type. Looking at score-interactive-graph.ts, the quadratic branch (line 112), sinusoid branch (line 134), and circle branch (line 151) all only return when the answer is correct — they return {type: "points", earned: 1, total: 1} and otherwise fall through. The fall-through is critical because lines 313-327 contain shared logic:
if (!hasValue || _.isEqual(userInput, rubric.graph)) {
return { type: "invalid", message: null };
}
return { type: "points", earned: 0, total: 1, message: null };This distinguishes between an unmodified submission (type: "invalid" = user hasn't answered yet, prompt to try) and an actual wrong answer (earned: 0 = incorrect).
Step-by-step proof of the bug:
- User opens an absolute-value graph exercise but does not move any points
- User clicks "Check answer" with the graph still at its initial/default position
- With the proposed pseudocode:
getAbsoluteValueCoefficients(userInput.coords)computes coefficients from the default position, which won't match the rubric ->correct = false-> immediately returns{type: "points", earned: 0}-> user sees "Wrong answer" - With the correct pattern (early return only on correct): the function falls through to line 315 ->
_.isEqual(userInput, rubric.graph)returnstrue(coords unchanged) -> returns{type: "invalid"}-> user sees "Please try" prompt instead
Fix: Change the pseudocode to match the existing pattern — only return on correct, and let incorrect answers fall through:
if (approximateDeepEqual(userCoeffs, rubricCoeffs)) {
return { type: "points", earned: 1, total: 1, message: null };
}Also, add a PR (or expand PR 1/PR 2) to include adding getAbsoluteValueCoefficients to packages/kmath/src/coefficients.ts.
handeyeco
changed the title
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