aiidalab / aiidalab-home

AiiDAlab Home App
MIT License
5 stars 9 forks source link

Disable prompt dialog when exiting home page #141

Closed danielhollas closed 1 year ago

danielhollas commented 1 year ago

This part of a solution for #133 (see more info there). See also the commit message below for full context.

In this PR, I am only affecting the home page. After we verify this works in real world testing, we should probably apply this fix in the Appmode itself, specifically after this line here: https://github.com/oschuett/appmode/blob/3998dc1a6d61f06581fe818351e70860c26c442a/appmode/static/main.js#L136

Essentially, I just added a piece of Javascript that disables the prompt functionality which comes from Jupyter notebook. We only disable the prompt when in the appmode.

Testing plan:

  1. Open home page
  2. Open "Tasks". You should see the running home page notebook.
  3. Close the home page. There should be no prompt warning from the browser.
  4. Verify that the notebook disappears from the task list.
  5. Open the home page as a jupyter notebook (not in Appmode).
  6. Modify the first cell (e.g. by adding a space) and execute it. Then try to exit the tab. You should get a prompt dialog because of unsaved changes, even though the JS snippet was executed, because we're not in Appmode.

Draft for commit message:

Currently, whenever one tries to navigate away from an appmode app,
(e.g. by clicking a URL link, or closing a tab), a confirmation dialog always pops up,
even without any real interaction with the page.
This is especially annoying for the home app.

The confirmation dialog comes from the jupyter notebook,
because Appmode always copies the notebook and executes it,
which leaves it in an "unsaved" state from the jupyter notebook point of
view.

Here we simply override the 'onbefereunload' event handler that
is defined.

For now this fix is only applied to the home page.
If there are no issues, we could fix this globally by implementing
the fix directly in the Appmode itself, specifically here:
https://github.com/oschuett/appmode/blob/3998dc1a6d61f06581fe818351e70860c26c442a/appmode/static/main.js#L136

Note that in the past, the `onbeforeunload` event used to be
overrided in Appmode, but later the handler was moved to the `unload`
event, which exposed the confirmation dialog.
See https://github.com/oschuett/appmode/commit/8665aa6474164023a9f59a3744ee5ffe5c3a8b4a

The unload event handler in Appmode takes care of the cleanup,
i.e. shutting down the kernel. Since we're modifying a different event,
we should be safe (and I have tested that sessions get cleaned properly).

See https://github.com/aiidalab/aiidalab-home/issues/133 for further technical details and discussion.
danielhollas commented 1 year ago

I just had an idea how to improve this. We should be able to detect inside the javascript, whether we're running in the Appmode or not, which would make this workaround safe. Marking as a draft for now.

danielhollas commented 1 year ago

@unkcpz @yakutovicha This should be ready. Please take a look and test (I suggested the testing plan in the OP). Let me know if the suggested commit message makes things sufficiently clear.

unkcpz commented 1 year ago

It turns out the solution is quite simple. I am not very familiar with javascript so need a close look after it goes to appmode. I give it a test and it works fine. I can now change the apps order in the home page without seeing the annoying prompt dialog, and the change of app order also gets saved after refresh.

danielhollas commented 1 year ago

Thanks @unkcpz.

Note that I've applied the same fix in my app and have been using this for last two weeks and did not notice any issues.

@yakutovicha If you agree I'll merge this so we can give this a try.