Repository navigation
fix(frontend): stop the evaluation run page minting an atom per read - #6196
Merged
Merged
Conversation
Opening an evaluation run with real data threw React error #185, "Maximum update depth exceeded", and the error boundary replaced the page. jotai's atomFamily keys its cache by REFERENCE unless given an areEqual: 2.20's implementation is a plain atoms.get(param) against a Map. Every family in EvalRunDetails is keyed by an object literal rebuilt at the call site, e.g. scenarioColumnValues.ts:485 does get(scenarioStepsQueryFamily({scenarioId, runId})) so the Map never hits and each call mints a brand-new atom. For the query families that is self-driving: each generation mounts its own observer, the observer emits, the emit invalidates the atom that created it, and the re-read mints the next generation. React counts an update per generation and throws. Measured on a seeded 25-scenario non-terminal run, where ~200 cells should create ~200 atoms once: before 2.5s 486 column-value atoms, 1504 step-query atoms 20s 3244 column-value atoms, 10744 step-query atoms, climbing after 2.5s 144 column-value atoms, 24 step-query atoms 20s 144 column-value atoms, 24 step-query atoms, flat This is why it never reproduced on an empty project: the steps query is gated on enabled: Boolean(effectiveRunId && batcher && scenarioId), so with no scenarios nothing reads the family. A non-terminal run makes it worse, because refetchInterval 5000 keeps every generation's observer emitting. The atoms themselves are byte-identical to v0.112.3. The extraction (#6065) replaced the antd table with the virtualized engine, which re-renders cells far more, and that is what pushed a latent churn over React's update-depth limit. Adds sameFamilyKey and passes it to the 24 object-keyed families across 9 files. Families keyed by a plain string are left alone: an areEqual makes jotai walk the Map linearly instead of hashing, so it would be a regression there.
dosubot
Bot
added
the
size:XL
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
|
Mahmoud Mabrouk seems not to be a GitHub user. You need a GitHub account to be able to sign the CLA. If you have already a GitHub account, please add the email address used for this commit to your account. You have signed the CLA already but the status is still pending? Let us recheck it. |
Contributor
📝 WalkthroughSummary by CodeRabbit
WalkthroughChangesThe change adds Atom-family equality
Estimated code review effort: 3 (Moderate) | ~20 minutes Merge Risk: 🟡 Moderate · up to The change addresses the evaluation run update loop, but it also uses direct, unvalidated API requests at several data-loading boundaries, which can allow incompatible responses or parameter handling to reach the page; this should be corrected or explicitly accepted before merge. 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
mmabrouk
marked this pull request as draft
August 22, 2026 08:38
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository YAML (base), Organization UI (inherited)
Review profile: CHILL
Plan: Pro Plus
Run ID: 582ff58c-bc63-48ca-b2d0-7df5fd566f30
📒 Files selected for processing (11)
web/oss/src/components/EvalRunDetails/atoms/annotations.tsweb/oss/src/components/EvalRunDetails/atoms/familyKeys.test.tsweb/oss/src/components/EvalRunDetails/atoms/familyKeys.tsweb/oss/src/components/EvalRunDetails/atoms/invocationTraceSummary.tsweb/oss/src/components/EvalRunDetails/atoms/metrics.tsweb/oss/src/components/EvalRunDetails/atoms/scenarioColumnValues.tsweb/oss/src/components/EvalRunDetails/atoms/scenarioSteps.tsweb/oss/src/components/EvalRunDetails/atoms/scenarioTestcase.tsweb/oss/src/components/EvalRunDetails/atoms/table/testcases.tsweb/oss/src/components/EvalRunDetails/atoms/traces.tsweb/oss/src/components/EvalRunDetails/hooks/useScenarioStepsSelectors.ts
Included review availability: Your plan provides up to 8 included reviews per hour; 6 remain after this review.
Contributor
Railway Preview Environment
|
mmabrouk
marked this pull request as ready for review
August 22, 2026 10:40
dosubot
Bot
added
the
refactoring
This branch was successfully deployed
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Context
Opening an evaluation run threw "Minified React error #185" and the error boundary replaced the page. React #185 is "Maximum update depth exceeded", so something was updating state without settling.
atomFamilyin jotai keys its cache by reference unless you give it anareEqual. Version 2.20's implementation is a plainatoms.get(param)against aMap:Every family under
EvalRunDetailsis keyed by an object literal rebuilt at the call site.scenarioColumnValues.ts:485is the load-bearing one:That literal is new on every evaluation, so the
Mapnever hits and the family mints a brand-new atom each time. For the query families that is self-driving: each generation mounts its ownQueryObserver, the observer emits, the emit invalidates the atom that created it, and the re-read mints the next generation. React counts an update per generation and eventually throws.Why it never reproduced on an empty project
I could not reproduce this for a long time, and the reason is in the gate at
scenarioSteps.ts:147:No scenarios means no cells, which means nothing ever reads the family. A run also has to be non-terminal to be at its worst, because
refetchInterval: runTerminal ? false : 5000keeps every generation's observer emitting on a timer.The measurement
I seeded a 25-scenario non-terminal run through the API and counted atom creations. About 200 cells should create about 200 atoms, once.
Before, the counters grew by roughly 2,584 step-query atoms every five seconds, which is the refetch timer driving each generation. After, they reach their real size and stop.
Changes
Adds
atoms/familyKeys.tswithsameFamilyKey, and passes it to the 24 object-keyed families across 9 files. The comparator compares the union of both key sets, so an absent key and an explicitundefinedmatch, and it goes one level deeper for the column descriptor, whosepathSegmentsis an array of scalars.Families keyed by a plain string are deliberately left alone. Giving those an
areEqualmakes jotai walk the wholeMaplinearly instead of hashing, so it would be a regression, not a fix. Three of them were caught and reverted while preparing this.A note on where the bug came from
The atoms are byte-identical to v0.112.3.
git diff v0.112.3..HEAD -- web/oss/src/components/EvalRunDetailsis nine files of pure import-path and type swaps. This churn is older than the release.What #6065 changed is the engine underneath: the antd table became the virtualized
InfiniteVirtualTable, which re-renders cells far more often. That is what pushed a latent, bounded-looking churn past React's update-depth limit. Worth remembering when triaging the next "the extraction broke X": the extraction can be the trigger without being the author.Tests
atoms/familyKeys.test.ts, 10 cases: the comparator's own behaviour, that anatomFamilygiven it returns the same atom for equal keys built fresh, that it still separates genuinely different keys, and one test that pins jotai's reference-keying so we find out if a future version makes these comparators redundant.@agenta/osssuite: 45 files, 442 passed, 1 skipped.tsc --noEmitclean for@agenta/ossand@agenta/ee.pnpm lint-fixinweb/: 25/25. Prettier 3.7.4 clean on all touched files.@agenta/ossproduction build.What to QA