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

PCDs added in multiple popups can be lost #1384

Open
artwyman opened this issue Dec 20, 2023 · 2 comments
Open

PCDs added in multiple popups can be lost #1384

artwyman opened this issue Dec 20, 2023 · 2 comments
Assignees
Labels

Comments

@artwyman
Copy link
Collaborator

If a user uses multiple "add PCD" popups, and closes them quickly, the resulting changes may not reach the server, and could be lost if they are clobbered by further changes.

Example steps to reproduce:

  • Open consumer-client -> Add Messages to the Passport
  • Click "prove and add a signature proof" with the message "1"
  • Pop-up opens. Don't click Prove yet.
  • Back in the consumer-client, click "prove and add a signature proof" again with message "2"
  • In Popup 1, Click Prove then quickly click Close.
  • In Popup 2, Click Prove then quickly click Close.
  • Go to main PCDPass client, reload page, and examine results. You'll probably have one but not both of the new signature proofs.

If I follow these same steps but pause for 2s before clicking Close, all the PCDs survive.

The underlying issue seems to be closing the window before the new PCD is uploaded to the server. The in-memory state is lost. The new PCD is saved to local storage quickly, which would help if the popup were the only tab on the device. Opening a new tab (or reloading in the steps above) will load from local storage. However, existing tabs don't load from local storage, and there's also no ability merge when loading/saving from local storage. There's also the broader issue that if a user never opens another tab on this device at all, that new PCD is stuck in limbo.

The right user-focused answer here is probably to make sure the prove screen can wait for upload to complete before showing the Close button. Changes which reach the server are safe, and will be subject to merge logic when they reach other devices/tabs. Local storage is currently structured like a backup for when the "app" is killed and reloaded, but doesn't have robust merge logic, and isn't guaranteed to ever reach other devices.

@artwyman artwyman added the bug label Dec 20, 2023
@artwyman artwyman self-assigned this Dec 20, 2023
@ichub
Copy link
Contributor

ichub commented Jan 9, 2024

Additionally I think it's possible to intercept a window's closing, and to confirm with the user that they really want to do that before proceeding. e.g. gmail does this when you've archived an email and close the tab immediately after.

@artwyman
Copy link
Collaborator Author

Yes, I saw that in my research for this issue. For future devs examining this issue, relevant docs are here: https://developer.mozilla.org/en-US/docs/Web/API/Window/beforeunload_event

Doing that in a reasonable way is blocked on having reliable "status flags" output by the sync state machine, so the UI can track when there are unuploaded changes and discourage the user from closing the tab. Doing that is probably easier if it's simply "anything to upload" vs. "my particular change has been uploaded" which is something I feel is needed for other use cases. But all of it might be better done as part of a larger refactor of sync out of the UI framework, so I didn't try to tackle it before the holidays.

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

No branches or pull requests

2 participants