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

Disable prompt dialog when exiting home page #141

Merged
merged 1 commit into from
Mar 16, 2023
Merged

Conversation

danielhollas
Copy link
Contributor

@danielhollas danielhollas commented Feb 22, 2023

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 danielhollas requested review from unkcpz and yakutovicha and removed request for unkcpz and yakutovicha February 22, 2023 14:39
@danielhollas
Copy link
Contributor Author

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 danielhollas marked this pull request as draft February 24, 2023 12:54
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 oschuett/appmode@8665aa6

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 #133 for further technical details and discussion.
@danielhollas danielhollas marked this pull request as ready for review March 6, 2023 21:36
@danielhollas
Copy link
Contributor Author

@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
Copy link
Member

unkcpz commented Mar 15, 2023

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
Copy link
Contributor Author

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.

Copy link
Member

@yakutovicha yakutovicha left a comment

Choose a reason for hiding this comment

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

I tested old and new versions. I agree with the change 👍

@yakutovicha yakutovicha merged commit ed851ef into main Mar 16, 2023
@yakutovicha yakutovicha deleted the disable-prompt-home branch March 16, 2023 10:18
danielhollas added a commit that referenced this pull request Jul 26, 2023
This is a follow up on #141 where we disabled the alert prompt when leaving the home page. We've been using it for quite a while since and haven't noticed any issues so it's time to apply to other apps / pages.
In the future, we should ideally apply this fix directly in Appmode, but that will require more work so for now here's just a simple hotfix.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants