-
Notifications
You must be signed in to change notification settings - Fork 89
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
base: master
Are you sure you want to change the base?
frontend: display offline errors on account summary #1859
Conversation
@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] }); |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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
There was a problem hiding this comment.
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.
3bd9f06
to
8b99efa
Compare
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 cc @benma |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
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. |
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 🤔 |
8b99efa
to
55cf5c3
Compare
rebased |
55cf5c3
to
ab3f18d
Compare
ab3f18d
to
ebac345
Compare
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.
ebac345
to
e1044a5
Compare
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.