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 6 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
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