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

style(frontend): uses session storage instead of local storage for closable message #3918

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

Conversation

BonomoAlessandro
Copy link
Collaborator

@BonomoAlessandro BonomoAlessandro commented Dec 10, 2024

Motivation

In some parts of the application, we display closable message bloxes. When logged in, closed message boxes should not appear again. But, if the user logs out and then logs in again, the messages should be displayed again.

Changes

  • uses session storage instead of local storage.
  • resets session storage on logout

Tests

before:

oisy-wallet-activity-messages-old.mp4

after:

oisy-wallet-activity-messages-new.mp4

@BonomoAlessandro BonomoAlessandro marked this pull request as ready for review December 10, 2024 09:56
@BonomoAlessandro BonomoAlessandro requested a review from a team as a code owner December 10, 2024 09:56
@peterpeterparker
Copy link
Member

peterpeterparker commented Dec 10, 2024

But, if the user logs out and then logs in again, the messages should be displayed again.

In the case of explicit logout (i.e., when the user clicks the 'Logout' button), or also in the case of automatic logout when the session expires?

If it's the former, I think we should stick with the current implementation and add a function to clear the local storage for this specific option when the user clicks 'Logout'. This functionality is already partially implemented in auth.service and would only require adding the clear action.

Side note: As far as I remember, the current implementation was designed this way because users can potentially have multiple identities and might find it annoying if this information always pops up when they switch accounts. Similarly, we also considered that on a single laptop, a user might not want to see this information repeatedly each time they sign up again, assuming they click 'Logout' to terminate their session. Of course, these were just assumptions.

@BonomoAlessandro
Copy link
Collaborator Author

@peterpeterparker I just had a chat with @StefanBerger-DFINITY and he meant that it is ok that the messages are shown again if the session exires or the user changes his identity. If a user changes the identity some other tokens can be enabled and/or broken/ missing an Index canister. This means it would always makes sense to display the messages again if the identity has changed.

@peterpeterparker
Copy link
Member

Surprising, that's the kind of small thing that really annoys me as a user when I use an app but that's probably just me then.
Alright, let me take a look at the code in the PR.

@@ -115,6 +115,11 @@ const clearTestnetsOption = async () => {
testnetsStore.reset({ key: 'testnets' });
};

// eslint-disable-next-line require-await
const clearSessionStorage = async () => {
sessionStorage.clear();
Copy link
Member

Choose a reason for hiding this comment

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

Why are we clearing the session storage?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

@peterpeterparker Because sveltes session storage is cleared when the page session ends. This only happens if the browser or the tab is closed. This means that we have to handle the session storage clear on a logout.

Copy link
Member

Choose a reason for hiding this comment

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

Thanks. Right. MDN https://developer.mozilla.org/en-US/docs/Web/API/Window/sessionStorage.

A page session lasts as long as the tab or the browser is open, and survives over page reloads and restores.

Can you provide a test which ensures clearSessionStorage is called on logout?

emptyIdbBtcAddressMainnet(),
emptyIdbEthAddress(),
clearTestnetsOption(),
clearSessionStorage()
Copy link
Member

Choose a reason for hiding this comment

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

he meant that it is ok that the messages are shown again if the session exires

I think clearSessionStorage isn't called when the session expires because of the option clearStorages which is set to false for that use case if I remember correctly. Can you double check and if needed adapt according the requirements?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

@peterpeterparker you are right. I checked it with @StefanBerger-DFINITY again and it should clear the session storage. I will fix it.
thx

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.

2 participants