Skip to content

Commit

Permalink
Fix dynamic groups carousel bug (#4863)
Browse files Browse the repository at this point in the history
* fix flashlight rerendering bug in dynamic groups

* remove log
  • Loading branch information
sashankaryal authored Sep 27, 2024
1 parent 887654f commit f5cdc88
Show file tree
Hide file tree
Showing 2 changed files with 38 additions and 8 deletions.
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand All @@ -17,6 +17,36 @@ 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);
}, []);

return (
<Resizable
size={{ height, width: "100%" }}
Expand All @@ -41,7 +71,7 @@ export const DynamicGroupCarousel = () => {
}}
data-cy={"group-carousel"}
>
<DynamicGroupsFlashlightWrapper />
<DynamicGroupsFlashlightWrapper key={groupByValue} />
</Resizable>
);
};
});
Original file line number Diff line number Diff line change
Expand Up @@ -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,
Expand Down Expand Up @@ -46,7 +46,7 @@ const pageParams = selector({
},
});

export const DynamicGroupsFlashlightWrapper = () => {
export const DynamicGroupsFlashlightWrapper = React.memo(() => {
const id = useId();

const store = fos.useLookerStore();
Expand Down Expand Up @@ -175,4 +175,4 @@ export const DynamicGroupsFlashlightWrapper = () => {
id={id}
></div>
);
};
});

0 comments on commit f5cdc88

Please sign in to comment.