Repository navigation
[R4b] fix(frontend): evaluations list update-depth loop with metric columns - #6204
Merged
Merged
Conversation
…columns Opening /evaluations threw React error #185, "Maximum update depth exceeded", and the error boundary replaced the page. It only happened once a run carried `data.mappings` that produce evaluator METRIC columns, which is why an empty project never reproduced it. `setOutputTypesMap` notified its listeners unconditionally: outputTypesCache.set(key, map) globalVersion += 1 listeners.forEach((listener) => listener()) Its only caller is the metric GROUP header, which rebuilds the map from the evaluator's schema on render and passes a NEW Map with the SAME content every time. `useEvaluationRunsColumns` subscribes to that key and its listener calls `setOutputTypesVersion(v => v + 1)`, so: write -> notify -> setState -> re-render -> rebuild map -> write -> notify -> ... An infinite synchronous loop through a module-level cache, which is exactly what React reports: a component sets state from an effect whose dependency changes on every render. The metric columns arm it because the group header is the only caller. The fix is to make the write idempotent: compare the map's content and return early, without bumping the version or notifying, when nothing changed. Worth recording, because every stack trace pointed elsewhere: the reported error always surfaced inside Radix's `composeRefs` under `ColumnVisibilityTrigger` (`dispatchSetState <- setRef <- Array.map <- ... <- safelyDetachRef <- commitDeletionEffectsOnFiber`). That is where the loop SURFACES, not where it starts — the churning columns remount the header subtrees and Radix detaches composed refs during the deletion. Five hypotheses in the table chrome were eliminated by experiment before instrumenting identity changes showed the hook's inputs were all stable (rows 3, everything else 1-2 changes across 129 renders) while its output churned 69 times, which led back through `isMetricHidden` and `outputTypesVersion` to this write. Verified on a dev build against a real EE API, with six seeded runs whose mappings produce metric columns: before crashed, 0 rows, 0 headers after 6 rows, and the metric columns render for the first time — "Contains JSON", "Exact Match", "Quality Rating", each with Score and Verdict after a reload with column-visibility state persisted: still clean Six tests on the rule the loop violated; two are red without the guard.
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
|
|
Contributor
|
Caution Review failedThe pull request is closed. ℹ️ Recent review info⚙️ Run configurationConfiguration used: Repository YAML (base), Organization UI (inherited) Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (2)
Disabled knowledge base sources:
📝 WalkthroughSummary by CodeRabbit
WalkthroughThe evaluator output type cache now compares incoming maps with cached content. Unchanged maps skip cache replacement, version increments, and listener notifications. New tests cover map changes, ChangesEvaluator output type updates
Estimated code review effort: 2 (Simple) | ~15 minutes ✨ 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 |
Member
Author
|
Full local verification for this branch:
|
Contributor
Railway Preview Environment
|
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.
The bug
Opening
/evaluationsthrew React error #185, "Maximum update depth exceeded", and the error boundary replaced the page. Hit twice by Mahmoud on staging; also reproduced by QA on staging with a real pipeline run.It only happened once a run carried
data.mappingsthat produce evaluator metric columns — which is why an empty project never reproduced it, and why a first round of seeded data (runs with evaluator references but no mappings) came back clean.Regression from #6065
The defective write is older than this release:
setOutputTypesMapand its subscriber both arrived inabcd1c770c("metric display fixes"), which is an ancestor ofv0.112.3. Production renders these pages fine, so the write alone is not sufficient.What changed in v0.113.0 is the renderer. #6065 (
lane/mobile-extracted-packages) contains bothcfb8c27974("TanStack Table and Virtual drive the table") and7628e20346("make @agenta/ui runtime antd-free, deleting the antd table"); neither is inv0.112.3.toTanstackColumnsbuilds anonymous header component functions, so a rebuilt column list is a new component type and React remounts the header subtree instead of updating it. Under the old antd table the same rebuild updated in place. That remount re-runs the group header's write, which closes the loop.Regression from #6065: the table switched to the TanStack renderer, whose per-rebuild anonymous header components remount header subtrees, turning a pre-existing non-idempotent cache write into a self-sustaining loop.
Root cause
setOutputTypesMapnotified its listeners unconditionally:Its only caller is
MetricGroupHeader, which rebuilds the map from the evaluator's schema on render and hands over a newMapwith the same content every time.useEvaluationRunsColumnssubscribes to that key and its listener callssetOutputTypesVersion(v => v + 1). So:An infinite synchronous loop through a module-level cache — exactly what React reports: a component sets state from an effect whose dependency changes on every render.
The fix: compare the map's content and return early — no version bump, no notify — when nothing changed.
Isolation matrix
Measured on a live repro, each state loaded fresh, same data:
evaluatorWorkflowQueryAtomFamilycomparator (#6203) ONLYThe idempotent write is necessary and sufficient. #6203 stays a separate hardening PR.
Why every stack trace pointed somewhere else
The reported error always surfaced inside Radix
composeRefsunderColumnVisibilityTrigger:That is where the loop surfaces, not where it starts.
docs/design/observability-chrome/evaluations-render-loop.mdandSimpleTooltip's own docstring both name that trigger and mark the bug unresolved. Five hypotheses in the table chrome were eliminated by experiment (horizontal overflow, the trigger's tooltip,MetricColumnHeadercontent,RunMetricCellContentcontent, theColumnVisibilityHeaderref callback) before instrumenting identity changes found it: the hook's inputs were all stable (rows3, everything else 1–2 across 129 renders) while its output churned 69 times, which led throughmetricNodes→isMetricHidden→outputTypesVersionto this write.Independent agreement
Two independent reviews reached the same cycle. Codex (gpt-5.6, read-only, clean checkout) named
MetricGroupHeaderas the cache writer andtoTanstackColumns's anonymous header components as the remount mechanism, and proposed essentially this diff. A third static review named the same write, the same listener, and the same conclusion that the Radix anchor ref is only the last update React sees. Codex additionally argued the atom-family comparator is the essential change; the matrix above tests that and shows it is not.The "existing data" bump — checked, and deliberately not changed here
useEvaluationRunsColumnsalso does an unconditional bump:That is genuinely not idempotent. It is not the driver here, because the effect's dependency (
metricGroupsForRendering) resolves to the memoizedevaluatorMetricGroupsarray in this scenario and so is stable in practice (measured: 3 identity changes across 129 renders). Making it idempotent is not a small change in the same spirit — it would need to track what the columns were last built from, which nothing records today. The cheap correct fix is to memoizemetricGroupsForRendering(that expression atindex.tsx:407is unmemoized while being an effect dependency). Left out to keep this PR minimal; listed below.Verification
Dev build of this branch's exact content against a real EE API, six seeded runs whose mappings produce metric columns, mixed statuses with some non-terminal:
The reload case matters: it is the exact condition the old design doc says was required for the earlier instance of this crash.
What I ran
pnpm --filter @agenta/oss exec vitest run— 48 files, 455 passed, 1 skippedpnpm --filter @agenta/oss exec tsc --noEmit --incremental false— cleannpx prettier --checkon both touched files — cleanpnpm build-oss— 10 successful, 10 total, exit 0Six tests on the rule the loop violated; two are red without the guard (verified by removing it and re-running).
Follow-ups, not in this PR
An ownership smell worth fixing properly: the header mutates the state that produced the header.
MetricGroupHeaderresolves evaluator output types after mount and writes them into a cache that the column builder reads. The durable fix is to resolve output types before constructing columns and keep headers pure renderers; that is a refactor, not a release fix.Hygiene items found along the way, to file as one issue after the release:
useSmartResizableColumnsreturns a freshheaderComponentsobject literal every render.metricGroupsForRenderingis unmemoized while being an effect dependency (see above).VirtualTable.tsx:485spreadsonHeaderCellresize props onto a native<th>— the source of the permanentminWidth/onResizeStart/onResizeStopconsole warnings.toTanstackColumnsbuilds anonymous header components — a remount hazard by construction, and the mechanism behind this regression.