Repository navigation
Conversation
`atomFamily` keys its cache by REFERENCE unless it is given an `areEqual`, so an
object-literal key never hits the cache and every read mints a brand-new atom.
`evaluatorWorkflowQueryAtomFamily` and `evaluatorReferenceAtomFamily` are both keyed
by a literal that callers rebuild at each call site, and neither declared a
comparator. The neighbouring families in the same file already do, including
`workflowArtifactScopedQueryAtomFamily`, which is read fifteen lines away in the same
function and is safe purely because of it.
`evaluatorReferenceAtomFamily` reads the query family from inside its own read
function, so the miss also feeds back: each generation mounts its own query observer,
the observer emits, and the emit invalidates the reader that created it. Measured
trajectory over 200 ticks with the comparator removed:
atoms: 1, 2, 2, 2, 2, 2, 2, 2, 2, 2 notifications: 1
It settles at two rather than running away, because this query carries a five minute
`staleTime` and neither polls nor refetches on focus, so generation two reads settled
cached data and emits nothing further. So this is a duplicated atom and a duplicated
fetch per call site, NOT the runaway that produced React error #185 on the run-details
page in #6196 — that family polled while a run was non-terminal, which is what
compounded. Adding a `refetchInterval` here would turn this into that bug.
The reference family's comparator also normalises `null` against `undefined`, because
`useEvaluatorHeaderReference` spells the absent half of the key one way and
`ReferenceLabels` the other, which otherwise gives one evaluator two cache entries and
two fetches.
The same omission in `queriesQueryAtomFamily` and its two selector families is fixed
alongside it. `evaluationRunsQueryOptionsAtom` reads the first one from inside a
derived atom on this very page, and is safe today only because that call site happens
to pass hoisted constants.
Two tests: one asserts the rule, that an equal-but-not-identical key returns the same
atom; the other drives the real code path in a jotai store and counts the atoms one
subscribed evaluator column creates. Both are red without the comparators.
This is hardening. It is NOT the fix for the #185 seen on the /evaluations list page,
which I could not reproduce and have not root-caused.
|
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. |
|
Important Review skippedDraft detected. Please check the settings in the CodeRabbit UI or the ⚙️ Run configurationConfiguration used: Repository YAML (base), Organization UI (inherited) Review profile: CHILL Plan: Pro Plus Run ID: You can disable this status message by setting the Use the checkbox below for a quick retry:
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 |
|
Closing: the isolation test on the live R4b repro showed these comparators do not fix the evaluations-list loop on their own (the idempotent cache write in #6204 is necessary and sufficient). Kept out of v0.113.0 to keep the release change minimal; the comparator hardening can return as a follow-up on main. |
What
Give the two evaluator-reference
atomFamilys inweb/oss/src/components/References/atoms/entityReferences.tsanareEqualcomparator, plus the three payload-keyed families inweb/oss/src/state/queries/atoms/fetcher.ts.Why
atomFamilykeys its cache by reference unless it is given anareEqual— the lookup is a plainMap.get(param). Both families are keyed by an object literal that callers rebuild at every call site, so theMapnever hits and each read mints a brand-new atom and a brand-new fetch.The neighbouring families in the same file already declare comparators.
workflowArtifactScopedQueryAtomFamilyis read fifteen lines away inside the same function and is safe purely because of it — these two were the outliers, not the design.evaluatorReferenceAtomFamilyreads the query family from inside its own read function, so the miss also feeds back: each generation mounts its own query observer, the observer emits, and the emit invalidates the reader that created it.What this is not
This is not the fix for the React error #185 on the
/evaluationslist page. I opened this investigation on that theory and the measurement disproved it. With the comparator removed, the atom count over 200 ticks is:It settles at two rather than running away, because this query carries
staleTime: 5 * 60_000and neither polls nor refetches on focus, so generation two reads settled cached data and emits nothing further. The family that did run away in #6196 polled while a run was non-terminal, and that is what compounded. Adding arefetchIntervalhere would turn this into that bug.So the defect being fixed is a duplicated atom and a duplicated fetch per call site, and a latent loop. Real, worth fixing, not release-blocking.
Scope note
queriesQueryAtomFamilyis included becauseevaluationRunsQueryOptionsAtomreads it from inside a derived atom on this very page, and is safe today only because that call site happens to pass hoisted module constants — one refactor away from breaking.Tests
Two files, both red without the comparators and green with them:
evaluatorReferenceFamilyKeys.test.ts— asserts the rule: an equal-but-not-identical key returns the same atom, different keys still separate, andnullandundefinednormalise to one entry (useEvaluatorHeaderReferencepassesundefined,ReferenceLabelspassesnull; without normalising, one evaluator gets two cache entries and two fetches).evaluatorReferenceLoop.test.ts— drives the real code path in a jotai store and counts the query atoms one subscribed evaluator column creates. Must be exactly one.Verification run:
pnpm --filter @agenta/oss exec vitest run— 47 files, 449 passed, 1 skipped.Also checked live: brought this branch's frontend up against a real EE API with six seeded runs carrying evaluator references and mixed statuses, and the list page renders and resolves evaluator names correctly, with no console errors.