Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merge release/v1.0.0 to develop #4858

Merged
merged 19 commits into from
Sep 30, 2024
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
19 commits
Select commit Hold shift + click to select a range
b305252
fix url tracking
ritch Sep 27, 2024
1e05d11
Merge branch 'release/v1.0.0' of https://github.com/voxel51/fiftyone …
voxel51-bot Sep 27, 2024
81037fa
Render summary fields in modal sidebar (#4851)
benjaminpkane Sep 27, 2024
f37884d
Merge branch 'release/v1.0.0' of https://github.com/voxel51/fiftyone …
voxel51-bot Sep 27, 2024
d3960f8
fix state scope missing in some panel hooks
imanjra Sep 27, 2024
f5e8a5a
timeline fix: loadrange in imavid (#4857)
sashankaryal Sep 27, 2024
473b8d7
Merge branch 'release/v1.0.0' of https://github.com/voxel51/fiftyone …
voxel51-bot Sep 27, 2024
ba40b0e
don't update dataset doc until field values are populated
brimoor Sep 27, 2024
3e1a9b1
increase zIndex of modal sidebar to fix resize bug (#4860)
imanjra Sep 27, 2024
58c2ae6
Merge branch 'release/v1.0.0' of https://github.com/voxel51/fiftyone …
voxel51-bot Sep 27, 2024
7ebfd94
Merge pull request #4861 from voxel51/robust-migration
brimoor Sep 27, 2024
a3f47e5
Merge branch 'release/v1.0.0' of https://github.com/voxel51/fiftyone …
voxel51-bot Sep 27, 2024
887654f
Merge pull request #4859 from voxel51/bugfix/missing-panel-state-scope
brimoor Sep 27, 2024
9ff80ab
Merge branch 'release/v1.0.0' of https://github.com/voxel51/fiftyone …
voxel51-bot Sep 27, 2024
f5cdc88
Fix dynamic groups carousel bug (#4863)
sashankaryal Sep 27, 2024
be1e074
Merge branch 'release/v1.0.0' of https://github.com/voxel51/fiftyone …
voxel51-bot Sep 27, 2024
a38596d
Small bug fix for Numerical Field in Summary Field Operator
Sep 27, 2024
543ecf3
Merge pull request #4864 from voxel51/bugfix/summary_field_operator
brimoor Sep 28, 2024
ab426cb
Merge branch 'release/v1.0.0' of https://github.com/voxel51/fiftyone …
voxel51-bot Sep 28, 2024
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
4 changes: 2 additions & 2 deletions app/packages/analytics/src/analytics.test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -151,7 +151,7 @@ describe("Analytics", () => {
// segment should be called with context.page.url = undefined
expect(mockSegment.track).toHaveBeenCalledWith("custom_event", undefined, {
context: {
page: { url: undefined },
page: { url: null, path: null, title: null },
},
});
});
Expand Down Expand Up @@ -208,7 +208,7 @@ describe("Analytics", () => {
version: "1.0.0",
});
});

describe("analytics.page()", () => {
it("should call segment.page()", () => {
analytics = new Analytics();
Expand Down
2 changes: 1 addition & 1 deletion app/packages/analytics/src/usingAnalytics.ts
Original file line number Diff line number Diff line change
Expand Up @@ -123,7 +123,7 @@ export class Analytics {
if (!this._segment) return;
let opts;
if (this._disableUrlTracking) {
opts = { context: { page: { url: undefined } } };
opts = { context: { page: { url: null, path: null, title: null } } };
}
if (this._version) {
opts = { ...opts, version: this._version };
Expand Down
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)
*
*/
Comment on lines +20 to +29
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue

Address the TODO and improve the workaround

The comment indicates that the current implementation is a temporary workaround ("BIG HACK") using setInterval to handle re-rendering when groupByField changes. Polling every 50ms can impact performance and isn't an ideal solution. It's important to find a better approach to manage state changes.

Would you like assistance in refactoring this code to eliminate the need for setInterval? I can help explore alternative methods to handle the state updates efficiently.

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);
}, []);
Comment on lines +30 to +48
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🛠️ Refactor suggestion

Avoid using setInterval for state synchronization

Using setInterval to poll for changes in groupByFieldValue is inefficient and may lead to performance issues. Consider subscribing directly to Recoil state changes or using a React Effect that depends on the relevant state.

One potential solution is to use useRecoilTransactionObserver_UNSTABLE to listen for changes in groupByFieldValue without causing unintended side effects. This allows the component to update when the state changes without polling.

import { useRecoilTransactionObserver_UNSTABLE } from "recoil";

useRecoilTransactionObserver_UNSTABLE(({ snapshot }) => {
  const groupByFieldValue = snapshot.getLoadable(fos.groupByFieldValue).getValue();
  if (groupByFieldValue !== groupByValueRef.current) {
    setGroupByValue(groupByFieldValue);
  }
});


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>
);
};
});
112 changes: 76 additions & 36 deletions app/packages/core/src/components/Modal/ImaVidLooker.tsx
Original file line number Diff line number Diff line change
@@ -1,6 +1,5 @@
import { useTheme } from "@fiftyone/components";
import { AbstractLooker, ImaVidLooker } from "@fiftyone/looker";
import { BUFFERING_PAUSE_TIMEOUT } from "@fiftyone/looker/src/lookers/imavid/constants";
import { BaseState } from "@fiftyone/looker/src/state";
import { FoTimelineConfig, useCreateTimeline } from "@fiftyone/playback";
import { useDefaultTimelineNameImperative } from "@fiftyone/playback/src/lib/use-default-timeline-name";
Expand Down Expand Up @@ -54,6 +53,8 @@ export const ImaVidLookerReact = React.memo(
});

const { activeLookerRef, setActiveLookerRef } = useModalContext();
const imaVidLookerRef =
activeLookerRef as unknown as React.MutableRefObject<ImaVidLooker>;
Comment on lines +56 to +57
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🛠️ Refactor suggestion

Simplify type assertion for imaVidLookerRef.

The current type assertion uses as unknown as, which is unnecessarily complex. You can simplify it by directly casting activeLookerRef to the desired type.

Apply this diff to simplify the type assertion:

- const imaVidLookerRef =
-   activeLookerRef as unknown as React.MutableRefObject<ImaVidLooker>;
+ const imaVidLookerRef =
+   activeLookerRef as React.MutableRefObject<ImaVidLooker>;
📝 Committable suggestion

‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.

Suggested change
const imaVidLookerRef =
activeLookerRef as unknown as React.MutableRefObject<ImaVidLooker>;
const imaVidLookerRef =
activeLookerRef as React.MutableRefObject<ImaVidLooker>;


const looker = React.useMemo(
() => createLooker.current(sampleDataWithExtraParams),
Expand Down Expand Up @@ -152,19 +153,59 @@ export const ImaVidLookerReact = React.memo(
);
}, [ref]);

const loadRange = React.useCallback(async (range: BufferRange) => {
// todo: implement
return new Promise<void>((resolve) => {
setTimeout(() => {
resolve();
}, BUFFERING_PAUSE_TIMEOUT);
});
}, []);
const loadRange = React.useCallback(
async (range: Readonly<BufferRange>) => {
const storeBufferManager =
imaVidLookerRef.current.frameStoreController.storeBufferManager;
const fetchBufferManager =
imaVidLookerRef.current.frameStoreController.fetchBufferManager;
Comment on lines +158 to +161
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue

Add null checks for imaVidLookerRef.current to prevent runtime errors.

Accessing imaVidLookerRef.current without ensuring it's defined could lead to runtime errors if current is null or undefined. Consider adding checks or using optional chaining.

Update the code to safely access imaVidLookerRef.current:

  const loadRange = React.useCallback(
    async (range: Readonly<BufferRange>) => {
-     const storeBufferManager =
-       imaVidLookerRef.current.frameStoreController.storeBufferManager;
-     const fetchBufferManager =
-       imaVidLookerRef.current.frameStoreController.fetchBufferManager;
+     if (!imaVidLookerRef.current) {
+       // Handle the case when current is null or undefined
+       return;
+     }
+     const storeBufferManager =
+       imaVidLookerRef.current.frameStoreController.storeBufferManager;
+     const fetchBufferManager =
+       imaVidLookerRef.current.frameStoreController.fetchBufferManager;
      // Rest of the code...
📝 Committable suggestion

‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.

Suggested change
const storeBufferManager =
imaVidLookerRef.current.frameStoreController.storeBufferManager;
const fetchBufferManager =
imaVidLookerRef.current.frameStoreController.fetchBufferManager;
if (!imaVidLookerRef.current) {
// Handle the case when current is null or undefined
return;
}
const storeBufferManager =
imaVidLookerRef.current.frameStoreController.storeBufferManager;
const fetchBufferManager =
imaVidLookerRef.current.frameStoreController.fetchBufferManager;


if (storeBufferManager.containsRange(range)) {
return;
}

const unprocessedStoreBufferRange =
storeBufferManager.getUnprocessedBufferRange(range);
const unprocessedBufferRange =
fetchBufferManager.getUnprocessedBufferRange(
unprocessedStoreBufferRange
);

if (!unprocessedBufferRange) {
return;
}

imaVidLookerRef.current.frameStoreController.enqueueFetch(
unprocessedBufferRange
);

return new Promise<void>((resolve) => {
const fetchMoreListener = (e: CustomEvent) => {
if (
e.detail.id === imaVidLookerRef.current.frameStoreController.key
) {
if (storeBufferManager.containsRange(unprocessedBufferRange)) {
resolve();
window.removeEventListener(
"fetchMore",
fetchMoreListener as EventListener
);
}
}
};

window.addEventListener(
"fetchMore",
fetchMoreListener as EventListener,
{ once: true }
);
});
},
[]
);

const renderFrame = React.useCallback((frameNumber: number) => {
(
activeLookerRef.current as unknown as ImaVidLooker
)?.element.drawFrameNoAnimation(frameNumber);
imaVidLookerRef.current?.element.drawFrameNoAnimation(frameNumber);
}, []);

const { getName } = useDefaultTimelineNameImperative();
Expand All @@ -189,7 +230,7 @@ export const ImaVidLookerReact = React.memo(

const readyWhen = useCallback(async () => {
return new Promise<void>((resolve) => {
// wait for total frame count to be resolved
// hack: wait for total frame count to be resolved
let intervalId;
intervalId = setInterval(() => {
if (totalFrameCountRef.current) {
Expand All @@ -200,6 +241,10 @@ export const ImaVidLookerReact = React.memo(
});
}, []);

const onAnimationStutter = useCallback(() => {
imaVidLookerRef.current?.element.checkFetchBufferManager();
}, []);
Comment on lines +244 to +246
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue

Improve handling of imaVidLookerRef.current in onAnimationStutter.

In the onAnimationStutter callback, consider adding a null check for imaVidLookerRef.current to prevent potential runtime errors.

Update the code to include a null check:

  const onAnimationStutter = useCallback(() => {
-   imaVidLookerRef.current?.element.checkFetchBufferManager();
+   imaVidLookerRef.current?.element?.checkFetchBufferManager();
  }, []);

This ensures that both current and element are defined before calling checkFetchBufferManager().

📝 Committable suggestion

‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.

Suggested change
const onAnimationStutter = useCallback(() => {
imaVidLookerRef.current?.element.checkFetchBufferManager();
}, []);
const onAnimationStutter = useCallback(() => {
imaVidLookerRef.current?.element?.checkFetchBufferManager();
}, []);


const {
isTimelineInitialized,
registerOnPauseCallback,
Expand All @@ -210,6 +255,10 @@ export const ImaVidLookerReact = React.memo(
name: timelineName,
config: timelineCreationConfig,
waitUntilInitialized: readyWhen,
// using this mechanism to resume fetch if it was paused
// ideally we have control of fetch in this component but can't do that yet
// since imavid is part of the grid too
onAnimationStutter,
});

/**
Expand All @@ -224,35 +273,27 @@ export const ImaVidLookerReact = React.memo(
});

registerOnPlayCallback(() => {
(activeLookerRef.current as unknown as ImaVidLooker).element.update(
() => ({
playing: true,
})
);
imaVidLookerRef.current?.element?.update(() => ({
playing: true,
}));
Comment on lines +276 to +278
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue

Ensure consistent null checks when accessing imaVidLookerRef.current.

In some instances, optional chaining is used (e.g., imaVidLookerRef.current?.element), but in other places, it's omitted. For consistency and to prevent potential runtime errors, consistently apply null checks when accessing imaVidLookerRef.current.

Review and update the following code segments:

  registerOnPlayCallback(() => {
-   imaVidLookerRef.current?.element?.update(() => ({
+   imaVidLookerRef.current?.element?.update(() => ({
      playing: true,
    }));
  });

  registerOnPauseCallback(() => {
-   imaVidLookerRef.current?.element?.update(() => ({
+   imaVidLookerRef.current?.element?.update(() => ({
      playing: false,
    }));
  });

  registerOnSeekCallbacks({
    start: () => {
-     imaVidLookerRef.current?.element?.update(() => ({
+     imaVidLookerRef.current?.element?.update(() => ({
        seeking: true,
      }));
    },
    end: () => {
-     imaVidLookerRef.current?.element?.update(() => ({
+     imaVidLookerRef.current?.element?.update(() => ({
        seeking: false,
      }));
    },
  });

Ensure that all accesses to imaVidLookerRef.current use optional chaining or appropriate null checks.

Also applies to: 282-284, 289-296

});

registerOnPauseCallback(() => {
(activeLookerRef.current as unknown as ImaVidLooker).element.update(
() => ({
playing: false,
})
);
imaVidLookerRef.current?.element?.update(() => ({
playing: false,
}));
});

registerOnSeekCallbacks({
start: () => {
(activeLookerRef.current as unknown as ImaVidLooker).element.update(
() => ({
seeking: true,
})
);
imaVidLookerRef.current?.element?.update(() => ({
seeking: true,
}));
},
end: () => {
(activeLookerRef.current as unknown as ImaVidLooker).element.update(
() => ({
seeking: false,
})
);
imaVidLookerRef.current?.element?.update(() => ({
seeking: false,
}));
},
});
}
Expand All @@ -265,9 +306,8 @@ export const ImaVidLookerReact = React.memo(
// hack: poll every 10ms for total frame count
// replace with event listener or callback
let intervalId = setInterval(() => {
const totalFrameCount = (
activeLookerRef.current as unknown as ImaVidLooker
).frameStoreController.totalFrameCount;
const totalFrameCount =
imaVidLookerRef.current.frameStoreController.totalFrameCount;
if (totalFrameCount) {
setTotalFrameCount(totalFrameCount);
clearInterval(intervalId);
Expand Down
Loading
Loading