Skip to content

Commit fb17f8e

Browse files
Nimrod007claude
andcommitted
[OPIK-8005] [FE] fix: scope hydration cancellation to the effect run (Baz review)
Baz caught a real bug in the previous commit, and it is worth being precise that the new early-return path made a latent flaw dangerous rather than merely inelegant. cancelledRef is a single ref shared across every effect run, and the effect body reset it to false on entry. React runs the previous run's cleanup before the next body, so the sequence was: run 1 starts hydrating dataset A and installs cleanup -> dataset changes -> cleanup sets cancelled = true -> run 2 enters and resets it to false, un-cancelling run 1's still-pending loop -> run 1's continuation passes its post-await check and calls setHydratedItems on the new array. Before this PR the only early return sat after setHydratedItems([]), so that stale continuation's prev.map() ran over an empty array and produced nothing — harmless. The no-media early return added in the previous commit sits after setHydratedItems(datasetItems) with a populated array, so the same continuation writes dataset A's data into dataset B's array at a positional index. That is silent cross-dataset corruption, reachable by switching dataset or page size while a media-bearing dataset is still hydrating. Replace the shared ref with a `cancelled` flag scoped to each effect run. Each run observes only its own flag, so early-returning without installing cleanup cannot resurrect an older run's loop. This also fixes the pre-existing empty- dataset path and the setState-after-unmount case, and removes the useRef import. Verified: tsc, eslint --max-warnings=0, prettier, and the full vitest suite (150 files, 2229 tests) all pass. Refs: OPIK-8005, CUST_6878, AI_546 Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
1 parent e1832ee commit fb17f8e

1 file changed

Lines changed: 12 additions & 6 deletions

File tree

apps/opik-frontend/src/v2/pages/PlaygroundPage/useIncrementalDatasetHydration.ts

Lines changed: 12 additions & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -1,4 +1,4 @@
1-
import { useEffect, useRef, useState } from "react";
1+
import { useEffect, useState } from "react";
22
import { DatasetItem } from "@/types/datasets";
33
import { useHydrateDatasetItemData } from "@/v2/pages/PlaygroundPage/useHydrateDatasetItemData";
44
import { containsTruncatedMedia } from "@/lib/media";
@@ -10,10 +10,16 @@ export function useIncrementalDatasetHydration(datasetItems: DatasetItem[]): {
1010
const hydrateDatasetItemData = useHydrateDatasetItemData();
1111
const [hydratedItems, setHydratedItems] = useState<DatasetItem[]>([]);
1212
const [isHydrating, setIsHydrating] = useState(false);
13-
const cancelledRef = useRef(false);
1413

1514
useEffect(() => {
16-
cancelledRef.current = false;
15+
// Scoped to this effect run rather than a shared ref. React runs the previous
16+
// run's cleanup before this body, so an in-flight hydration from an earlier
17+
// dataset observes its own `cancelled === true` and stops — even when this run
18+
// returns early below and installs no cleanup of its own. A shared ref instead
19+
// got reset here on every run, un-cancelling the previous run's loop; its
20+
// continuation would then write the old dataset's data into the new array at a
21+
// positional index. Also covers unmount.
22+
let cancelled = false;
1723

1824
if (datasetItems.length === 0) {
1925
setHydratedItems([]);
@@ -48,11 +54,11 @@ export function useIncrementalDatasetHydration(datasetItems: DatasetItem[]): {
4854

4955
const hydrateItems = async () => {
5056
for (const index of indexesToHydrate) {
51-
if (cancelledRef.current) return;
57+
if (cancelled) return;
5258

5359
const hydratedData = await hydrateDatasetItemData(datasetItems[index]);
5460

55-
if (cancelledRef.current) return;
61+
if (cancelled) return;
5662

5763
setHydratedItems((prev) =>
5864
prev.map((item, idx) =>
@@ -67,7 +73,7 @@ export function useIncrementalDatasetHydration(datasetItems: DatasetItem[]): {
6773
hydrateItems();
6874

6975
return () => {
70-
cancelledRef.current = true;
76+
cancelled = true;
7177
};
7278
}, [datasetItems, hydrateDatasetItemData]);
7379

0 commit comments

Comments
 (0)