-
Notifications
You must be signed in to change notification settings - Fork 33
fix: Typing in notebooks is laggy #1977
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
Changes from 6 commits
d042c3c
2c57223
c318d92
af19103
6e303eb
f7e61d6
dfd35ca
1739edb
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -14,7 +14,7 @@ import type { | |
| ReactComponentConfig, | ||
| } from '@deephaven/golden-layout'; | ||
| import Log from '@deephaven/log'; | ||
| import { usePrevious } from '@deephaven/react-hooks'; | ||
| import { usePrevious, useThrottledCallback } from '@deephaven/react-hooks'; | ||
| import { RootState } from '@deephaven/redux'; | ||
| import { useDispatch, useSelector } from 'react-redux'; | ||
| import PanelManager, { ClosedPanels } from './PanelManager'; | ||
|
|
@@ -46,6 +46,8 @@ const DEFAULT_LAYOUT_CONFIG: DashboardLayoutConfig = []; | |
|
|
||
| const DEFAULT_CALLBACK = (): void => undefined; | ||
|
|
||
| const STATE_CHANGE_DEBOUNCE_MS = 1000; | ||
|
|
||
| // If a component isn't registered, just pass through the props so they are saved if a plugin is loaded later | ||
| const FALLBACK_CALLBACK = (props: unknown): unknown => props; | ||
|
|
||
|
|
@@ -195,6 +197,40 @@ export function DashboardLayout({ | |
| ] | ||
| ); | ||
|
|
||
| // eslint-disable-next-line react-hooks/exhaustive-deps | ||
|
mofojed marked this conversation as resolved.
Outdated
|
||
| const processDehydratedLayoutConfig = useCallback( | ||
| dehydratedLayoutConfig => { | ||
| const hasChanged = | ||
| lastConfig == null || | ||
| !LayoutUtils.isEqual(lastConfig, dehydratedLayoutConfig); | ||
|
|
||
| log.debug('handleLayoutStateChanged', hasChanged, dehydratedLayoutConfig); | ||
|
|
||
| if (hasChanged) { | ||
| setIsDashboardEmpty(layout.root.contentItems.length === 0); | ||
|
|
||
| setLastConfig(dehydratedLayoutConfig); | ||
|
|
||
| onLayoutChange(dehydratedLayoutConfig); | ||
|
|
||
| setLayoutChildren(layout.getReactChildren()); | ||
| } | ||
| }, | ||
| [lastConfig, layout, onLayoutChange] | ||
| ); | ||
|
|
||
| // Throttle the calls so that we don't flood comparing these layouts | ||
| const throttledProcessDehydratedLayoutConfig = useThrottledCallback( | ||
| processDehydratedLayoutConfig, | ||
| STATE_CHANGE_DEBOUNCE_MS, | ||
| { flushOnUnmount: true } | ||
| ); | ||
|
|
||
| useEffect( | ||
| () => () => throttledProcessDehydratedLayoutConfig.flush(), | ||
| [throttledProcessDehydratedLayoutConfig] | ||
| ); | ||
|
Comment on lines
+223
to
+226
Collaborator
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Should this be part of the hook instead?
Member
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Moved it to the hook. |
||
|
|
||
| const handleLayoutStateChanged = useCallback(() => { | ||
| // we don't want to emit stateChanges that happen during item drags or else | ||
| // we risk the last saved state being one without that panel in the layout entirely | ||
|
|
@@ -206,27 +242,13 @@ export function DashboardLayout({ | |
| contentConfig, | ||
| dehydrateComponent | ||
| ); | ||
| const hasChanged = | ||
| lastConfig == null || | ||
| !LayoutUtils.isEqual(lastConfig, dehydratedLayoutConfig); | ||
|
|
||
| log.debug( | ||
| 'handleLayoutStateChanged', | ||
| hasChanged, | ||
| contentConfig, | ||
| dehydratedLayoutConfig | ||
| ); | ||
|
|
||
| if (hasChanged) { | ||
| setIsDashboardEmpty(layout.root.contentItems.length === 0); | ||
|
|
||
| setLastConfig(dehydratedLayoutConfig); | ||
|
|
||
| onLayoutChange(dehydratedLayoutConfig); | ||
|
|
||
| setLayoutChildren(layout.getReactChildren()); | ||
| } | ||
| }, [dehydrateComponent, isItemDragging, lastConfig, layout, onLayoutChange]); | ||
| throttledProcessDehydratedLayoutConfig(dehydratedLayoutConfig); | ||
| }, [ | ||
| dehydrateComponent, | ||
| isItemDragging, | ||
| layout, | ||
| throttledProcessDehydratedLayoutConfig, | ||
| ]); | ||
|
|
||
| const handleLayoutItemPickedUp = useCallback( | ||
| (component: Container) => { | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,115 @@ | ||
| import { renderHook } from '@testing-library/react-hooks'; | ||
| import useThrottledCallback from './useThrottledCallback'; | ||
|
|
||
| const callback = jest.fn((text: string) => undefined); | ||
| const arg = 'mock.arg'; | ||
| const arg2 = 'mock.arg2'; | ||
| const throttleMs = 400; | ||
|
|
||
| beforeEach(() => { | ||
| jest.clearAllMocks(); | ||
| jest.useFakeTimers(); | ||
| }); | ||
|
mofojed marked this conversation as resolved.
|
||
|
|
||
| it('should throttle a given callback', () => { | ||
| const { result } = renderHook(() => | ||
| useThrottledCallback(callback, throttleMs) | ||
| ); | ||
|
|
||
| result.current(arg); | ||
| result.current(arg); | ||
|
|
||
| jest.advanceTimersByTime(5); | ||
|
|
||
| result.current(arg); | ||
|
|
||
| result.current(arg2); | ||
|
|
||
| expect(callback).toHaveBeenCalledTimes(1); | ||
| expect(callback).toHaveBeenCalledWith(arg); | ||
|
|
||
| jest.clearAllMocks(); | ||
|
|
||
| jest.advanceTimersByTime(throttleMs); | ||
| expect(callback).toHaveBeenCalledTimes(1); | ||
| expect(callback).toHaveBeenCalledWith(arg2); | ||
| }); | ||
|
|
||
| it('should cancel throttle if component unmounts', () => { | ||
| const { result, unmount } = renderHook(() => | ||
| useThrottledCallback(callback, throttleMs) | ||
| ); | ||
|
|
||
| result.current(arg); | ||
| result.current(arg2); | ||
|
|
||
| jest.advanceTimersByTime(throttleMs - 1); | ||
|
|
||
| expect(callback).toHaveBeenCalledTimes(1); | ||
| expect(callback).toHaveBeenCalledWith(arg); | ||
| callback.mockClear(); | ||
|
|
||
| jest.spyOn(result.current, 'cancel'); | ||
|
|
||
| unmount(); | ||
|
|
||
| expect(result.current.cancel).toHaveBeenCalled(); | ||
|
|
||
| jest.advanceTimersByTime(5); | ||
|
|
||
| expect(callback).not.toHaveBeenCalled(); | ||
| }); | ||
|
|
||
| it('should call the updated callback if the ref changes', () => { | ||
| const { rerender, result } = renderHook( | ||
| fn => useThrottledCallback(fn, throttleMs), | ||
| { | ||
| initialProps: callback, | ||
| } | ||
| ); | ||
|
|
||
| result.current(arg); | ||
| result.current(arg2); | ||
|
|
||
| // Leading is always called | ||
| expect(callback).toHaveBeenCalledTimes(1); | ||
| expect(callback).toHaveBeenCalledWith(arg); | ||
| callback.mockClear(); | ||
|
|
||
| jest.advanceTimersByTime(throttleMs - 1); | ||
|
|
||
| const newCallback = jest.fn(); | ||
| rerender(newCallback); | ||
|
|
||
| jest.advanceTimersByTime(1); | ||
|
|
||
| expect(callback).not.toHaveBeenCalled(); | ||
| expect(newCallback).toHaveBeenCalledTimes(1); | ||
| expect(newCallback).toHaveBeenCalledWith(arg2); | ||
| }); | ||
|
|
||
| it('should flush on unmount if that option is set', () => { | ||
| const { result, unmount } = renderHook(() => | ||
| useThrottledCallback(callback, throttleMs, { flushOnUnmount: true }) | ||
| ); | ||
|
|
||
| result.current(arg); | ||
| result.current(arg2); | ||
|
|
||
| jest.advanceTimersByTime(throttleMs - 1); | ||
|
|
||
| expect(callback).toHaveBeenCalledTimes(1); | ||
| expect(callback).toHaveBeenCalledWith(arg); | ||
| callback.mockClear(); | ||
|
|
||
| jest.spyOn(result.current, 'flush'); | ||
|
|
||
| unmount(); | ||
|
|
||
| expect(result.current.flush).toHaveBeenCalled(); | ||
|
|
||
| jest.advanceTimersByTime(1); | ||
|
|
||
| expect(callback).toHaveBeenCalledTimes(1); | ||
| expect(callback).toHaveBeenCalledWith(arg2); | ||
| }); | ||
| Original file line number | Diff line number | Diff line change | ||||
|---|---|---|---|---|---|---|
| @@ -0,0 +1,51 @@ | ||||||
| import { useEffect, useMemo, useRef } from 'react'; | ||||||
| import type { DebouncedFunc, ThrottleSettings } from 'lodash'; | ||||||
| import throttle from 'lodash.throttle'; | ||||||
|
|
||||||
| /** | ||||||
| * Wraps a given callback in a cancelable, throttled function. The throttled | ||||||
| * callback is automatically cancelled if the callback reference changes or the | ||||||
|
Collaborator
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Something like this? Since
Suggested change
Member
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. So I don't think we want to actually flush or cancel when the callback reference changes, we only want to cancel or flush on unmount, and we want the throttled callback to always call the latest callback reference.
Member
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Also of note the |
||||||
| * component unmounts. | ||||||
| * @param callback callback function to throttle | ||||||
| * @param throttleMs throttle milliseconds | ||||||
| * @param options lodash throttle options. Will not react to changes to this param | ||||||
|
mofojed marked this conversation as resolved.
Outdated
|
||||||
| * @returns a cancelable, throttled function | ||||||
| */ | ||||||
| export function useThrottledCallback<TArgs extends unknown[], TResult>( | ||||||
| callback: (...args: TArgs) => TResult, | ||||||
| throttleMs: number, | ||||||
| initialOptions?: ThrottleSettings & { flushOnUnmount?: boolean } | ||||||
| ): DebouncedFunc<(...args: TArgs) => TResult> { | ||||||
| const options = useRef(initialOptions); | ||||||
|
|
||||||
| // Use a ref for the callback | ||||||
| // We want to keep a stable callback so the flush/cancel works as expected | ||||||
| // So we keep a ref to the current callback, then we have a throttled callback that will just call this | ||||||
| const callbackRef = useRef(callback); | ||||||
| callbackRef.current = callback; | ||||||
|
|
||||||
| const throttledCallback = useMemo( | ||||||
| () => | ||||||
| throttle( | ||||||
| (...args: TArgs) => callbackRef.current(...args), | ||||||
| throttleMs, | ||||||
| options.current | ||||||
| ), | ||||||
| [throttleMs] | ||||||
| ); | ||||||
|
|
||||||
| useEffect( | ||||||
| () => () => { | ||||||
| if (options.current?.flushOnUnmount ?? false) { | ||||||
|
mofojed marked this conversation as resolved.
|
||||||
| throttledCallback.flush(); | ||||||
| } else { | ||||||
| throttledCallback.cancel(); | ||||||
| } | ||||||
| }, | ||||||
| [throttledCallback] | ||||||
| ); | ||||||
|
|
||||||
| return throttledCallback; | ||||||
| } | ||||||
|
|
||||||
| export default useThrottledCallback; | ||||||
Uh oh!
There was an error while loading. Please reload this page.