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

Persist local statuses #3211

Merged
merged 8 commits into from
Dec 23, 2021
Merged

Persist local statuses #3211

merged 8 commits into from
Dec 23, 2021

Conversation

iamacook
Copy link
Member

@iamacook iamacook commented Dec 22, 2021

What it solves

Resolves #3194

How this PR fixes it

Local statuses are now persisted in the sessionStorage by means of a middleware. The Redux store is initialised by said sessionStorage key.

How to test it

  1. Create a transaction that immediately executes/execute a queued transaction.
  2. Refresh the page and observe that the status is still pending.

@github-actions
Copy link

CLA Assistant Lite All Contributors have signed the CLA.

@github-actions
Copy link

github-actions bot commented Dec 22, 2021

ESLint Summary View Full Report

Annotations are provided inline on the Files Changed tab. You can also see all annotations that were generated on the annotations page.

Type Occurrences Fixable
Errors 0 0
Warnings 0 0
Ignored 1 N/A
  • Result: ✅ success
  • Annotations: 0 total

Report generated by eslint-plus-action

@coveralls
Copy link

coveralls commented Dec 22, 2021

Pull Request Test Coverage Report for Build 1615773475

  • 11 of 31 (35.48%) changed or added relevant lines in 7 files are covered.
  • 59 unchanged lines in 6 files lost coverage.
  • Overall coverage increased (+0.06%) to 32.253%

Changes Missing Coverage Covered Lines Changed/Added Lines %
src/components/AppLayout/Sidebar/DebugToggle/index.tsx 0 1 0.0%
src/logic/safe/hooks/useOwnerSafes.ts 0 1 0.0%
src/utils/storage/useCachedState.ts 0 1 0.0%
src/logic/safe/store/middleware/notificationsMiddleware.ts 0 3 0.0%
src/logic/safe/store/middleware/localTransactionsMiddleware.ts 8 12 66.67%
src/logic/safe/store/selectors/txStatus.ts 2 12 16.67%
Files with Coverage Reduction New Missed Lines %
src/components/AppLayout/Header/components/Layout.tsx 2 0%
src/routes/routes.ts 2 93.62%
src/routes/safe/container/index.tsx 5 0%
src/routes/safe/components/Transactions/TxList/hooks/useQueueTransactions.ts 8 55.26%
src/logic/safe/store/reducer/gatewayTransactions.ts 17 2.04%
src/routes/safe/components/Transactions/TxList/TxSingularDetails.tsx 25 1.79%
Totals Coverage Status
Change from base Build 1610904042: 0.06%
Covered Lines: 3093
Relevant Lines: 8523

💛 - Coveralls

@github-actions
Copy link

Deployment links

🟠 Safe Rinkeby Safe Mainnet 🟣 Safe Polygon 🟡 Safe BSC Safe Arbitrum 🟢 Safe xDai

Copy link
Member

@DiogoSoaress DiogoSoaress left a comment

Choose a reason for hiding this comment

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

👌

@github-actions
Copy link

github-actions bot commented Dec 22, 2021

E2E Tests Passed ✅
Check the results here: https://github.com/gnosis/safe-react-e2e-tests/actions/runs/1615801355

Copy link
Member

@katspaugh katspaugh left a comment

Choose a reason for hiding this comment

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

Thanks!

@francovenica
Copy link
Contributor

francovenica commented Dec 23, 2021

Question/Issue:

Is this pending status being persistent only something for the deep-link view? Is working fine for the deep-link view since refreshing the page keeps showing the pending status, but if you refresh the page while being in queue tab the status goes back to "Needs execution" until you open the tx details

Steps:
In a 1 ouf x safe, create a tx but do no execute it (uncheck the Execute checkbox)
See the tx in the queue tab
Now execute it, the pending status will show
Reload the page

The result is that the pending status is lost until you open the tx details:
12-23-2021_x(3821)

@katspaugh
Copy link
Member

No, it was supposed to be for the queued txns as well. I'll check.

@katspaugh
Copy link
Member

@francovenica should be good now. ✅

@katspaugh
Copy link
Member

Screenshot 2021-12-23 at 17 01 24

Tested, works now ✅

@katspaugh
Copy link
Member

One other small bug I noticed though: the sessionStorage (unlike localStorage) somehow isn't shared across tabs in the same browser.
Apparently, it's a known "feature". And there's a workaround: https://stackoverflow.com/a/32766809/352796
IMHO not critical but we can discuss it. cc @iamacook

Gonna go ahead and merge this for now.

@katspaugh katspaugh merged commit c526be5 into dev Dec 23, 2021
@katspaugh katspaugh deleted the persist-local-statuses branch December 23, 2021 16:07
@github-actions github-actions bot locked and limited conversation to collaborators Dec 23, 2021
@iamacook
Copy link
Member Author

And there's a workaround

I think using BroadcastChannel here is much cleaner. What do you think? @katspaugh

@katspaugh
Copy link
Member

Or we use just localStorage and clear it either on status update or on expiry (I would vote for the latter as it will open up other possibilities).

@iamacook
Copy link
Member Author

Both sound like reasonable approaches. Can you make an issue to address this in the new year?

@katspaugh
Copy link
Member

katspaugh commented Dec 24, 2021

Thinking about it further, in our current solution saving the data with an expiration date won't work because we save all the status in one key. It will refresh every time a new tx is made.
I'll make a ticket.
Update: #3223

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.

Persist/sanitise local transaction statuses
5 participants