From e74fe84dd31e6fe176477334e266a84d8c3e4d66 Mon Sep 17 00:00:00 2001 From: Sashank Aryal Date: Fri, 27 Sep 2024 18:01:19 -0500 Subject: [PATCH 1/2] fix flashlight rerendering bug in dynamic groups --- .../carousel/DynamicGroupCarousel.tsx | 41 ++++++++++++++++--- .../DynamicGroupsFlashlightWrapper.tsx | 6 +-- 2 files changed, 39 insertions(+), 8 deletions(-) diff --git a/app/packages/core/src/components/Modal/Group/DynamicGroup/carousel/DynamicGroupCarousel.tsx b/app/packages/core/src/components/Modal/Group/DynamicGroup/carousel/DynamicGroupCarousel.tsx index 1583ad26c1..283fb2185e 100644 --- a/app/packages/core/src/components/Modal/Group/DynamicGroup/carousel/DynamicGroupCarousel.tsx +++ b/app/packages/core/src/components/Modal/Group/DynamicGroup/carousel/DynamicGroupCarousel.tsx @@ -2,13 +2,13 @@ import { useTheme } from "@fiftyone/components"; import * as fos from "@fiftyone/state"; import { useBrowserStorage } from "@fiftyone/state"; import { Resizable } from "re-resizable"; -import React from "react"; -import { useRecoilValue } from "recoil"; +import React, { useEffect, useState } from "react"; +import { useRecoilCallback, useRecoilValue } from "recoil"; import { DynamicGroupsFlashlightWrapper } from "./DynamicGroupsFlashlightWrapper"; const MAX_CAROUSEL_HEIGHT = 600; -export const DynamicGroupCarousel = () => { +export const DynamicGroupCarousel = React.memo(() => { const [height, setHeight] = useBrowserStorage( "dynamic-group-carousel-height", 150 @@ -17,6 +17,37 @@ export const DynamicGroupCarousel = () => { const theme = useTheme(); const isMainVisible = useRecoilValue(fos.groupMediaIsMainVisibleSetting); + /** + * BIG HACK: TODO: FIX ME + * + * Problem = flashlight is not re-rendering when group by field changes. + * Solution was to key it by groupByValue, but when the component + * subscribes to the groupByFieldValue using useRecoilValue(fos.groupByFieldValue), + * while it solves the problem,it causes flashlight to behave weirdly. + * (try scrolling carousel and selecting samples, flashlight will reset to the front) + * + */ + const getGroupByFieldValue = useRecoilCallback(({ snapshot }) => () => { + const groupByField = snapshot.getLoadable(fos.groupByFieldValue).getValue(); + return groupByField; + }); + + const [groupByValue, setGroupByValue] = useState(getGroupByFieldValue()); + const groupByValueRef = React.useRef(groupByValue); + groupByValueRef.current = groupByValue; + + useEffect(() => { + const intervalId = window.setInterval(() => { + const groupByFieldValue = getGroupByFieldValue(); + if (groupByFieldValue !== groupByValueRef.current) { + setGroupByValue(groupByFieldValue); + } + }, 50); + + return () => window.clearInterval(intervalId); + }, []); + + console.log("Group by value is ", groupByValueRef.current); return ( { }} data-cy={"group-carousel"} > - + ); -}; +}); diff --git a/app/packages/core/src/components/Modal/Group/DynamicGroup/carousel/DynamicGroupsFlashlightWrapper.tsx b/app/packages/core/src/components/Modal/Group/DynamicGroup/carousel/DynamicGroupsFlashlightWrapper.tsx index 69950952c0..a27625d224 100644 --- a/app/packages/core/src/components/Modal/Group/DynamicGroup/carousel/DynamicGroupsFlashlightWrapper.tsx +++ b/app/packages/core/src/components/Modal/Group/DynamicGroup/carousel/DynamicGroupsFlashlightWrapper.tsx @@ -3,7 +3,7 @@ import { Sample, freeVideos } from "@fiftyone/looker"; import * as fos from "@fiftyone/state"; import { selectedSamples } from "@fiftyone/state"; import { get } from "lodash"; -import { +import React, { useCallback, useEffect, useId, @@ -46,7 +46,7 @@ const pageParams = selector({ }, }); -export const DynamicGroupsFlashlightWrapper = () => { +export const DynamicGroupsFlashlightWrapper = React.memo(() => { const id = useId(); const store = fos.useLookerStore(); @@ -175,4 +175,4 @@ export const DynamicGroupsFlashlightWrapper = () => { id={id} > ); -}; +}); From 4176f18a874aab1945351b48261f9edd3883df5c Mon Sep 17 00:00:00 2001 From: Sashank Aryal Date: Fri, 27 Sep 2024 18:04:42 -0500 Subject: [PATCH 2/2] remove log --- .../Modal/Group/DynamicGroup/carousel/DynamicGroupCarousel.tsx | 3 +-- 1 file changed, 1 insertion(+), 2 deletions(-) diff --git a/app/packages/core/src/components/Modal/Group/DynamicGroup/carousel/DynamicGroupCarousel.tsx b/app/packages/core/src/components/Modal/Group/DynamicGroup/carousel/DynamicGroupCarousel.tsx index 283fb2185e..b4dac56971 100644 --- a/app/packages/core/src/components/Modal/Group/DynamicGroup/carousel/DynamicGroupCarousel.tsx +++ b/app/packages/core/src/components/Modal/Group/DynamicGroup/carousel/DynamicGroupCarousel.tsx @@ -23,7 +23,7 @@ export const DynamicGroupCarousel = React.memo(() => { * Problem = flashlight is not re-rendering when group by field changes. * Solution was to key it by groupByValue, but when the component * subscribes to the groupByFieldValue using useRecoilValue(fos.groupByFieldValue), - * while it solves the problem,it causes flashlight to behave weirdly. + * while it solves the problem, it causes flashlight to behave weirdly. * (try scrolling carousel and selecting samples, flashlight will reset to the front) * */ @@ -47,7 +47,6 @@ export const DynamicGroupCarousel = React.memo(() => { return () => window.clearInterval(intervalId); }, []); - console.log("Group by value is ", groupByValueRef.current); return (