-
Notifications
You must be signed in to change notification settings - Fork 364
fix(LEMS-3701): update editorPage to sync json state #3196
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: main
Are you sure you want to change the base?
Conversation
🗄️ Schema Change: No Changes ✅ |
npm Snapshot: PublishedGood news!! We've packaged up the latest commit from this PR (ac23e42) and published it to npm. You Example: pnpm add @khanacademy/perseus@PR3196If you are working in Khan Academy's frontend, you can run the below command. ./dev/tools/bump_perseus_version.ts -t PR3196If you are working in Khan Academy's webapp, you can run the below command. ./dev/tools/bump_perseus_version.js -t PR3196 |
|
Size Change: +186 B (+0.04%) Total Size: 485 kB
ℹ️ View Unchanged
|
🛠️ Item Splitting: No Changes ✅ |
1d24064 to
1eed041
Compare
| }); | ||
| } | ||
| } | ||
| } |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This reads much better! Thank you!!
| componentDidUpdate() { | ||
| getSnapshotBeforeUpdate(prevProps: Props, prevState: State) { | ||
| if (!prevProps.jsonMode && this.props.jsonMode) { | ||
| return _.extend( |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Instead of adding an underscore function (I think we are wanting to eventually get rid of underscore), could we write this as:
if (!prevProps.jsonMode && this.props.jsonMode) {
const serializedItemEditor = this.itemEditor.current?.serialize({
keepDeletedWidgets: true,
}) || {};
const serializedHintsEditor = hints: this.hintsEditor.current?.serialize({
keepDeletedWidgets: true,
});
return {...serializedItemEditor, ...serializedHintsEditor};
}
return null;
ivyolamit
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@anakaren-rojas here's my initial comment/question. Will do a second pass and smoke test.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We can remove code in line 2, if UNSAFE tag is removed
/* eslint-disable react/no-unsafe */
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I wish ESLint would tell us if there's "disable" statements that are unused. 🤷♂️
| }; | ||
| } | ||
|
|
||
| UNSAFE_componentWillReceiveProps(nextProps) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Do you know why this was marked UNSAFE?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Update, I asked Claude about this:
- UNSAFE_componentWillReceiveProps = unsafe (has issues with concurrent rendering)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
"componentWillReceiveProps was problematic because it was called before every render triggered by a prop change, making it difficult to distinguish between essential updates and potentially causing infinite loops or unpredictable behavior, especially with asynchronous operations. "
😓
| syncJsonStateFromProps() { | ||
| this.setState({ | ||
| // @ts-expect-error - TS2322 - Type 'Pick<Readonly<Props> & Readonly<{ children?: ReactNode; }>, "hints" | "question" | "answerArea">' is not assignable to type 'PerseusJson'. | ||
| json: _.pick(this.props, "question", "answerArea", "hints"), |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Perhaps the following (to avoid adding underscore):
syncJsonStateFromProps() {
this.setState({
json: {
question: this.props.question,
answerArea: this.props.answerArea,
hints: this.props.hints,
},
});
}
| ); | ||
|
|
||
| // Assert | ||
| expect(screen.getByDisplayValue(/Updated content/)).toBeInTheDocument(); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should we also assert that "Initial content" is NOT in the document?
|
|
||
| await userEvent.clear(textarea); | ||
| textarea.focus(); | ||
| await userEvent.paste('{"content": "User is typing'); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I am missing something - how is this invalid user input?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It's incomplete, its missing the " and the closing bracket
{"content": "User is typing
instead of:
{"content": "User is typing"}
| expect(screen.getByRole("textbox")).toBeDisabled(); | ||
| }); | ||
|
|
||
| it("should replace invalid user input when parent updates value", async () => { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should there be a test for valid user input when the parent updates the value?
mark-fitzgerald
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for fixing this!!
jeremywiebe
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think this makes sense. Thanks for fixing this! I think I've doubled-up on some of Mark's comments but I'd typed them before seeing his. :/
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I wish ESLint would tell us if there's "disable" statements that are unused. 🤷♂️
| this.state.currentValue ? this.state.currentValue : "", | ||
| ), | ||
| ); | ||
| componentDidUpdate(prevProps: Props) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Given that this function is used to update state, could we just use the static getDerivedStateFromProps instead? I'm not sure it's a huge difference, but it's a static method (which is nice as it is then a pure function.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thank you for the suggestion!
I looked into it and it doesn't seem like it would work for our use case 😞
The problem we were having was that the JsonEditor wasn't syncing when the parent sent updated props (whether on load or later), so we need ongoing prop synchronization
With componentDidUpdate, we're checking if the value prop from the parent has changed, not just on initial load but throughout the component's lifetime
| } | ||
|
|
||
| componentDidUpdate() { | ||
| getSnapshotBeforeUpdate(prevProps: Props, prevState: State) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
TIL: Cool, I hadn't seen this React function before.
| return _.extend( | ||
| this.itemEditor.current?.serialize({ | ||
| keepDeletedWidgets: true, | ||
| }) || {}, | ||
| { | ||
| hints: this.hintsEditor.current?.serialize({ | ||
| keepDeletedWidgets: true, | ||
| }), | ||
| }, | ||
| ); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can we use a simple object spread here instead of introducing another use of _?
| return _.extend( | |
| this.itemEditor.current?.serialize({ | |
| keepDeletedWidgets: true, | |
| }) || {}, | |
| { | |
| hints: this.hintsEditor.current?.serialize({ | |
| keepDeletedWidgets: true, | |
| }), | |
| }, | |
| ); | |
| return { | |
| ...this.itemEditor.current?.serialize({ | |
| keepDeletedWidgets: true, | |
| }) ?? {}, | |
| hints: this.hintsEditor.current?.serialize({ | |
| keepDeletedWidgets: true, | |
| }, | |
| ); |
Summary:
JSON editor data wasn't being synced with Editor Page data
JSON Editor
valueprop changed before updating stateEditor Page
Issue: LEMS-3701
Test plan:
Screen recordings
Before
Screen.Recording.2026-01-26.at.16.28.27.mov
After
Screen.Recording.2026-01-26.at.16.29.14.mov