Skip to content

ITEP-163854 - replace react virtuoso - p4 #230

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

Open
wants to merge 1 commit into
base: main
Choose a base branch
from
Open
Show file tree
Hide file tree
Changes from all commits
Commits
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
Original file line number Diff line number Diff line change
Expand Up @@ -78,7 +78,9 @@ export const DatasetListItemDetails = ({
</View>
<Flex direction={'column'} width={'100%'}>
<TooltipTrigger placement={'bottom'}>
<PressableElement UNSAFE_className={classes.itemMediaName}>{name}</PressableElement>
<PressableElement isTruncated UNSAFE_className={classes.itemMediaName}>
{name}
</PressableElement>
<Tooltip>{name}</Tooltip>
</TooltipTrigger>

Expand Down
Original file line number Diff line number Diff line change
@@ -1,16 +1,14 @@
// Copyright (C) 2022-2025 Intel Corporation
// LIMITED EDGE SOFTWARE DISTRIBUTION LICENSE

import { useEffect, useMemo, useRef } from 'react';
import { useMemo } from 'react';

import { Flex } from '@adobe/react-spectrum';
import { Loading } from '@geti/ui';
import { InfiniteData, UseInfiniteQueryResult } from '@tanstack/react-query';
import { isEmpty } from 'lodash-es';
import { VirtuosoGridHandle } from 'react-virtuoso';

import { MediaAdvancedFilterResponse, MediaItem, MediaItemResponse } from '../../../../../core/media/media.interface';
import { usePrevious } from '../../../../../hooks/use-previous/use-previous.hook';
import { MediaItemsList } from '../../../../../shared/components/media-items-list/media-items-list.component';
import { ViewModes } from '../../../../../shared/components/media-view-modes/utils';
import { NotFound } from '../../../../../shared/components/not-found/not-found.component';
Expand Down Expand Up @@ -54,15 +52,11 @@ export const DatasetList = ({
selectedMediaItem,
isInActiveMode = false,
isMediaFilterEmpty = false,
previouslySelectedMediaItem,
shouldShowAnnotationIndicator,
}: DatasetListProps): JSX.Element => {
const ref = useRef<VirtuosoGridHandle | null>(null);

const { hasNextPage, isPending: isMediaItemsLoading, isFetchingNextPage, fetchNextPage, data } = mediaItemsQuery;

const mediaItems = useMemo(() => data?.pages?.flatMap(({ media }) => media) ?? [], [data?.pages]);
const prevViewMode = usePrevious(viewMode);

const loadNextMedia = async () => {
if (isInActiveMode) {
Expand All @@ -77,32 +71,6 @@ export const DatasetList = ({
const groupedMediaItems = useGroupedMediaItems(mediaItems);
const mediaItemIndex = useSelectedMediaItemIndex(mediaItems, selectedMediaItem, isInActiveMode);

useEffect(() => {
let timeoutId: ReturnType<typeof setTimeout> | null = null;
const mediaItemChanged = previouslySelectedMediaItem?.identifier !== selectedMediaItem?.identifier;
const viewModeChanged = prevViewMode !== viewMode;

// We want to automatically scroll to the previously selected media.
// But only if the media item or view mode is different.
if ((mediaItemChanged || viewModeChanged) && ref.current) {
timeoutId = setTimeout(() => {
ref.current?.scrollToIndex({
index: mediaItemIndex,
behavior: 'smooth',
align: 'center',
});
// we don't want to scroll immediately
// in case of changed view mode we have to scroll once view is rendered
}, 500);
}

return () => {
timeoutId && clearTimeout(timeoutId);
};

// eslint-disable-next-line react-hooks/exhaustive-deps
}, [viewMode, selectedMediaItem?.identifier, previouslySelectedMediaItem?.identifier, prevViewMode]);

const allPagesAreEmpty = data?.pages.every((page) => isEmpty(page.media));

const shouldShowNotFound = allPagesAreEmpty && !isMediaItemsLoading && !isMediaFilterEmpty && !isInActiveMode;
Expand Down Expand Up @@ -136,6 +104,7 @@ export const DatasetList = ({
getTextValue={(item) => item.name}
mediaItems={groupedMediaItems}
viewModeSettings={viewModeSettings}
scrollToIndex={mediaItemIndex}
itemContent={(mediaItem) => {
return (
<DatasetItemFactory
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -22,7 +22,7 @@ import {
import { MediaDropBoxHeader } from '../../../../shared/components/media-drop/media-drop-box-header.component';
import { MediaDropBox } from '../../../../shared/components/media-drop/media-drop-box.component';
import { MediaItemsList } from '../../../../shared/components/media-items-list/media-items-list.component';
import { INITIAL_VIEW_MODE } from '../../../../shared/components/media-view-modes/utils';
import { INITIAL_VIEW_MODE, VIEW_MODE_SETTINGS, ViewModes } from '../../../../shared/components/media-view-modes/utils';
import { TutorialCardBuilder } from '../../../../shared/components/tutorial-card/tutorial-card-builder.component';
import { VALID_MEDIA_TYPES_DISPLAY } from '../../../../shared/media-utils';
import { idMatchingFormat } from '../../../../test-utils/id-utils';
Expand Down Expand Up @@ -61,6 +61,11 @@ export interface MediaContentBucketProps {
footerInfo?: ReactNode;
}

const VIEW_MODE_SETTINGS_ANOMALY = {
...VIEW_MODE_SETTINGS,
[ViewModes.LARGE]: { minItemSize: 180, gap: 8, maxColumns: 2 },
};

export const MediaContentBucket = ({
header,
description,
Expand Down Expand Up @@ -200,6 +205,7 @@ export const MediaContentBucket = ({
getTextValue={(item) => item.name}
mediaItems={media}
viewMode={viewMode}
viewModeSettings={isAnomalyProject ? VIEW_MODE_SETTINGS_ANOMALY : VIEW_MODE_SETTINGS}
itemContent={(item) => (
<MediaItemFactory
mediaItem={item}
Expand Down
Original file line number Diff line number Diff line change
@@ -1,13 +1,12 @@
// Copyright (C) 2022-2025 Intel Corporation
// LIMITED EDGE SOFTWARE DISTRIBUTION LICENSE

import { useEffect, useMemo, useRef } from 'react';
import { useMemo } from 'react';

import { Flex, IllustratedMessage, Tooltip, TooltipTrigger, View } from '@adobe/react-spectrum';
import { Loading } from '@geti/ui';
import { InfiniteData, UseInfiniteQueryResult } from '@tanstack/react-query';
import { isEmpty } from 'lodash-es';
import { VirtuosoGridHandle } from 'react-virtuoso';

import { MEDIA_TYPE } from '../../../../core/media/base-media.interface';
import { MediaItem } from '../../../../core/media/media.interface';
Expand Down Expand Up @@ -61,7 +60,6 @@ export const TestMediaItemsList = ({
loadNextMedia,
selectMediaItem,
}: TestMediaItemsListProps): JSX.Element => {
const ref = useRef<VirtuosoGridHandle | null>(null);
const { isLoading: isMediaItemsLoading, isFetchingNextPage, data } = mediaItemsQuery;
const mediaItems = useMemo(() => data?.pages?.flatMap(({ media }) => media) ?? [], [data?.pages]);

Expand All @@ -71,28 +69,6 @@ export const TestMediaItemsList = ({

const mediaItemIndex = useSelectedMediaItemIndex(mediaItems, selectedMediaItem, false, true);

useEffect(() => {
let timeoutId: ReturnType<typeof setTimeout> | null = null;

if (ref.current && selectedMediaItem !== undefined) {
timeoutId = setTimeout(() => {
ref.current?.scrollToIndex({
index: mediaItemIndex,
behavior: 'smooth',
align: 'center',
});
// we don't want to scroll immediately
// in case of changed view mode we have to scroll once view is rendered
}, 500);
}

return () => {
timeoutId && clearTimeout(timeoutId);
};

// eslint-disable-next-line react-hooks/exhaustive-deps
}, [selectedMediaItem]);

if (isMediaItemsLoading) {
return (
<Flex position={'relative'} alignItems={'center'} justifyContent={'center'} height={'100%'}>
Expand Down Expand Up @@ -124,6 +100,7 @@ export const TestMediaItemsList = ({
viewModeSettings={viewModeSettings}
idFormatter={getTestMediaItemId}
getTextValue={(item) => item.media.name}
scrollToIndex={mediaItemIndex}
itemContent={(mediaItem) => {
const mediaImageItem = mediaItem as unknown as TestImageMediaItem;
const handleSelectMediaItem = () => selectMediaItem(mediaItem);
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -16,6 +16,7 @@ import {
} from 'react-aria-components';

import { VIEW_MODE_SETTINGS, ViewModes } from '../media-view-modes/utils';
import { useScrollToTargetItem } from './use-scroll-to-target-item.hook';

import classes from './media-items-list.module.scss';

Expand All @@ -26,6 +27,7 @@ interface MediaItemsListProps<T> {
viewMode: ViewModes;
mediaItems: T[];
height?: Responsive<DimensionValue>;
scrollToIndex?: number;
viewModeSettings?: ViewModeSettings;
endReached?: () => void;
itemContent: (item: T) => ReactNode;
Expand All @@ -38,6 +40,7 @@ export const MediaItemsList = <T extends object>({
height,
viewMode,
mediaItems,
scrollToIndex,
ariaLabel = 'media items list',
viewModeSettings = VIEW_MODE_SETTINGS,
itemContent,
Expand All @@ -49,9 +52,6 @@ export const MediaItemsList = <T extends object>({
const isDetails = viewMode === ViewModes.DETAILS;
const layout = isDetails ? 'stack' : 'grid';

const ref = useRef<HTMLDivElement | null>(null);
useLoadMore({ onLoadMore: endReached }, ref);

const layoutOptions = isDetails
? {
gap: config.gap,
Expand All @@ -64,6 +64,19 @@ export const MediaItemsList = <T extends object>({
preserveAspectRatio: true,
};

const ref = useRef<HTMLDivElement | null>(null);
useLoadMore({ onLoadMore: endReached }, ref);

const container = ref?.current?.firstElementChild;
Copy link
Preview

Copilot AI May 16, 2025

Choose a reason for hiding this comment

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

[nitpick] Consider moving the retrieval of the container element inside the useScrollToTargetItem hook or ensuring that it updates correctly, as accessing ref.current outside of the hook’s effect may lead to stale references.

Copilot uses AI. Check for mistakes.


useScrollToTargetItem({
gap: config.gap,
container,
targetIndex: scrollToIndex,
dependencies: [scrollToIndex, viewMode, container],
callback: (top) => ref.current?.scrollTo({ top, behavior: 'smooth' }),
});

return (
<View id={id} UNSAFE_className={classes.mainContainer} height={height}>
<Virtualizer layout={isDetails ? ListLayout : GridLayout} layoutOptions={layoutOptions}>
Expand Down
Original file line number Diff line number Diff line change
@@ -0,0 +1,101 @@
// Copyright (C) 2022-2025 Intel Corporation
// LIMITED EDGE SOFTWARE DISTRIBUTION LICENSE

import { renderHook } from '@testing-library/react';

import { useScrollToTargetItem } from './use-scroll-to-target-item.hook';

jest.useFakeTimers();
Copy link
Contributor

Choose a reason for hiding this comment

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

Could we do it in the beforeEach? I think we also miss useRealTimers, we might add it to afterEach.


describe('useScrollToTargetItem', () => {
const mockCallback = jest.fn();

beforeEach(() => {
jest.clearAllMocks();
jest.clearAllTimers();
});

it('should not call callback when container is not provided', () => {
renderHook(() =>
useScrollToTargetItem({
gap: 10,
container: null,
targetIndex: 5,
callback: mockCallback,
})
);

jest.advanceTimersByTime(500);

expect(mockCallback).not.toHaveBeenCalled();
});

it('calls callback with correct scroll position', () => {
const gap = 0;
const childWidth = 100;
const childHeight = 100;
const targetIndex = 12;
const containerWidth = 200;

const container = document.createElement('div');
Object.defineProperty(container, 'clientWidth', { value: containerWidth });

const child = document.createElement('div');
Object.defineProperty(child, 'clientWidth', { value: childWidth });
Object.defineProperty(child, 'clientHeight', { value: childHeight });

container.appendChild(child);

const itemsPerRow = Math.floor(containerWidth / childWidth); // 2
const targetRow = Math.floor(targetIndex / itemsPerRow); // 6
const expectedScrollPos = (childHeight + gap) * targetRow; // 600

renderHook(() =>
useScrollToTargetItem({
gap,
container,
targetIndex,
callback: mockCallback,
})
);

jest.advanceTimersByTime(500);

expect(mockCallback).toHaveBeenCalledWith(expectedScrollPos);
});

it('return zero when container has no children', () => {
const container = document.createElement('div');
Object.defineProperty(container, 'clientWidth', { value: 1000 });

renderHook(() =>
useScrollToTargetItem({
gap: 10,
container,
targetIndex: 5,
callback: mockCallback,
})
);

jest.advanceTimersByTime(500);

expect(mockCallback).toHaveBeenCalledWith(0);
});

describe('should not call callback with invalid index', () => {
it.each([undefined, null, -1, 1.5, NaN])('targetIndex: %p', (invalidIndex) => {
renderHook(() =>
useScrollToTargetItem({
gap: 10,
container: document.createElement('div'),
targetIndex: invalidIndex as number,
callback: mockCallback,
})
);

jest.advanceTimersByTime(500);

expect(mockCallback).not.toHaveBeenCalled();
});
});
});
Original file line number Diff line number Diff line change
@@ -0,0 +1,48 @@
// Copyright (C) 2022-2025 Intel Corporation
// LIMITED EDGE SOFTWARE DISTRIBUTION LICENSE

import { DependencyList, useEffect } from 'react';

import { isNil } from 'lodash-es';

interface useScrollToTargetItemProps {
gap: number;
dependencies?: DependencyList;
container?: Element | null;
targetIndex?: number;
callback: (scrollTo: number) => void;
}

const isValidIndex = (index?: number): index is number => !isNil(index) && Number.isInteger(index) && index >= 0;

export const useScrollToTargetItem = ({
gap,
container,
targetIndex,
dependencies = [],
callback,
}: useScrollToTargetItemProps) => {
useEffect(() => {
const timeoutId = setTimeout(() => {
if (!container || !isValidIndex(targetIndex)) {
return;
}

const containerWidth = container.clientWidth;
const childrenWidth = container.firstElementChild?.clientWidth ?? 1;
const childrenHeight = container.firstElementChild?.clientHeight ?? 1;
const childrenPreRow = Math.floor(containerWidth / childrenWidth);
const targetRow = Math.floor(targetIndex / childrenPreRow);
const scrollTo = (childrenHeight + gap) * targetRow;

callback(scrollTo);
// we don't want to scroll immediately
// in case of changed view mode we have to scroll once view is rendered
}, 500);
Copy link
Contributor

Choose a reason for hiding this comment

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

a slow network might break this behavior. Could we not handle this on the parent? Maybe pass a "isReady" or "isEnabled" prop once the parent renders, perhaps inside a useEffect?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The hook re-executes when scrollToIndex changes. Once the items are loaded and the index is set, it responds as expected. I tested it on a 3G connection, and it worked fine


return () => {
timeoutId && clearTimeout(timeoutId);
};
// eslint-disable-next-line react-hooks/exhaustive-deps
}, dependencies);
};
4 changes: 2 additions & 2 deletions web_ui/src/shared/components/media-view-modes/utils.ts
Original file line number Diff line number Diff line change
Expand Up @@ -12,8 +12,8 @@ export const INITIAL_VIEW_MODE = ViewModes.MEDIUM;
export const VIEW_MODE_LABEL = 'View mode';

export const VIEW_MODE_SETTINGS = {
[ViewModes.SMALL]: { minItemSize: 112, gap: 4, maxColumns: 11 },
[ViewModes.LARGE]: { minItemSize: 300, gap: 8, maxColumns: 4 },
Copy link
Contributor

Choose a reason for hiding this comment

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

gap for the LARGE mode was 12 before, is it intentional change?

[ViewModes.MEDIUM]: { minItemSize: 150, gap: 8, maxColumns: 8 },
[ViewModes.LARGE]: { minItemSize: 300, gap: 12, maxColumns: 4 },
[ViewModes.SMALL]: { minItemSize: 112, gap: 4, maxColumns: 11 },
[ViewModes.DETAILS]: { size: 81, gap: 0 },
};
Loading