docs(changeset): Removing Interactive Graph Builder and refactoring tests/storybook to use more standardized generator patterns.#3415
docs(changeset): Removing Interactive Graph Builder and refactoring tests/storybook to use more standardized generator patterns.#3415SonicScrewdriver wants to merge 12 commits intomainfrom
Conversation
…se similar generator patterns to the rest of Perseus.
…r and refactoring tests/storybook to use more standardized generator patterns.
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
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.
Tip: disable this comment in your organization's Code Review settings.
🗄️ Schema Change: No Changes ✅ |
|
Size Change: +206 B (+0.04%) Total Size: 495 kB
ℹ️ View Unchanged
|
🛠️ Item Splitting: No Changes ✅ |
npm Snapshot: PublishedGood news!! We've packaged up the latest commit from this PR (b82456b) and published it to npm. You Example: pnpm add @khanacademy/perseus@PR3415If you are working in Khan Academy's frontend, you can run the below command. ./dev/tools/bump_perseus_version.ts -t PR3415If you are working in Khan Academy's webapp, you can run the below command. ./dev/tools/bump_perseus_version.js -t PR3415 |
| }); | ||
| }); | ||
|
|
||
| describe("generateIGExponentialGraph", () => { |
There was a problem hiding this comment.
Decided to round out the tests for the new graphs while I was in here.
|
|
||
| export function generateIGAngleGraph( | ||
| options?: Partial<PerseusGraphTypeAngle>, | ||
| options?: Partial<Omit<PerseusGraphTypeAngle, "type">>, |
There was a problem hiding this comment.
This is just some additional type safety that brings these generators inline with the implementation of the rest of the IG generators.
| "*.cypress.ts", | ||
| "*.test.ts", | ||
| "*.test.tsx", | ||
| "*.testdata.ts", |
There was a problem hiding this comment.
It made sense to me to add a global override for our test data as it seems reasonable for these files to be exempt, but I'm also happy to remove this and re-add the file-specific override for interactive-graph.testdata.ts.
|
@claude review |
...ges/perseus/src/widgets/interactive-graphs/__docs__/interactive-graph-regression.stories.tsx
Show resolved
Hide resolved
packages/perseus-core/src/utils/generators/interactive-graph-widget-generator.ts
Show resolved
Hide resolved
packages/perseus/src/widgets/interactive-graphs/interactive-graph.testdata.ts
Show resolved
Hide resolved
...ges/perseus/src/widgets/interactive-graphs/__docs__/interactive-graph-regression.stories.tsx
Show resolved
Hide resolved
packages/perseus-core/src/utils/generators/interactive-graph-widget-generator.ts
Outdated
Show resolved
Hide resolved
packages/perseus/src/widgets/interactive-graphs/interactive-graph.testdata.ts
Show resolved
Hide resolved
packages/perseus/src/widgets/interactive-graphs/interactive-graph.testdata.ts
Show resolved
Hide resolved
| export const circleWithStartingCoordsQuestion: PerseusRenderer = | ||
| interactiveGraphQuestionBuilder() | ||
| .withCircle({startCoords: {center: [9, 9], radius: 5}}) | ||
| .build(); | ||
| generateInteractiveGraphQuestion({ | ||
| graph: generateIGCircleGraph({ | ||
| startCoords: {center: [9, 9], radius: 5}, | ||
| }), | ||
| correct: generateIGCircleGraph(), | ||
| }); |
There was a problem hiding this comment.
🔴 The circleWithStartingCoordsQuestion fixture in packages/perseus-editor/src/testdata/interactive-graph.testdata.ts calls correct: generateIGCircleGraph() with no arguments, producing {type: 'circle'} with no center or radius — dropping the correct answer that the old CircleGraphConfig defaulted to {center: [0, 0], radius: 2}. Fix by changing to correct: generateIGCircleGraph({center: [0, 0], radius: 2}), matching what was done for the analogous fixture in the perseus package.
Extended reasoning...
What the bug is and how it manifests
The fixture circleWithStartingCoordsQuestion in packages/perseus-editor/src/testdata/interactive-graph.testdata.ts (lines 165-171) was migrated from the old builder pattern to:
correct: generateIGCircleGraph()
generateIGCircleGraph() with no arguments returns {type: 'circle'} — no center, no radius. The rubric's correct answer is therefore missing both required fields.
The specific code path that triggers it
The deleted CircleGraphConfig constructor in the old builder explicitly defaulted these fields:
this.correctCenter = options?.center ?? [0, 0];
this.correctRadius = options?.radius ?? 2;
So .withCircle({startCoords: {center: [9, 9], radius: 5}}) (passing only startCoords, no center/radius) still produced correct: {type: 'circle', center: [0, 0], radius: 2}. The new generateIGCircleGraph() is a thin spread with no defaults — whatever you pass is what you get, and passing nothing yields {type: 'circle'}.
Why existing code does not prevent it
There is no TypeScript-enforced requirement that circle graph correct answers include center and radius. The PerseusGraphTypeCircle type marks them as optional. The generator is intentionally minimal (opt-in fields), so the responsibility falls on call sites to pass defaults explicitly.
What the impact would be
The fixture is consumed by the InteractiveGraphCircle Storybook story in packages/perseus-editor/src/widgets/docs/interactive-graph-editor.stories.tsx (line 109). The editor preview renders a circle question with no defined correct answer. Any scoring logic reading rubric.correct.center or rubric.correct.radius from this fixture receives undefined. Per score-interactive-graph.ts, circle scoring does approximateDeepEqual(userInput.center, rubric.correct.center) — with rubric.correct.center === undefined and a real array as userInput.center, approximateDeepEqual returns false immediately (the Array.isArray guard in equality.ts fires), making the question unscorable as correct.
How to fix it
Change the fixture to:
correct: generateIGCircleGraph({center: [0, 0], radius: 2})
This restores the old CircleGraphConfig defaults exactly.
Step-by-step proof
- Old code: .withCircle({startCoords: {center: [9, 9], radius: 5}}) triggers CircleGraphConfig which sets correctCenter = options?.center ?? [0, 0] = [0, 0] and correctRadius = options?.radius ?? 2 = 2, so correct() returns {type: 'circle', center: [0, 0], radius: 2}.
- New code: correct: generateIGCircleGraph() returns {type: 'circle'} with no center or radius.
- The InteractiveGraphCircle Storybook story renders the editor with this fixture; the widget rubric carries correct: {type: 'circle'}.
- Scoring: approximateDeepEqual(userInput.center, undefined) — Array.isArray(userInput.center) is true — returns false immediately per equality.ts.
- The question can never be scored correct regardless of where the user drags the circle.
- Contrast: the analogous circleQuestionWithDefaultCorrect in packages/perseus/src/widgets/interactive-graphs/interactive-graph.testdata.ts was correctly migrated to generateIGCircleGraph({center: [0, 0], radius: 2}), proving the developer knew to add these defaults — the editor-package fixture was simply missed.
|
|
||
| export function generateInteractiveGraphQuestion( | ||
| options?: Partial<PerseusInteractiveGraphWidgetOptions> & { | ||
| content?: string; | ||
| isStatic?: boolean; | ||
| }, | ||
| ): PerseusRenderer { | ||
| const {content, isStatic, ...widgetOptions} = options ?? {}; | ||
|
|
||
| // The `graph` and `correct` fields share all fields except for | ||
| // the answers (coords, center, etc.) When only `correct` is provided, | ||
| // derive `graph` from it by stripping the answer-specific fields. | ||
| // This allows us to keep our test data more succinct. | ||
| if (widgetOptions.correct && !widgetOptions.graph) { | ||
| const { | ||
| coords: _, | ||
| coord: __, | ||
| center: ___, | ||
| radius: ____, | ||
| asymptote: _____, | ||
| match: ______, | ||
| ...graphConfig | ||
| } = widgetOptions.correct as Record<string, unknown>; |
There was a problem hiding this comment.
🟡 The developer architecture notes in logarithm.md (line 322) and tangent.md (line 187) still reference interactive-graph-question-builder.ts, which this PR deletes. Developers following these notes to implement a new graph type will hit a broken file reference; update both notes to point to the replacement module interactive-graph-widget-generator.ts.
Extended reasoning...
What the bug is and how it manifests
This PR deletes packages/perseus/src/widgets/interactive-graphs/interactive-graph-question-builder.ts and replaces it with packages/perseus-core/src/utils/generators/interactive-graph-widget-generator.ts. However, two developer architecture notes — logarithm.md and tangent.md — were not updated alongside this deletion. Both notes contain explicit file-path references that are now dead.
packages/perseus/src/widgets/interactive-graphs/__docs__/notes/logarithm.mdline 322 references...interactive-graph-question-builder.ts — withLogarithm(), LogarithmGraphConfigpackages/perseus/src/widgets/interactive-graphs/__docs__/notes/tangent.mdline 187 references...interactive-graph-question-builder.ts — Builder
The specific code path that triggers it
These notes are static Markdown documentation files intended to guide developers implementing new graph types. They list the files and functions a developer needs to touch, with interactive-graph-question-builder.ts called out as containing the builder class. Since that file no longer exists in the repository after this PR, any developer following the guide will look for a file that is simply gone.
Why existing code doesn't prevent it
The deletion of the builder was a source-code change; there is no automated tooling that cross-checks prose documentation for file-path accuracy. The notes were written to describe the old architecture and were not updated in this refactor.
What the impact would be
A developer tasked with adding a new interactive graph type (e.g., logarithm or tangent) would consult these notes, encounter the dead reference, and either waste time searching for the file or be left confused about where the builder logic now lives. The correct file is packages/perseus-core/src/utils/generators/interactive-graph-widget-generator.ts, which is where all the new generator functions (e.g., generateIGExponentialGraph, generateIGAbsoluteValueGraph) live.
How to fix it
In both logarithm.md:322 and tangent.md:187, replace the reference to interactive-graph-question-builder.ts with packages/perseus-core/src/utils/generators/interactive-graph-widget-generator.ts and update the function/class names cited to match the new generator API (e.g., the generator functions instead of withLogarithm()/LogarithmGraphConfig/Builder).
Step-by-step proof
- The PR diff shows
interactive-graph-question-builder.tsdeleted (entire file removed with--- a/.../interactive-graph-question-builder.tsdiff header). logarithm.mdline 322 contains the literal stringinteractive-graph-question-builder.ts — withLogarithm(), LogarithmGraphConfig— a path that no longer resolves.tangent.mdline 187 contains the literal stringinteractive-graph-question-builder.ts — Builder— also a dead path.- The new replacement module
interactive-graph-widget-generator.tsis added by this PR and exports graph generators such asgenerateIGTangentGraph,generateIGExponentialGraph, etc. - Neither doc file appears in the PR diff, confirming neither was updated.
2 participants
Summary:
[maximum-overline] Remove Interactive Graph Builder and refactor to use similar generator patterns to the rest of Perseus.
Issue: LEMS-XXXX
Test plan: