Skip to content

Commit

Permalink
fix: useViewportData - memoize subscriptions and first row of viewport (
Browse files Browse the repository at this point in the history
#2008)

fixes #2003 - useViewportData - re-subscribes to table any time
ViewportData changes
- Memoized `dh.Table.EVENT_SIZECHANGED` handler
- Memoized `dh.Table.EVENT_UPDATED` handler

fixes #1928 - useViewportData - setViewport should maintain last
firstRow value when table size changes
- Track last set "first row" to be re-used when table size changes but
table reference remains the same

**Tested**
- Ticking table 
- table update subscriptions no longer occur on each viewportData change
   - size change handler no longer re-subscribes on every render
- ACL Editor - scrolling viewport data still works. Filtering still
properly resets viewport to zero
  • Loading branch information
bmingles committed May 14, 2024
1 parent 7d59d09 commit 2246a4a
Show file tree
Hide file tree
Showing 3 changed files with 31 additions and 9 deletions.
6 changes: 3 additions & 3 deletions packages/jsapi-components/src/useTableSize.test.ts
Original file line number Diff line number Diff line change
@@ -1,6 +1,6 @@
import { act, renderHook } from '@testing-library/react-hooks';
import dh from '@deephaven/jsapi-shim';
import type { Table } from '@deephaven/jsapi-types';
import type { dh as DhType } from '@deephaven/jsapi-types';
import { TestUtils } from '@deephaven/utils';
import useTableSize from './useTableSize';
import useTableListener from './useTableListener';
Expand All @@ -21,7 +21,7 @@ it.each([null, undefined])('should return 0 if no table', table => {

it('should return the size of the given table', () => {
const size = 10;
const table = TestUtils.createMockProxy<Table>({ size });
const table = TestUtils.createMockProxy<DhType.Table>({ size });

const { result } = renderHook(() => useTableSize(table), { wrapper });

Expand All @@ -33,7 +33,7 @@ it('should re-render if dh.Table.EVENT_SIZECHANGED event occurs', () => {
const table = {
addEventListener: jest.fn(),
size: initialSize,
} as unknown as Table;
} as unknown as DhType.Table;

const { result } = renderHook(() => useTableSize(table), { wrapper });

Expand Down
8 changes: 5 additions & 3 deletions packages/jsapi-components/src/useTableSize.ts
Original file line number Diff line number Diff line change
@@ -1,4 +1,4 @@
import { useState } from 'react';
import { useCallback, useState } from 'react';
import { useApi } from '@deephaven/jsapi-bootstrap';
import type { dh } from '@deephaven/jsapi-types';
import { getSize } from '@deephaven/jsapi-utils';
Expand All @@ -18,9 +18,11 @@ export function useTableSize(

const dh = useApi();

useTableListener(table, dh.Table.EVENT_SIZECHANGED, () => {
const onSizeChanged = useCallback(() => {
forceRerender(i => i + 1);
});
}, []);

useTableListener(table, dh.Table.EVENT_SIZECHANGED, onSizeChanged);

return getSize(table);
}
Expand Down
26 changes: 23 additions & 3 deletions packages/jsapi-components/src/useViewportData.ts
Original file line number Diff line number Diff line change
@@ -1,10 +1,11 @@
import { useCallback, useEffect, useMemo } from 'react';
import { useCallback, useEffect, useMemo, useRef } from 'react';
import type { dh } from '@deephaven/jsapi-types';
import {
RowDeserializer,
defaultRowDeserializer,
isClosed,
createOnTableUpdatedHandler,
OnTableUpdatedEvent,
} from '@deephaven/jsapi-utils';
import Log from '@deephaven/log';
import { useApi } from '@deephaven/jsapi-bootstrap';
Expand Down Expand Up @@ -78,6 +79,8 @@ export function useViewportData<TItem, TTable extends dh.Table | dh.TreeTable>({
deserializeRow = defaultRowDeserializer,
reuseItemsOnTableResize = false,
}: UseViewportDataProps<TItem, TTable>): UseViewportDataResult<TItem, TTable> {
const currentViewportFirstRowRef = useRef<number>(0);

const viewportData = useInitializeViewportData<TItem>(
table,
reuseItemsOnTableResize
Expand All @@ -91,6 +94,8 @@ export function useViewportData<TItem, TTable extends dh.Table | dh.TreeTable>({

const setViewport = useCallback(
(firstRow: number) => {
currentViewportFirstRowRef.current = firstRow;

if (table && !isClosed(table)) {
setPaddedViewport(firstRow);
} else {
Expand All @@ -114,20 +119,35 @@ export function useViewportData<TItem, TTable extends dh.Table | dh.TreeTable>({

const dh = useApi();

const onTableUpdated = useMemo(
// Store the memoized callback in a ref so that changes to `viewportData`
// don't invalidate the memoization of `onTableUpdated`. This prevents
// `useTableListener` from unnecessarily re-subscribing to the same event over
// and over.
const onTableUpdatedRef = useRef<(event: OnTableUpdatedEvent) => void>();
onTableUpdatedRef.current = useMemo(
() => createOnTableUpdatedHandler(viewportData, deserializeRow),
[deserializeRow, viewportData]
);

const onTableUpdated = useCallback((event: OnTableUpdatedEvent) => {
onTableUpdatedRef.current?.(event);
}, []);

useTableListener(table, dh.Table.EVENT_UPDATED, onTableUpdated);

const size = useTableSize(table);

useEffect(() => {
log.debug('Initializing viewport');

// Hydrate the viewport with real data. This will fetch data from index
// 0 to the end of the viewport + padding.
setViewport(0);
}, [table, setViewport, size]);
}, [table, setViewport]);

useEffect(() => {
setViewport(currentViewportFirstRowRef.current);
}, [setViewport, size]);

const onScroll = useOnScrollOffsetChangeCallback(
itemHeight,
Expand Down

0 comments on commit 2246a4a

Please sign in to comment.