Omit extra properties from InteractiveGraph onChange arg #1654

benchristel · 2024-09-19T20:36:52Z

Previously, we were passing the entire Mafs graph state to onChange. Since
the onChange callback data is used by the Interactive Graph widget editor to
set the correct answer for the graph, a bunch of extra data like
"hasBeenInteractedWith": true was getting saved in the correct field of
the Widget data.

This extra data is not used, so its presence in datastore is potentially
confusing to future maintainers trying to understand the data. It might
also cause bugs in the future - e.g. if some code merges it into another
object in which those properties are meaningful.

This PR ensures that we only save relevant data in the correct field of
interactive graph widgets.

Issue: none

Test plan:

Edit, save and publish an interactive graph widget in an exercise. As a
learner, you should be able to complete the exercise successfully.

khan-actions-bot · 2024-09-19T20:37:08Z

Gerald

Required Reviewers

@Khan/perseus for changes to .changeset/eleven-elephants-trade.md, packages/perseus/src/types.ts, packages/perseus/src/widgets/interactive-graph.tsx, packages/perseus/src/widgets/interactive-graphs/mafs-state-to-interactive-graph.test.ts, packages/perseus/src/widgets/interactive-graphs/mafs-state-to-interactive-graph.ts, packages/perseus/src/widgets/interactive-graphs/stateful-mafs-graph.tsx

Don't want to be involved in this pull request? Comment #removeme and we won't notify you of further changes.

benchristel · 2024-09-19T20:38:10Z

packages/perseus/src/types.ts

@@ -90,7 +91,7 @@ export type ChangeHandler = (
        // perseus-all-package/widgets/grapher.jsx
        plot?: any;
        // Interactive Graph callback (see legacy: interactive-graph.tsx)
-        graph?: InteractiveGraphState;
+        graph?: PerseusGraphType;


This type was wrong - InteractiveGraphState is the internal state of our StatefulMafsGraph component - we don't want it to leak outside that component. PerseusGraphType is what gets saved in the widget JSON.

benchristel · 2024-09-19T20:38:40Z

packages/perseus/src/widgets/interactive-graph.tsx

@@ -1656,7 +1656,6 @@ class LegacyInteractiveGraph extends React.Component<Props, State> {

        $(this.angle).on("move", () => {
            this.onChange({
-                // @ts-expect-error Type '{ coords: any; type: "angle"; showAngles?: boolean | undefined; allowReflexAngles?: boolean | undefined; angleOffsetDeg?: number | undefined; snapDegrees?: number | undefined; match?: "congruent" | undefined; }' is not assignable to type 'InteractiveGraphState | undefined'.


The incorrect type above was causing this error.

github-actions · 2024-09-19T20:39:16Z

npm Snapshot: Published

Good news!! We've packaged up the latest commit from this PR (2e7e30a) and published it to npm. You
can install it using the tag PR1654.

Example:

yarn add @khanacademy/perseus@PR1654

If you are working in Khan Academy's webapp, you can run:

./dev/tools/bump_perseus_version.sh -t PR1654

github-actions · 2024-09-19T20:39:31Z

Size Change: +149 B (+0.02%)

Total Size: 862 kB

Filename	Size	Change
`packages/perseus/dist/es/index.js`	418 kB	+149 B (+0.04%)

ℹ️ View Unchanged

Filename	Size
`packages/kas/dist/es/index.js`	38.3 kB
`packages/keypad-context/dist/es/index.js`	760 B
`packages/kmath/dist/es/index.js`	4.27 kB
`packages/math-input/dist/es/index.js`	78 kB
`packages/math-input/dist/es/strings.js`	1.79 kB
`packages/perseus-core/dist/es/index.js`	1.48 kB
`packages/perseus-editor/dist/es/index.js`	278 kB
`packages/perseus-linter/dist/es/index.js`	22.2 kB
`packages/perseus/dist/es/strings.js`	3.36 kB
`packages/pure-markdown/dist/es/index.js`	3.66 kB
`packages/simple-markdown/dist/es/index.js`	12.4 kB

_{compressed-size-action}

benchristel · 2024-09-19T20:39:44Z

packages/perseus/src/widgets/interactive-graphs/mafs-state-to-interactive-graph.ts

TypeScript forces this function to be unfortunately verbose, but all it's doing is converting the circle point coords to center and radius, and passing the coords and type through for all the other graph types.

benchristel · 2024-09-19T20:42:24Z

packages/perseus/src/widgets/interactive-graphs/mafs-state-to-interactive-graph.ts

+// the the parent InteractiveGraph.
+//
+// The transformed state is used in the interactive graph widget editor, to
+// set the `correct` field of the graph options.


@jeremywiebe @nishasy I'd especially like your feedback on this comment. Is this correct? Does anything besides the editor handle widgets' onChange event?

I found some dead test prep code in webapp that was using onChange, but that doesn't seem worth mentioning here since I believe it's about to be deleted. I also had a vague memory that we saved learners' progress to the backend on change, but that didn't seem to be the case when I tested - perhaps that feature was only in test prep.

The renderer itself sends an onChange callback to widgets where it updates its state (widget state).

I also think that some widgets that accept user input send an onChange event with updated prop values and expect to be re-rendered with these new values. For example, the label-image widget does this (Github).

So I don't think the editor is the only thing that uses onChange. I am not sure as I've never dug through, but I've long wondered if we need such an all-encompassing onChange handler type or if we could break it apart by widget.

codecov · 2024-09-19T20:42:52Z

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 70.55%. Comparing base (7822ea6) to head (2e7e30a).
Report is 2 commits behind head on main.

Additional details and impacted files

@@            Coverage Diff             @@
##             main    #1654      +/-   ##
==========================================
- Coverage   70.67%   70.55%   -0.12%     
==========================================
  Files         559      586      +27     
  Lines      107880   112437    +4557     
  Branches     5306    11368    +6062     
==========================================
+ Hits        76240    79332    +3092     
- Misses      31519    33105    +1586     
+ Partials      121        0     -121

see 1145 files with indirect coverage changes

Continue to review full report in Codecov by Sentry.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 7822ea6...2e7e30a. Read the comment docs.

jeremywiebe

I think we could use a slight tweak to the comment (based on my comments in-line), but the code changes look good!

jeremywiebe · 2024-09-23T20:29:41Z

packages/perseus/src/widgets/interactive-graphs/mafs-state-to-interactive-graph.ts

+// the the parent InteractiveGraph.
+//
+// The transformed state is used in the interactive graph widget editor, to
+// set the `correct` field of the graph options.


The renderer itself sends an onChange callback to widgets where it updates its state (widget state).

I also think that some widgets that accept user input send an onChange event with updated prop values and expect to be re-rendered with these new values. For example, the label-image widget does this (Github).

jeremywiebe · 2024-09-23T20:30:47Z

packages/perseus/src/widgets/interactive-graphs/mafs-state-to-interactive-graph.ts

+// the the parent InteractiveGraph.
+//
+// The transformed state is used in the interactive graph widget editor, to
+// set the `correct` field of the graph options.


So I don't think the editor is the only thing that uses onChange. I am not sure as I've never dug through, but I've long wondered if we need such an all-encompassing onChange handler type or if we could break it apart by widget.

…)" This reverts commit c04d275.

The linked PR caused a bug where certain interactive graph types would lose key features after you interacted with them: - Polygon graphs would lose angle and side labels - Unlimited point graphs would only let you add a single point The cause was that the interactive graph widget was passing incomplete data to its onChange callback. The Renderer stored this data in its state, overwriting the original widget config. The borked data then got passed back down to the widget on the next render, which caused the features to disappear. Issue: none ## Test plan: `yarn dev; open http://localhost:5173` Turn on Mafs for the unlimited point and polygon graphs. Play with the graphs and confirm that everything looks OK. Author: benchristel Reviewers: nicolecomputer, #perseus Required Reviewers: Approved By: nicolecomputer Checks: ✅ codecov/patch, ❌ codecov/project, ✅ Upload Coverage (ubuntu-latest, 20.x), ✅ Publish npm snapshot (ubuntu-latest, 20.x), ✅ Check builds for changes in size (ubuntu-latest, 20.x), ✅ Cypress (ubuntu-latest, 20.x), ✅ Jest Coverage (ubuntu-latest, 20.x), ✅ Check for .changeset entries for all changed files (ubuntu-latest, 20.x), ✅ Lint, Typecheck, Format, and Test (ubuntu-latest, 20.x), 🚫 Upload Coverage, ✅ gerald, ✅ Publish npm snapshot (ubuntu-latest, 20.x), 🚫 Lint, Typecheck, Format, and Test (ubuntu-latest, 20.x), ✅ Check for .changeset entries for all changed files (ubuntu-latest, 20.x), ✅ Check builds for changes in size (ubuntu-latest, 20.x), ✅ Publish Storybook to Chromatic (ubuntu-latest, 20.x), 🚫 Jest Coverage (ubuntu-latest, 20.x), ✅ Cypress (ubuntu-latest, 20.x), ✅ gerald Pull Request URL: #1663

This reverts commit 1642ad9.

This change was previously implemented in PR #1654, then reverted in #1663 due to a bug. This PR reimplements the change without the bug. The original PR summary follows. --- Previously, we were passing the entire Mafs graph state to onChange. Since the onChange callback data is used by the Interactive Graph widget editor to set the correct answer for the graph, a bunch of extra data like "hasBeenInteractedWith": true was getting saved in the correct field of the Widget data. This extra data is not used, so its presence in datastore is potentially confusing to future maintainers trying to understand the data. It might also cause bugs in the future - e.g. if some code merges it into another object in which those properties are meaningful. This PR ensures that we only save relevant data in the correct field of interactive graph widgets. Issue: none ## Test plan: `yarn dev; open http://localhost:5173` Play around with the graphs on the dev UI gallery page. In particular: - Unlimited point - Polygons with angle and side labels - Polygons with side snapping You should not observe any issues. Edit, save and publish an interactive graph widget in an exercise. As a learner, you should be able to complete the exercise successfully. Author: benchristel Reviewers: jeremywiebe, nicolecomputer Required Reviewers: Approved By: jeremywiebe Checks: ✅ Publish npm snapshot (ubuntu-latest, 20.x), ✅ Lint, Typecheck, Format, and Test (ubuntu-latest, 20.x), ✅ Cypress (ubuntu-latest, 20.x), ✅ Check for .changeset entries for all changed files (ubuntu-latest, 20.x), ✅ Publish Storybook to Chromatic (ubuntu-latest, 20.x), ✅ Check builds for changes in size (ubuntu-latest, 20.x), ✅ gerald Pull Request URL: #1664

benchristel self-assigned this Sep 19, 2024

benchristel requested review from jeremywiebe and nishasy September 19, 2024 20:36

khan-actions-bot requested a review from a team September 19, 2024 20:37

benchristel commented Sep 19, 2024

View reviewed changes

jeremywiebe approved these changes Sep 23, 2024

View reviewed changes

benchristel added 6 commits September 23, 2024 15:50

Simplify mafsStateToInteractiveGraph

102bce7

Omit extra properties from InteractiveGraph onChange arg

953d069

Fix lint

db96a79

Add changeset

8bc3c8f

Update comment per PR review

59bb7d2

Fix types after rebasing

2e7e30a

benchristel force-pushed the benc/fix-onChange branch from adbd077 to 2e7e30a Compare September 23, 2024 22:51

benchristel merged commit c04d275 into main Sep 23, 2024
12 of 13 checks passed

benchristel deleted the benc/fix-onChange branch September 23, 2024 23:32

khan-actions-bot mentioned this pull request Sep 23, 2024

Version Packages #1660

Merged

benchristel added a commit that referenced this pull request Sep 24, 2024

Revert "Omit extra properties from InteractiveGraph onChange arg (#1654…

5ab3962

…)" This reverts commit c04d275.

benchristel added a commit that referenced this pull request Sep 24, 2024

Revert "Revert PR #1654 (#1663)"

5629b91

This reverts commit 1642ad9.

benchristel mentioned this pull request Sep 24, 2024

Reimplement mafsStateToInteractiveGraph #1664

Merged

benchristel added a commit that referenced this pull request Sep 27, 2024

Revert "Revert PR #1654 (#1663)"

d4e1926

This reverts commit 1642ad9.

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Omit extra properties from InteractiveGraph onChange arg #1654

Omit extra properties from InteractiveGraph onChange arg #1654

benchristel commented Sep 19, 2024

khan-actions-bot commented Sep 19, 2024 •

edited

Loading

benchristel Sep 19, 2024

benchristel Sep 19, 2024

github-actions bot commented Sep 19, 2024 •

edited

Loading

github-actions bot commented Sep 19, 2024 •

edited

Loading

benchristel Sep 19, 2024

benchristel Sep 19, 2024

jeremywiebe Sep 23, 2024

jeremywiebe Sep 23, 2024

codecov bot commented Sep 19, 2024 •

edited

Loading

jeremywiebe left a comment

jeremywiebe Sep 23, 2024

jeremywiebe Sep 23, 2024

Omit extra properties from InteractiveGraph onChange arg #1654

Omit extra properties from InteractiveGraph onChange arg #1654

Conversation

benchristel commented Sep 19, 2024

Test plan:

khan-actions-bot commented Sep 19, 2024 • edited Loading

Gerald

Choose a reason for hiding this comment

Choose a reason for hiding this comment

github-actions bot commented Sep 19, 2024 • edited Loading

npm Snapshot: Published

github-actions bot commented Sep 19, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

codecov bot commented Sep 19, 2024 • edited Loading

Codecov Report

jeremywiebe left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

khan-actions-bot commented Sep 19, 2024 •

edited

Loading

github-actions bot commented Sep 19, 2024 •

edited

Loading

github-actions bot commented Sep 19, 2024 •

edited

Loading

codecov bot commented Sep 19, 2024 •

edited

Loading