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

Section in running sidebar triggers React rerender on any tab switch even if hidden #13676

Closed
krassowski opened this issue Dec 29, 2022 · 1 comment · Fixed by #14172
Closed

Comments

@krassowski
Copy link
Member

krassowski commented Dec 29, 2022

Description

On initialization of running sidebar Section (PanelWithToolbar subclass) a ReactWidget observing options.manager.runningChanged signal is created:

this.addWidget(
ReactWidget.create(
<UseSignal signal={options.manager.runningChanged}>
{() => {
return (
<div className={CONTAINER_CLASS}>
<List
runningItems={options.manager.running()}
shutdownLabel={options.manager.shutdownLabel}
shutdownAllLabel={shutdownAllLabel}
shutdownItemIcon={options.manager.shutdownItemIcon}
translator={options.translator}
/>
</div>
);
}}
</UseSignal>
)
);

Screenshot from 2022-12-29 13-14-05

There are two problems

  • this signal fires too often leading to unncessary re-rendering when tab was switched (rather than closed/removed), which is because no filtering of the tabsChanged signal takes place - it is just proxied as-is:
    runningChanged: signaler.tabsChanged,
  • this signal is received and triggers re-render even if the running sidebar is closed; it should be proxied in the widget and skipped if widget isHidden.

Reproduce

  1. Record profile
  2. Change tab
  3. See that running sidebar was re-rendered

Expected behavior

Hidden widgets do not re-render unnecessarily.

We could use props.shouldUpdate for UseSignal.

Context

  • JupyterLab version: 4.0.0.a31
@krassowski krassowski added this to the 4.0.0 milestone Dec 29, 2022
@krassowski krassowski removed the status:Needs Triage Applied to new issues that need triage label Dec 29, 2022
@JasonWeill JasonWeill modified the milestones: 4.0.0, 4.1.0 Feb 1, 2023
@fcollonval fcollonval modified the milestones: 4.1.0, 4.0.0 Feb 2, 2023
@krassowski
Copy link
Member Author

this signal fires too often leading to unnecessary re-rendering when tab was switched (rather than closed/removed), which is because no filtering of the tabsChanged signal takes place - it is just proxied as-is:

This one is difficult because the original signal comes from lumino's title.changed which emits without any data from changes to text, icon or className; it is reasonable that we listen to this signal because we may want to sync some of the title styling changes (e.g. text, icon, maybe colour in the future) in the "Open tabs" section of the running panel. The problem is that this changed fires on tab switch as a consequence of className update and we would need to make assumptions on what class name changes mean "tab switched" (and therefore are safe to ignore) and which should possibly need to be propagated.

I can also imagine a feature request of highlighting active tab in the panel so maybe this is not a priority.

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants