fix: Sync current safe address into store #3761

usame-algan · 2022-04-05T11:18:00Z

What it solves

Resolves #3760

How this PR fixes it

Instead of relying on the currentSafe selector, we handle the logic inside the component by getting all safes from the store, extracting the current safe address from the url and finally selecting the current safe.

How to test it

Open a Safe
Switch to a different Safe
Observe that Safe name, address, balance, icon updates in the sidebar

github-actions · 2022-04-05T11:18:15Z

CLA Assistant Lite All Contributors have signed the CLA.

iamacook · 2022-04-05T11:21:13Z

src/components/App/index.tsx

  const { safeActionsState, onShow, onHide, showSendFunds, hideSendFunds } = useSafeActions()
  const currentCurrency = useSelector(currentCurrencySelector)
  const granted = useSelector(grantedSelector)
  const sidebarItems = useSidebarItems()
-  const { address: safeAddress } = useSelector(currentSafeWithNames)
+  const safeAddress = extractSafeAddress()


I thought the issue stemmed from this function? I would be apprehensive to rely on this. It feels as though the Safe may not be rendering when the Safe address in the URL changes. How does a different selector fix this?

It would be better to fix the root cause: the selector.

extractSafeAddress is not a selector. The issue is that we call extractSafeAddress inside the currentSafe selector which apparently uses a cached value instead of always calling extractSafeAddress again. Having the call inside the component ensures that it will always extract the current safe address when rerendering.

I know it's not a selector. It's not triggering a rerender because it isn't. I would much prefer we fix the currentSafe selector instead of hackily selecting the data we want as a workaround. The current fix is relying on other selectors to re-render the component, hence having the latested value returned from extractSafeAddress.

github-actions · 2022-04-05T11:21:29Z

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	0	N/A

Result: ✅ success
Annotations: 0 total

^{Report generated by eslint-plus-action}

github-actions · 2022-04-05T11:21:40Z

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	0	N/A

Result: ✅ success
Annotations: 0 total

^{Report generated by eslint-plus-action}

github-actions · 2022-04-05T11:24:54Z

Deployment links

🟠 Rinkeby	⚪ Mainnet	🟣 Polygon	🟡 BSC	⚫ Arbitrum	🟢 Gnosis Chain

coveralls · 2022-04-05T11:26:52Z

Pull Request Test Coverage Report for Build 2201465937

6 of 15 (40.0%) changed or added relevant lines in 6 files are covered.
1 unchanged line in 1 file lost coverage.
Overall coverage decreased (-0.009%) to 35.546%

Changes Missing Coverage	Changed/Added Lines	%
src/components/App/index.tsx	1	0.0%
src/logic/currentSession/store/reducer/currentSession.ts	3	0.0%
src/logic/currentSession/hooks/useCurrentSafeAddressSync.ts	5	0.0%

Files with Coverage Reduction	New Missed Lines	%
src/components/App/index.tsx	1	0%

Totals
Change from base Build 2195129238:	-0.009%
Covered Lines:	3558
Relevant Lines:	9056

💛 - Coveralls

iamacook

Really nice approach! I've left a few comments.

src/logic/currentSession/hooks/useCurrentSafeAddressSync.ts

src/logic/currentSession/store/reducer/currentSession.ts

iamacook · 2022-04-13T08:44:05Z

src/logic/safe/store/selectors/index.ts

-    return safes.get(address, baseSafe(address)) ?? {}
-  },
-)
+export const currentSafeAddress = (state: AppReduxState): string => state[CURRENT_SESSION_REDUCER_ID].currentSafeAddress


This should live in the folder for currentSession selectors. You could reselect the currentSession state selector as the basis for this as well.

Good catch! Will move it to the other folder.

iamacook · 2022-04-13T08:45:53Z

Note: once we've merged this. We should remove all instances of extractSafeAddress() where we can readily use a selector.

…ess selector

github-actions · 2022-04-20T15:02:10Z

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	2	N/A

Result: ✅ success
Annotations: 0 total

^{Report generated by eslint-plus-action}

usame-algan · 2022-04-20T15:14:44Z

Note: once we've merged this. We should remove all instances of extractSafeAddress() where we can readily use a selector.

Added a new issue #3801 to address this.

src/logic/currentSession/hooks/useCurrentSafeAddressSync.ts

iamacook

🚀

francovenica · 2022-04-20T19:13:50Z

Looks good to me.
Switched between safes, the ones added by "Load safe" and the ones in the "Safe owned" list and also between networks. The sidebar updates right away.

katspaugh · 2022-04-21T06:02:59Z

@francovenica could you test one more time after clearing and disabling your cache and throttling the network to Slow 3G?

src/logic/currentSession/hooks/useCurrentSafeAddressSync.ts

src/logic/safe/store/reducer/gatewayTransactions.ts

usame-algan · 2022-04-25T08:52:37Z

@francovenica could you test one more time after clearing and disabling your cache and throttling the network to Slow 3G?

Also tested this successfully. The safe address in the sidebar immediately changes. Other values like balance take longer to update which is expected.

katspaugh

Good to go then! 🚀

usame-algan requested review from katspaugh and iamacook April 5, 2022 11:18

usame-algan force-pushed the fix-switch-safe-sidebar branch from 808b462 to f4474b6 Compare April 5, 2022 11:18

iamacook reviewed Apr 5, 2022

View reviewed changes

iamacook reviewed Apr 13, 2022

View reviewed changes

usame-algan added 2 commits April 20, 2022 16:25

fix: Get current safe address outside of selector

19b3026

fix: Sync current safe address to store on location change

3ca4a00

usame-algan force-pushed the fix-switch-safe-sidebar branch from 917c279 to 3ca4a00 Compare April 20, 2022 14:58

fix: Use history.location instead of location, move current safe addr…

041b46b

…ess selector

refactor: Add currentSession selector

e7f3a2b

usame-algan requested a review from iamacook April 20, 2022 15:14

katspaugh reviewed Apr 20, 2022

View reviewed changes

src/logic/currentSession/hooks/useCurrentSafeAddressSync.ts Outdated Show resolved Hide resolved

fix: Failing tests

bda306f

iamacook reviewed Apr 20, 2022

View reviewed changes

iamacook approved these changes Apr 20, 2022

View reviewed changes

refactor: useLocation for safe address sync

8f3305d

usame-algan force-pushed the fix-switch-safe-sidebar branch from 2909e1f to 8f3305d Compare April 20, 2022 16:28

katspaugh reviewed Apr 21, 2022

View reviewed changes

src/logic/currentSession/hooks/useCurrentSafeAddressSync.ts Outdated Show resolved Hide resolved

katspaugh reviewed Apr 21, 2022

View reviewed changes

src/logic/safe/store/reducer/gatewayTransactions.ts Outdated Show resolved Hide resolved

fix: Adjust custom hook dependency

5dbb701

usame-algan changed the title ~~fix: Get current safe address outside of selector~~ fix: Sync current safe address into store Apr 21, 2022

fix: Use LabelValue enum

3bc40ea

katspaugh approved these changes Apr 25, 2022

View reviewed changes

usame-algan merged commit 71bd092 into dev Apr 25, 2022

usame-algan deleted the fix-switch-safe-sidebar branch April 25, 2022 14:35

github-actions bot locked and limited conversation to collaborators Apr 25, 2022

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

fix: Sync current safe address into store #3761

fix: Sync current safe address into store #3761

usame-algan commented Apr 5, 2022

github-actions bot commented Apr 5, 2022

iamacook Apr 5, 2022

iamacook Apr 5, 2022

usame-algan Apr 5, 2022

iamacook Apr 5, 2022

github-actions bot commented Apr 5, 2022

github-actions bot commented Apr 5, 2022

github-actions bot commented Apr 5, 2022

coveralls commented Apr 5, 2022 •

edited

Loading

iamacook left a comment

iamacook Apr 13, 2022

usame-algan Apr 20, 2022

iamacook commented Apr 13, 2022

github-actions bot commented Apr 20, 2022 •

edited

Loading

usame-algan commented Apr 20, 2022

iamacook left a comment

francovenica commented Apr 20, 2022

katspaugh commented Apr 21, 2022

usame-algan commented Apr 25, 2022

katspaugh left a comment

fix: Sync current safe address into store #3761

fix: Sync current safe address into store #3761

Conversation

usame-algan commented Apr 5, 2022

What it solves

How this PR fixes it

How to test it

github-actions bot commented Apr 5, 2022

iamacook Apr 5, 2022

Choose a reason for hiding this comment

iamacook Apr 5, 2022

Choose a reason for hiding this comment

usame-algan Apr 5, 2022

Choose a reason for hiding this comment

iamacook Apr 5, 2022

Choose a reason for hiding this comment

github-actions bot commented Apr 5, 2022

ESLint Summary View Full Report

github-actions bot commented Apr 5, 2022

ESLint Summary View Full Report

github-actions bot commented Apr 5, 2022

Deployment links

coveralls commented Apr 5, 2022 • edited Loading

Pull Request Test Coverage Report for Build 2201465937

💛 - Coveralls

iamacook left a comment

Choose a reason for hiding this comment

iamacook Apr 13, 2022

Choose a reason for hiding this comment

usame-algan Apr 20, 2022

Choose a reason for hiding this comment

iamacook commented Apr 13, 2022

github-actions bot commented Apr 20, 2022 • edited Loading

ESLint Summary View Full Report

usame-algan commented Apr 20, 2022

iamacook left a comment

Choose a reason for hiding this comment

francovenica commented Apr 20, 2022

katspaugh commented Apr 21, 2022

usame-algan commented Apr 25, 2022

katspaugh left a comment

Choose a reason for hiding this comment

coveralls commented Apr 5, 2022 •

edited

Loading

github-actions bot commented Apr 20, 2022 •

edited

Loading