Fix issue where network table height is not updated on initial load

[buxomant]

One-line summary

Issue : RDX-664

Description

When the network viewer is loaded for the first time, the height of <NetworkTableBody> is initialized as 0, but does not immediately update its height based on the data. The data is there, but it's not visible, until switching to another sub-tab, and then back to the "All" tab.

This seems to happen because the resizeObserver for elementDims does not trigger on this initial load, so the dimensions stay as 0. And we only update tableBodyHeight if elementDims is non-zero for some reason.

The fix is to check ref.current.clientHeight instead of elementDims. This value seems to be more accurately gauged as non-zero on initial page load. And we don't seem to care about the exact value, just that it's non-zero.

The root cause was basically this line in NetworkTableContainer:

  const { elementDims } = useResizeObserver(ref?.current);

That ref?.current gets evaluated the first time to undefined, and then passed to useResizeObserver which uses it as a dependency for its useEffect function. But because it was already evaluated to undefined, it's no longer a MutableRefObject and can never update, and so the effect never triggers.

I changed this to simply pass through the MutableRefObject ref, and changed the code inside useResizeObserver to make it work with this change. I also had to make changes to NetworkTableBody, the other place where useResizeObserver is used.

Types of Changes

• Bug fix (non-breaking change which fixes an issue)

[sakhisheikh]

wouldn't if (ref?.current) be enough?

[buxomant]

I think it would be enough as well, but I didn't want to make a bigger change without having a lot of context in this area. I think it makes sense to check for ref?.current.clientHeight since this is the same value we're setting tableBodyHeight to.

[sakhisheikh]

do we need elementDims after this change?

[buxomant]

Maybe, maybe not 😃

I'm not 100% sure about the intended relation between elementDims and ref?.current. For now, I left elementDims in the list of dependencies for triggering setTableBodyHeight.

[sakhisheikh]

alright. I see elemenDims will change on resizing the table so we need that to trigger the effect. Just to be on the safe clientHeight also changes when you resize?

[sakhisheikh]

Also the issue is with useResizeObserver, it should update the elementDims after the component is mounted. providing height: 0 and width: 0 which is the initial state on the first render and not updating it afterwards sounds like a bug.
We wrote useResizeObserver and I see we don't check if the component is mounted. This is what we are missing so you could update the dimensions safely after the component is mounted.

[buxomant]

Will test the resize locally, and also try to add that mount check.

[buxomant]

@sakhisheikh Still not entirely sure about the bug, but in debugging locally it looks like elementRef is always null during the initial page load (where we define useResizeObserver), so we never even call observe here:

    if (elementRef) {
      resizeObserver.observe(elementRef);
    }

[buxomant]

@sakhisheikh I think I understand the issue better now -- I reverted my initial fix, and implemented an entirely different approach.

The root cause was basically this line in NetworkTableContainer:

  const { elementDims } = useResizeObserver(ref?.current);

That ref?.current gets evaluated the first time to undefined, and then passed to useResizeObserver which uses it as a dependency for its useEffect function. But because it was already evaluated to undefined, it's no longer a MutableRefObject and can never update, and so the effect never triggers.

I changed this to simply pass through the MutableRefObject ref, and changed the code inside useResizeObserver to make it work with this change. I also had to make changes to NetworkTableBody, the other place where useResizeObserver is used.

Tested locally that:

• Height change events are handled properly for NetworkTableContainer
• Width change events are handled properly for NetworkTableBody

[sakhisheikh]

why you are checking _outRef? also seems like internal API?

[buxomant]

I need to do this here now, because it was previously done in NetworkTableBody:

  const { elementDims } = useResizeObserver(listRef?.current?._outerRef || listRef?.current);

[sakhisheikh]

I saw we were actually already doing it in useResizeObserver. It was not done right in the first place

To me it seems like we are not using ref and effect properly here that should actually do the job. _outref seems to be workaround like we are relying on something else because our ref is not working as expected. React also encourages to use hooks which are publicly available.

I know you didn't write that useResizeObserver. The ref inside that behaves buggy. That would require a proper fix.

However if you want you can go ahead and merge your changes. We can come back to it later.

[sakhisheikh]

I also have the feeling useLayouteffect could fix the issue for initial render because it runs before the browser repaints

[buxomant]

To me it seems like we are not using ref and effect properly here that should actually do the job. _outref seems to be workaround like we are relying on something else because our ref is not working as expected.

No, based on my local debugging we're just abusing the fact that types aren't strictly enforced in JS, I think. This _outerRef is coming from react-window: https://github.com/bvaughn/react-window/blob/72db696dd8ebb7f0f287c78d037ff68ba9534183/src/createListComponent.js#L161

It looks to me like sometimes we pass an actual DOM element as a reference, other times we pass this kind of virtualized list, and we need to handle both cases. This _outerRef is the only way we can get to the DOM element in the second case.

[sakhisheikh]

Alright. I forgot we have react-window here 😄 so listRef isn't enough. Right? idk it seems either react-window lacks something or we could handle refs better. I didn't run it locally yet. but using outerRef seems to me not the right approach. it must've been exposed by author of the library.

Go ahead and merge your changes.