Skip to content
This repository has been archived by the owner on Oct 25, 2023. It is now read-only.

reorg account init in truthsayer to make usage deterministic #419

Merged
merged 4 commits into from
Jan 30, 2023

Conversation

SergNikitin
Copy link
Collaborator

@SergNikitin SergNikitin commented Jan 28, 2023

Current way authentication is handled in truthsayer: most UI components that require authentication are rendered conditionally under PrivateRoute & PublicRoute which only show them if MzdGlobal.account is not null. Since historically StorageApi was an always-ready singleton that didn't require init, this lead to code that treats account != null checks as "application is fully ready".

This became problematic with the addition of extra things which have asynchronous initialisation to MzdGlobal, like MzdGlobal.storage which has to fetch AppSettings from archaeologist before it can correctly initialise itself. Components start rendering as soon as authentication is done and they start using other parts of MzdGlobal which may or may not be ready. At best this leads to a brief moment when SearchGrid starts to furiously load nodes from our datacenter, even if user chose to use offline mode. At worst it can lead to unexpected access to AlwaysThrowingStorageApi which crashes the whole UI.

Hacking around that turned out to be awkward and fragile, so this PR takes a different approach -- it changes the role of MzdGlobal in a three ways:

  1. MzdGlobal now only exists when a user is authenticated and it's only accessible to components under private routes
  2. MzdGlobal renders its children only once it's fully initialised (including async init) which enables its deterministic usage without surprises.
  3. authentication now happens outside of MzdGlobal which allows to render public routes without MzdGlobal existing at all

#351

@SergNikitin SergNikitin added enhancement New feature or request effort [XS] Up to 4 hours truthsayer labels Jan 28, 2023
Copy link
Contributor

@akindyakov akindyakov left a comment

Choose a reason for hiding this comment

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

Looks great!

  1. Couple of questions, what's the status of offline/online mode - are they both functional by now?
  2. What's going to happen when local mode is enabled in settings but there is no Archaeologist installed?

truthsayer/src/App.tsx Outdated Show resolved Hide resolved
@SergNikitin SergNikitin mentioned this pull request Jan 30, 2023
8 tasks
@SergNikitin
Copy link
Collaborator Author

what's the status of offline/online mode - are they both functional by now?

Online mode remains as functional as it was 🤞
Offline - you can find the list of known bugs in #351(scroll to the bottom) 🐞 This PR is a part of fixing one of them.

What's going to happen when local mode is enabled in settings but there is no Archaeologist installed?

truthsayer will try to fetch AppSettings from archaeologist but that'll fail. It will fallback to defaultSettings() which right now means "store data in our datacenter". I suspect soon-ish we'll want to change the defaults and then this PR will help a lot!

@SergNikitin SergNikitin changed the title reorg account init to make usage deterministic reorg account init in truthsayer to make usage deterministic Jan 30, 2023
@SergNikitin SergNikitin marked this pull request as ready for review January 30, 2023 22:09
@SergNikitin SergNikitin merged commit 6293c7c into main Jan 30, 2023
@SergNikitin SergNikitin deleted the storagep-api-bugfix-ownership branch January 30, 2023 22:57
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
effort [XS] Up to 4 hours enhancement New feature or request truthsayer
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants