[Image] | (fix) | Batch setState calls in setupGraphie to prevent infinite loop#3329
[Image] | (fix) | Batch setState calls in setupGraphie to prevent infinite loop#3329
Conversation
…ulate the situation We need the rerender to happen at a specific point in the graphie loading process, but this doesn't do that
🗄️ Schema Change: No Changes ✅ |
|
Size Change: +18 B (0%) Total Size: 486 kB
ℹ️ View Unchanged
|
🛠️ Item Splitting: No Changes ✅ |
…by batching setState calls in setupGraphie
npm Snapshot: PublishedGood news!! We've packaged up the latest commit from this PR (854b70a) and published it to npm. You Example: pnpm add @khanacademy/perseus@PR3329If you are working in Khan Academy's frontend, you can run the below command. ./dev/tools/bump_perseus_version.ts -t PR3329If you are working in Khan Academy's webapp, you can run the below command. ./dev/tools/bump_perseus_version.js -t PR3329 |
There was a problem hiding this comment.
Will remove this before landing!
github-actions
bot
added
item-splitting-change
and removed
item-splitting-change
labels
…l labels rendered
…'t catch the issue
…Graphie setState Reads from `prev` instead of `this.state` to guarantee the latest state is used, avoiding potential staleness if `setupGraphie` were ever called while a setState is still pending.
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
There was a problem hiding this comment.
LGTM — straightforward batching of setState calls to fix infinite loop.
Extended reasoning...
Overview
This PR fixes a React "Maximum update depth exceeded" error (LEMS-3853) by batching setState calls in SvgImage.setupGraphie. The only meaningful code change is ~10 lines in packages/perseus/src/components/svg-image.tsx: moving per-label setState calls out of a loop into a single batched call with a functional updater (prev) => .... The other files are a changeset and an investigation plan doc (author noted it will be removed before landing).
Security risks
None. The change is purely a React state management fix — no user input handling, auth, or data exposure changes.
Level of scrutiny
Low scrutiny needed. The fix applies a well-established React pattern (accumulate updates, single setState with functional updater). The before/after behavior is easy to reason about, and the PR description thoroughly explains the root cause. The change is small and self-contained.
Other factors
No bugs were found by the automated bug hunting system. The author has an outstanding note to remove .claude/plans/lems-3853-summary.md before landing, but that is a non-code housekeeping item that does not affect correctness. The bundle size impact is negligible (+18 B).
1 participant
Problem
SvgImage.setupGraphiecalledsetStateonce per label inside a loop. Becausethis.statedoesn't update synchronously betweensetStatecalls in class components, each iteration spread the same stale base — leavinglabelsRenderedin an inconsistent intermediate state. An async prop change fromImageComponent'szoomSizeeffect would land mid-loop, bypassingshouldComponentUpdateand exposingGraphieto the bad state.Graphieinterpreted this as a meaningful options change, re-ransetupGraphie, and triggered another setState storm — repeating until React hit its update depth limit.Solution
Accumulate all label updates into a plain object during the loop and call
setStateonce. The state transition is now atomic, eliminating intermediate inconsistent states and the re-render storm they caused. The updater also uses a functional form (prev => ...) so the merge always reads from the latest committed state rather than this.state at call time.Issue: LEMS-3853
Test plan:
where that matters)
Completed with Claude Code