datalayer / jupyter-ui

⚛️ React.js components 💯% compatible with 🪐 Jupyter. https://jupyter-ui-storybook.datalayer.tech
https://jupyter-ui.datalayer.tech
Other
291 stars 42 forks source link

fix keyboad event cleanup #220

Closed box-turtle closed 1 month ago

box-turtle commented 1 month ago

This is a suggested fix for https://github.com/datalayer/jupyter-ui/issues/219#issue-2300634048

The problem seems to be that each time the NotebookAdapter is created a new keydown event listener is added to the document. This means that keyboard command like shift-enter don't work if you modify the notebook by changing the path or nbformat parameters.

Although the modified useEffect function in Notebook.tsx had a call to the adapter cleanup function, this was never called because in my testing adapter was always undefined by the time the useEffect cleanup function was executed.

Instead we call adapter.cleanup() in the main part of useEffect, which will still have a reference to the previous adapter before it creates the new one.

I don't understand the full codebase enough to know if that has any other deleterious side effects, but it fixes the bug reported in the issue above.

Here's a gif of the NotebookPathChange.tsx example being run after this fix:

notebook

echarles commented 1 month ago

Awesome @box-turtle :+1: Thank you so much for this fix. CI builds and I have tested it on various case, it all works fine. Merging. Congrats for this!🎉