Skip to content
This repository was archived by the owner on Jan 22, 2025. It is now read-only.

explorer: Auto-update transactions until they reach max confirmation#11841

Merged
oJshua merged 18 commits intosolana-labs:masterfrom
oJshua:feature/11760-auto-update-transactions-until-max
Aug 28, 2020
Merged

explorer: Auto-update transactions until they reach max confirmation#11841
oJshua merged 18 commits intosolana-labs:masterfrom
oJshua:feature/11760-auto-update-transactions-until-max

Conversation

@oJshua
Copy link
Copy Markdown
Contributor

@oJshua oJshua commented Aug 25, 2020

Problem

When a transaction that hasn't reached max confirmations is entered, the user needs to mash the refresh button on the right until it reaches max confirmation.

Summary of Changes

Status card auto-refreshes every 2 seconds until the confirmation max is reached. Account card will display loading card while auto-refresh is in process. A loading indicator is placed in the top right. Once full confirmation is complete, the refresh button becomes available. Autorefresh will bailout if confirmations stuck at 0 due to transaction landing in a fork.

Fixes #11760

Comment thread explorer/src/providers/cache.tsx Outdated
Comment thread explorer/src/providers/cache.tsx Outdated
@oJshua
Copy link
Copy Markdown
Contributor Author

oJshua commented Aug 26, 2020

Cool, so now we have the edge case for when a transaction gets stuck in 0 confirmation land covered, and the bailout state lives back in the component.

Comment on lines -236 to -239
<ErrorCard
retry={refreshStatus}
text="Details are not available until the transaction reaches MAX confirmations"
/>
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Why did you decide to remove this state? I liked that it communicated to a user why they weren't seeing the full details for a transaction.

Copy link
Copy Markdown
Contributor Author

@oJshua oJshua Aug 27, 2020

Choose a reason for hiding this comment

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

I think we discussed this earlier this week or last. We decided to show the loading card instead of having a "details are not available" w/ the retry button, since an auto refresh would be in process and the "retry" button wouldn't make much sense.

Copy link
Copy Markdown
Contributor Author

@oJshua oJshua Aug 27, 2020

Choose a reason for hiding this comment

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

Another option would be to show the message "Details are not available until the transaction reaches MAX confirmations" w/o retry (with a loading indicator?).

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Went ahead and added that back!

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Ah ok I missed your reply here:

#11747 (comment)

So basically, there are three overall states: transaction not found, transaction hasn't reached max (auto refresh active), transaction has max confirmations.

The "details are not available until the transaction reaches MAX confirmations" error card would be eliminated.

I think there is one more state when we bail out from auto refresh. I think it would be sufficient to show this error card when the auto refresh is bailed out.

Another option would be to show the message "Details are not available until the transaction reaches MAX confirmations" w/o retry (with a loading indicator?).

But this sounds good too! Up to you. Dropping the retry in either case sounds good

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Oh I see you went with showing the message during auto refresh, nice!

if (autoRefresh === AutoRefresh.BailedOut || !status?.data?.info) {
return null;
} else if (!details) {
} else if (autoRefresh === AutoRefresh.Active) {
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

I still think it would be nice to show this error card when auto refresh has been bailed out, thoughts?

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

I agree! I added that case. If it doesn't make sense to have a retry button I can pull that out.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Retry makes sense! But it should be refreshStatus because otherwise the details could get refreshed but the status would be still stuck showing 0 confirmations

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Oh yes, done!

@oJshua oJshua merged commit 0a8523b into solana-labs:master Aug 28, 2020
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

explorer: Auto-update transactions until they reach max confirmation

2 participants