Skip to content

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

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

Merged
buxomant merged 4 commits into from
Jul 11, 2025
Merged

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

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
ee79c06
Fix issue where network table height is not updated on initial load
buxomant Jul 11, 2025
0dc52c4
Revert "Fix issue where network table height is not updated on initia…
buxomant Jul 11, 2025
596d164
Pass 'ref' instead of 'ref?.current' to 'useResizeObserver' call
buxomant Jul 11, 2025
8c003a7
Handle ref coming from 'NetworkTableBody' as well
buxomant Jul 11, 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
2 changes: 1 addition & 1 deletion src/Components/NetworkTable/NetworkTableBody.jsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -51,7 +51,7 @@ const NetworkTableBody = ({ height }) => {
const selectedReqIndex = state.get('selectedReqIndex');

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

useEffect(() => {
actions.setTableHeaderWidth(elementDims.width);
Expand Down
2 changes: 1 addition & 1 deletion src/Containers/NetworkTableContainer.jsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -23,7 +23,7 @@ const NetworkTableContainer = () => {

const [tableBodyHeight, setTableBodyHeight] = useState(0);
const ref = useRef();
const { elementDims } = useResizeObserver(ref?.current);
const { elementDims } = useResizeObserver(ref);

useEffect(() => {
if (ref?.current && elementDims.height) {
Expand Down
17 changes: 9 additions & 8 deletions src/hooks/useResizeObserver.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -19,27 +19,28 @@ export const useResizeObserver = (elementRef) => {
});

useEffect(() => {
const ref = elementRef?.current?._outerRef || elementRef?.current;
Copy link
Copy Markdown
Contributor

@sakhisheikh sakhisheikh Jul 11, 2025

Choose a reason for hiding this comment

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

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

Copy link
Copy Markdown
Contributor Author

@buxomant buxomant Jul 11, 2025

Choose a reason for hiding this comment

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

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

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

Copy link
Copy Markdown
Contributor

@sakhisheikh sakhisheikh Jul 11, 2025 •
edited
Loading

Choose a reason for hiding this comment

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

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.

Copy link
Copy Markdown
Contributor

@sakhisheikh sakhisheikh Jul 11, 2025 •
edited
Loading

Choose a reason for hiding this comment

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

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

Copy link
Copy Markdown
Contributor Author

@buxomant buxomant Jul 11, 2025 •
edited
Loading

Choose a reason for hiding this comment

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

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.

Copy link
Copy Markdown
Contributor

@sakhisheikh sakhisheikh Jul 11, 2025

Choose a reason for hiding this comment

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

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.

const onResize = debounce(() => {
if (elementRef) {
if (ref) {
setElementDims({
width: elementRef.clientWidth,
height: elementRef.clientHeight,
width: ref.clientWidth,
height: ref.clientHeight,
});
}
}, 50);

const resizeObserver = new ResizeObserver(() => onResize());

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

return () => {
if (elementRef) {
resizeObserver.unobserve(elementRef);
if (ref) {
resizeObserver.unobserve(ref);
}
};
}, [elementRef]);
}, [elementRef?.current]);

return { elementDims };
};