Skip to content
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

fix[DevTools/Tree]: only scroll to item when panel is visible #32018

Open
wants to merge 4 commits into
base: main
Choose a base branch
from

Conversation

hoxyq
Copy link
Contributor

@hoxyq hoxyq commented Jan 8, 2025

Stacked on #31968. See commit on top.

Fixes an issue with bank tree view, when we are scrolling to an item while syncing user DOM selection. This should only have an effect on browser extension. Added events with extension prefix will only be emitted in browser extension implementation, for other implementations useExtensionComponentsPanelVisibility will return constant true value.

Before:

Screen.Recording.2025-01-08.at.15.28.21.mov

After:

Screen.Recording.2025-01-08.at.15.27.17.mov

@facebook-github-bot facebook-github-bot added CLA Signed React Core Team Opened by a member of the React Core Team labels Jan 8, 2025
@hoxyq hoxyq changed the title React devtools/fix tree item scroll while syncing fix[DevTools/Tree]: only scroll to item when panel is visible Jan 8, 2025
Copy link
Contributor

@EdmondChuiHW EdmondChuiHW left a comment

Choose a reason for hiding this comment

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

Thanks for diving into it and fixing this! Added some inline questions. (CI for Vercel deployment also failed. notsureif.gif)

Comment on lines +209 to +214
createdPanel.onShown.addListener(() => {
bridge.emit('extensionComponentsPanelShown');
});
createdPanel.onHidden.addListener(() => {
bridge.emit('extensionComponentsPanelHidden');
});
Copy link
Contributor

Choose a reason for hiding this comment

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

Do we need to call createdPanel.onShown.removeListener(…) somewhere?

Comment on lines +28 to +34
bridge.addListener('extensionComponentsPanelShown', onPanelShown);
bridge.addListener('extensionComponentsPanelHidden', onPanelHidden);

return () => {
bridge.removeListener('extensionComponentsPanelShown', onPanelShown);
bridge.removeListener('extensionComponentsPanelHidden', onPanelHidden);
};
Copy link
Contributor

Choose a reason for hiding this comment

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

Beautiful setup+tear down pairs

export function useExtensionComponentsPanelVisibility(
bridge: FrontendBridge,
): boolean {
const [isVisible, setIsVisible] = useState(true);
Copy link
Contributor

Choose a reason for hiding this comment

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

Is see from the comment and PR description that the default should be true if it's not an extension.

But for extensions, is possible that it's initalised in the background/invisibly without a extensionComponentsPanelHidden event emitted?

Is there a way to read/store the visibility from bridge directly?

Comment on lines +100 to +102
if (listRef.current != null && inspectedElementIndex !== null) {
listRef.current.scrollToItem(inspectedElementIndex, 'smart');
}
Copy link
Contributor

Choose a reason for hiding this comment

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

I see the need to respond to componentsPanelVisible changing, but I don't see how the concerns of the previous impl is addressed, i.e. this impl would miss a call to scrollToItem if listRef failed a null check earlier.

Conceptually, I think we want scrollToItem to run in response to three changes:

  1. listRef itself getting assigned an element ref
  2. inspectedElementIndex changing
  3. componentsPanelVisible changing

The previous impl triggers on 1) only. The cached function is updated in response to 2), but it's not actually called.
The current impl adds triggers on both 2) and 3). Is responding to 2) part of this PR?

If we want all three, we'd need both callback ref and effect, e.g.

const listRef = useRef(null);

const maybeScrollToItem = useCallback(() => {
    if (componentsPanelVisible && inspectedElementIndex !== null) {
      listRef.current?.scrollToItem(inspectedElementIndex, 'smart');
    }
}, [inspectedElementIndex, componentsPanelVisible]);

// triggered by 2) and 3) when `useCallback` busts the cache
useEffect(() => maybeScrollToItem(), [maybeScrollToItem]);

// triggered by 1)
const listCallbackRef = useCallback((list: HTMLElement) => {
  listRef.current = list;
  maybeScrollToItem();
}, [maybeScrollToItem]);

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
CLA Signed React Core Team Opened by a member of the React Core Team
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants