Skip to content

useResizeableDrawer: remove size state storage #88170

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.

Sign up for GitHub

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 5 commits into
base: master
Choose a base branch
from
Open

useResizeableDrawer: remove size state storage #88170

Show file tree
Hide file tree
Changes from 2 commits
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
b450dbe
useResizeableDrawer: remove size state storage
JonasBa Mar 27, 2025
1d79896
useResizeableDrawer: remove size state storage
JonasBa Mar 27, 2025
82380c1
useResizeableDrawer: remove setSize (#88172)
JonasBa Apr 8, 2025
5055fef
Merge branch 'master' into jb/resizeabledrawe/size-management
JonasBa May 22, 2025
f54a92f
Merge branch 'master' into jb/resizeabledrawe/size-management
JonasBa May 23, 2025
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
16 changes: 5 additions & 11 deletions static/app/components/events/interfaces/breadcrumbs/breadcrumbs.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,5 +1,5 @@
import type React from 'react';
import {Fragment, useCallback, useEffect, useMemo, useRef} from 'react';
import {Fragment, useCallback, useEffect, useMemo, useRef, useState} from 'react';
import type {ListProps} from 'react-virtualized';
import {AutoSizer, CellMeasurer, CellMeasurerCache, List} from 'react-virtualized';
import type {ListRowRenderer} from 'react-virtualized/dist/es/List';
Expand Down Expand Up @@ -29,8 +29,6 @@ import {Breadcrumb} from './breadcrumb';
const PANEL_MIN_HEIGHT = 200;
export const PANEL_INITIAL_HEIGHT = 400;

const noop = () => void 0;

const cache = new CellMeasurerCache({
fixedWidth: true,
defaultHeight: 38,
Expand Down Expand Up @@ -158,14 +156,10 @@ function Breadcrumbs({
}
}, []);

const {
size: containerSize,
isHeld,
onMouseDown,
onDoubleClick,
} = useResizableDrawer({
const [size, setSize] = useState(PANEL_INITIAL_HEIGHT);
const {isHeld, onMouseDown, onDoubleClick} = useResizableDrawer({
direction: 'down',
onResize: noop,
onResize: s => setSize(s),
initialSize: PANEL_INITIAL_HEIGHT,
min: PANEL_MIN_HEIGHT,
});
Expand Down Expand Up @@ -208,7 +202,7 @@ function Breadcrumbs({
<VirtualizedList
ref={listRef}
deferredMeasurementCache={cache}
height={containerSize}
height={size}
overscanRowCount={5}
organization={organization}
rowCount={breadcrumbs.length}
Expand Down
6 changes: 3 additions & 3 deletions static/app/components/profiling/flamegraph/differentialFlamegraphLayout.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -43,16 +43,16 @@ export function DifferentialFlamegraphLayout(props: DifferentialFlamegraphLayout
? MIN_FLAMEGRAPH_DRAWER_DIMENSIONS[0]
: MIN_FLAMEGRAPH_DRAWER_DIMENSIONS[1];

const onResize = (newSize: number, maybeOldSize: number | undefined) => {
const onResize = (newSize: number) => {
if (!flamegraphDrawerRef.current) {
return;
}

if (isSidebarLayout) {
flamegraphDrawerRef.current.style.width = `${maybeOldSize ?? newSize}px`;
flamegraphDrawerRef.current.style.width = `${newSize}px`;
flamegraphDrawerRef.current.style.height = `100%`;
} else {
flamegraphDrawerRef.current.style.height = `${maybeOldSize ?? newSize}px`;
flamegraphDrawerRef.current.style.height = `${newSize}px`;
flamegraphDrawerRef.current.style.width = `100%`;
}
};
Expand Down
6 changes: 3 additions & 3 deletions static/app/components/profiling/flamegraph/flamegraphDrawer/profileDetails.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -91,14 +91,14 @@ export function ProfileDetails(props: ProfileDetailsProps) {
// Only used when in sidebar layout
const initialSize = isSidebarLayout ? 260 : 0;

const onResize = (newSize: number, maybeOldSize?: number) => {
const onResize = (delta: number) => {
if (!detailsBarRef.current) {
return;
}

if (isSidebarLayout) {
detailsBarRef.current.style.width = `100%`;
detailsBarRef.current.style.height = `${maybeOldSize ?? newSize}px`;
detailsBarRef.current.style.width = `${initialSize + delta}px`;
detailsBarRef.current.style.height = `100%`;
} else {
detailsBarRef.current.style.height = '';
detailsBarRef.current.style.width = '';
Expand Down
6 changes: 3 additions & 3 deletions static/app/components/profiling/flamegraph/flamegraphLayout.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -50,16 +50,16 @@ export function FlamegraphLayout(props: FlamegraphLayoutProps) {
? MIN_FLAMEGRAPH_DRAWER_DIMENSIONS[0]
: MIN_FLAMEGRAPH_DRAWER_DIMENSIONS[1];

const onResize = (newSize: number, maybeOldSize: number | undefined) => {
const onResize = (newSize: number) => {
if (!flamegraphDrawerRef.current) {
return;
}

if (isSidebarLayout) {
flamegraphDrawerRef.current.style.width = `${maybeOldSize ?? newSize}px`;
flamegraphDrawerRef.current.style.width = `${newSize}px`;
flamegraphDrawerRef.current.style.height = `100%`;
} else {
flamegraphDrawerRef.current.style.height = `${maybeOldSize ?? newSize}px`;
flamegraphDrawerRef.current.style.height = `${newSize}px`;
flamegraphDrawerRef.current.style.width = `100%`;
}
};
Expand Down
9 changes: 6 additions & 3 deletions static/app/components/replays/virtualizedGrid/useDetailsSplit.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,5 +1,5 @@
import type {RefObject} from 'react';
Copy link
Member Author

Choose a reason for hiding this comment

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

@ryan953 would you mind linking me to where this component is in the UI? I have verified that this works in profiling, but I would like to check for replay as well

Copy link
Member

Choose a reason for hiding this comment

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

oh, sorry for the huge delay!

this is imported by the replay network details thing. So if you're looking at a replay, click into the Network tab, then click on a specific network row.... you'll get Network Details coming up from the bottom. It's resizable.

SCR-20250411-jfsd

Copy link
Member Author

Choose a reason for hiding this comment

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

Thanks @ryan953, not a problem about the delay. I have more stuff that I'll need to fix, I just opened a bunch of stacked PRs to get this in, and I'll make sure to test everything where this is used before merging this in

import {useCallback} from 'react';
import {useCallback, useState} from 'react';

import {useResizableDrawer} from 'sentry/utils/useResizableDrawer';
import useUrlParams from 'sentry/utils/useUrlParams';
Expand Down Expand Up @@ -52,12 +52,15 @@ export default function useDetailsSplit({
// `initialSize` cannot depend on containerRef because the ref starts as
// `undefined` which then gets set into the hook and doesn't update.
const initialSize = Math.max(150, window.innerHeight * 0.4);
const [containerSize, setContainerSize] = useState(initialSize);

const {size: containerSize, ...resizableDrawerProps} = useResizableDrawer({
const resizableDrawerProps = useResizableDrawer({
direction: 'up',
initialSize,
min: 0,
onResize: () => {},
onResize: (newSize: number) => {
Copy link
Contributor

Choose a reason for hiding this comment

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

types can be inferred:

Suggested change
onResize: (newSize: number) => {
onResize: (newSize) => {

setContainerSize(newSize);
},
});

const maxContainerHeight =
Expand Down
24 changes: 16 additions & 8 deletions static/app/components/splitPanel.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,4 +1,4 @@
import {createContext, useCallback, useMemo} from 'react';
import {createContext, useCallback, useMemo, useState} from 'react';
import styled from '@emotion/styled';

import {IconGrabbable} from 'sentry/icons';
Expand Down Expand Up @@ -110,17 +110,25 @@ function SplitPanel(props: SplitPanelProps) {
const min = isLeftRight ? props.left.min : props.top.min;
const max = isLeftRight ? props.left.max : props.top.max;

const [containerSize, setContainerSize] = useState(() => {
const storedSize = sizeStorageKey
? parseInt(localStorage.getItem(sizeStorageKey) ?? '', 10)
: undefined;
return storedSize ?? initialSize;
});

const {
isHeld,
onDoubleClick,
onMouseDown: onDragStart,
size: containerSize,
setSize,
} = useResizableDrawer({
direction: isLeftRight ? 'left' : 'down',
initialSize,
min,
onResize: onResize ?? (() => {}),
onResize: (size: number) => {
setContainerSize(size);
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
onResize: (size: number) => {
onResize: (size) => {

onResize?.(size);
},
sizeStorageKey,
});

Expand All @@ -141,11 +149,11 @@ function SplitPanel(props: SplitPanelProps) {
() => ({
isMaximized,
isMinimized,
maximiseSize: () => setSize(max),
minimiseSize: () => setSize(min),
resetSize: () => setSize(initialSize),
maximiseSize: () => setContainerSize(max),
minimiseSize: () => setContainerSize(min),
resetSize: () => setContainerSize(initialSize),
}),
[isMaximized, isMinimized, setSize, max, min, initialSize]
[isMaximized, isMinimized, setContainerSize, max, min, initialSize]
);

if (isLeftRight) {
Expand Down
50 changes: 12 additions & 38 deletions static/app/utils/useResizableDrawer.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -16,11 +16,7 @@ export interface UseResizableDrawerOptions {
/**
* Triggered while dragging
*/
onResize: (
newSize: number,
maybeOldSize: number | undefined,
userEvent: boolean
) => void;
onResize: (size: number, userEvent: boolean) => void;
/**
* The local storage key used to persist the size of the container
*/
Expand Down Expand Up @@ -51,32 +47,16 @@ export function useResizableDrawer(options: UseResizableDrawerOptions): {
* Call this function to manually set the size of the drawer.
*/
setSize: (newSize: number, userEvent?: boolean) => void;
/**
* The resulting size of the container axis. Updated while dragging.
*
* NOTE: Be careful using this as this as react state updates are not
* synchronous, you may want to update the element size using onResize instead
*/
size: number;
} {
const rafIdRef = useRef<number | null>(null);
const currentMouseVectorRaf = useRef<[number, number] | null>(null);
const [size, setSize] = useState<number>(() => {
const storedSize = options.sizeStorageKey
? parseInt(localStorage.getItem(options.sizeStorageKey) ?? '', 10)
: undefined;

return storedSize || options.initialSize;
});
const [isHeld, setIsHeld] = useState(false);
const sizeRef = useRef<number>(options.initialSize);

const updateSize = useCallback(
(newSize: number, userEvent = false) => {
setSize(newSize);
options.onResize(newSize, undefined, userEvent);
if (options.sizeStorageKey) {
localStorage.setItem(options.sizeStorageKey, newSize.toString());
}
sizeRef.current = newSize;
options.onResize(newSize, userEvent);
},
[options]
Copy link
Contributor

Choose a reason for hiding this comment

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

making a memoized version of something with useCallback that depends on an object passed in as a prop (options) is rarely a good idea, because it means consumers would need to memoize the options object, which isn’t clear, so lots of usages don’t do it. It’s also not a great API.

If we call useResizableDrawer with an inline object, this useCallback does nothing. That also means all further memoization that relies on updateSize will be disappointed because it will be a new reference every render.

I guess the idea here was to have memoized versions returned to consumers, and there are consumers that do it right, like flamegraphDrawer/profileDetails.tsx, where we actually pass the returned functions to a memo’d component.

My suggestion here would be to use the latest ref pattern to write the options to a ref, then always use the latest version of it inside useCallback. This ensures consumers can pass options in however they want:

const optionsRef = useRef(options);
useLayoutEffect(() => {
  optionsRef.current = options;
});

This is also what react-query does to the options, because we need them in a useEffect, but don’t want it to be “reactive” towards it:

https://github.com/TanStack/query/blob/ed2d4e95a00df867347e13de456bb8ba057c51da/packages/react-query-persist-client/src/PersistQueryClientProvider.tsx#L27-L29

This is also where useEffectEvent from React itself will help in the future.

Copy link
Member Author

Choose a reason for hiding this comment

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

Agree. This API is bad and needs to be changed.

Curious, but why even use a useLayoutEffect for the latest ref? Couldn't you just write to the ref inside the hook body directly, I don't think that changes anything + you can drop the useLayoutEffect. The hook will be invoked if the options object changes, meaning you'd still get the latest value each time

const optionsRef = useRef(options)
optionsRef.current = options;

Copy link
Contributor

Choose a reason for hiding this comment

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

it’s technically not allowed to write to refs during render, as that is seen as a side-effect. The react compiler won’t optimize components were it sees those patterns. Technically, react could start a render, you would write to the ref, then it aborts a render (e.g. when using concurrent features). In those cases, the ref would’ve gotten updated “too early”, as the render didn’t complete.

so, it’s just against the “rules of react”.

);
Expand All @@ -85,14 +65,11 @@ export function useResizableDrawer(options: UseResizableDrawerOptions): {
// any potentional values set by CSS will be overriden. If no initialDimensions are provided,
// invoke the onResize callback with the previously stored dimensions.
useLayoutEffect(() => {
options.onResize(options.initialSize ?? 0, size, false);
setSize(options.initialSize ?? 0);
sizeRef.current = options.initialSize;
options.onResize(options.initialSize ?? 0, false);
// eslint-disable-next-line react-hooks/exhaustive-deps
}, [options.direction]);
Comment on lines 62 to 63
Copy link
Contributor

Choose a reason for hiding this comment

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

the only thing we exclude here is options, but if options were put into a ref with the pattern I outlined before, we wouldn’t need to disable the linter here.


const sizeRef = useRef<number>(size);
sizeRef.current = size;

const onMouseMove = useCallback(
(event: MouseEvent) => {
event.stopPropagation();
Expand Down Expand Up @@ -138,6 +115,11 @@ export function useResizableDrawer(options: UseResizableDrawerOptions): {
);

const onMouseUp = useCallback(() => {
if (rafIdRef.current !== null) {
window.cancelAnimationFrame(rafIdRef.current);
rafIdRef.current = null;
}

document.body.style.pointerEvents = '';
document.body.style.userSelect = '';
document.documentElement.style.cursor = '';
Expand All @@ -161,13 +143,5 @@ export function useResizableDrawer(options: UseResizableDrawerOptions): {
updateSize(options.initialSize, true);
}, [updateSize, options.initialSize]);

useLayoutEffect(() => {
return () => {
if (rafIdRef.current !== null) {
window.cancelAnimationFrame(rafIdRef.current);
}
};
});

return {size, isHeld, onMouseDown, onDoubleClick, setSize: updateSize};
return {isHeld, onMouseDown, onDoubleClick, setSize: updateSize};
}
Loading