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

frontend: display offline errors on account summary #1859

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

thisconnect
Copy link
Collaborator

Collect status.offlineError of all different accounts and if there are any, render the errors instead of the account summary.

This can happen if Tor is down or misconfigured or internet not reachable.

@thisconnect thisconnect requested a review from benma November 29, 2022 08:15
@thisconnect
Copy link
Collaborator Author

@benma this currently only looks at offlineError but ignores fatalError.

Also it collects the offlineErrors from all accounts and shows them all. This only works the first time, but when navigating to other accounts and back somehow it doesn't receive the errors anymore 🤔

@@ -157,6 +160,10 @@ class AccountsSummary extends Component<Props, State> {

private async onStatusChanged(code: string, initial: boolean = false) {
const status = await accountApi.getStatus(code);
const { offlineErrors } = this.state;
if (status.offlineError && !offlineErrors.includes(status.offlineError)) {
this.setState({ offlineErrors: [...offlineErrors, status.offlineError] });
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

could also push status.fatalError && t('account.fatalError') into this array, or replace the array completely if 1 account had fatalError.

On the other hand the offlineError might contain useful information like Tor misconfiguration.

Copy link
Collaborator

Choose a reason for hiding this comment

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

I would not replace the array in case of fatalError, I agree with you that offlineError could be useful

Copy link
Contributor

Choose a reason for hiding this comment

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

what if you get online again? this code seems to never reset offlineErrors when back online.

@thisconnect thisconnect force-pushed the frontend-summary-offlineerrors branch from 3bd9f06 to 8b99efa Compare November 29, 2022 08:32
@thisconnect thisconnect requested review from Beerosagos and removed request for benma December 1, 2022 08:46
@Beerosagos
Copy link
Collaborator

Also it collects the offlineErrors from all accounts and shows them all. This only works the first time, but when navigating to other accounts and back somehow it doesn't receive the errors anymore thinking

Tried to investigate this, but couldn't really figure it out. It seems to me that after a while backend account handlers stop working correctly, stalling frontend status requests. Could be related to a mutex that will not be unlocked? I tried to debug removing some locks and seems to improve (even if it doesn't solve it completely).

cc @benma

Copy link
Collaborator

@Beerosagos Beerosagos left a comment

Choose a reason for hiding this comment

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

LGTM

@benma
Copy link
Contributor

benma commented Dec 8, 2022

I also couldn't see the reason after staring at it for 30min. Will need more time on this.

It also happens without this PR, if you have an error and you switch to account->portfolio->account, the backend does not respond on many handlers.

Ideally we refactor and unit test account status management, it grew to be quite a mess.

@benma
Copy link
Contributor

benma commented Dec 8, 2022

It's probably because /balance and /transactions block until synced, but it might never sync. We should make these non blocking and return an error instead, with notifications to load them when ready or something like that.

No clue why /status would not respond, it looks like it should simply deliver a response immediately 🤔

@thisconnect thisconnect force-pushed the frontend-summary-offlineerrors branch from 8b99efa to 55cf5c3 Compare December 22, 2022 10:06
@thisconnect
Copy link
Collaborator Author

rebased

Collect status.offlineError of all different accounts and if there
are any, render the errors instead of the account summary.

This can happen if Tor is down or misconfigured or internet not
reachable.
@thisconnect thisconnect force-pushed the frontend-summary-offlineerrors branch from ebac345 to e1044a5 Compare March 22, 2023 12:19
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