fix: use location.state to store background route to prevent unmount #4062

pete-watters · 2023-07-26T13:49:12Z

Try out this version of the Hiro Wallet - download extension builds.

This PR makes a change to how we handle modals displayed over routes.

location.state is used to store the background route to prevent it being unmounted by feeding that to the router if available

pete-watters · 2023-07-26T13:51:59Z

src/app/pages/home/home.tsx

@@ -47,7 +52,13 @@ function HomeContainer({ account }: HomeContainerProps) {
      actions={<HomeActions />}
    >
      <HomeTabs>
-        <Outlet context={{ address: account.address }} />
+        <>


I need to declare Routes here so I can manipulate the location depending on whether backgroundLocation is populated or not.

backgroundLocation is the route above which the modal should appear.

Isn't it possible to also read location in app-routes to see if bg location is defined? Nw if not, just curious

We can read the location there but the issue I struggled with is how to then feed said location into how we are creating the routes.

We are using createHashRouter and createRoutesFromElements and I was unable to figure out how to pass the location there.

Eventually I came up with the idea of using <Routes nested in <Home so I could then trick the location.

We can probably improve on that structure with more time and I am open to suggestions but I didn't want to start refactoring the whole of app-routes

pete-watters · 2023-07-26T14:00:25Z

src/app/routes/app-routes.tsx

@@ -186,8 +185,9 @@ function useAppRoutes() {
            </AccountGate>
          }
        >
-          <Route index element={<BalancesList />} />
-          <Route path={RouteUrls.Activity} element={<ActivityList />} />
+          <Route path={RouteUrls.Activity} element={<ActivityList />}>


ActivityList is loaded in home.tsx but the route needs to be declared here also.

Similarly settingsModalRoutes needs to be passed in so you can use the settings modals from /activities

kyranjamie · 2023-07-26T14:50:15Z

src/app/pages/home/home.tsx

@@ -47,7 +52,13 @@ function HomeContainer({ account }: HomeContainerProps) {
      actions={<HomeActions />}
    >
      <HomeTabs>
-        <Outlet context={{ address: account.address }} />
+        <>


Isn't it possible to also read location in app-routes to see if bg location is defined? Nw if not, just curious

src/app/pages/receive-tokens/receive-modal.tsx

…using modals, closes #4028

src/app/common/hooks/use-location-state.ts

…undLocation as home

pete-watters · 2023-07-28T14:04:01Z

src/app/common/hooks/use-background-location-redirect.ts

@@ -0,0 +1,34 @@
+import { useEffect } from 'react';


I added this hook so that:

If you have a modal open

Open a new tab

It will update the backgroundLocation to be the home page

Kapture.2023-07-28.at.14.28.19.mp4

Without doing this, an empty screen appears behind the modal

Thinking on this further: couldn't each modal component handle their own backgroundLocation logic?

Where the sign out modal component has a check in its render that, if there's no backgroundLocation, it should re-router to having a default value of home?

Could work with a similar hook API, but where all the logic is colocated to the modal itself, rather than one big catch-all.

Something like:

// src/app/pages/sign-out-confirm/sign-out-confirm.tsx useDefaultBackgroundLocation(RouteUrls.Home)

which internally handles the

if (!backgroundLocation) navigate(currentRoute, { state: { backgroundLocation: defaultVal, // this might fix the problem you've commented in the other file re route state ...otherRouteState } })`

Yes we can definitely do that. I just implemented a catch all for now as Home seems like the logical background. I can change it to accept a preferred default route

I can give it a try but I think once you open a new tab the location.state gets wiped completely.

pete-watters · 2023-07-28T14:06:08Z

src/app/common/hooks/use-background-location-redirect.ts

+      switch (true) {
+        // FIXME ReceiveCollectibleOrdinal loses state?.btcAddressTaproot in a new tab
+        // this can be improved to try and fetch btcAddressTaproot
+        case pathname === RouteUrls.ReceiveCollectibleOrdinal && !state?.btcAddressTaproot:


For RouteUrls.ReceiveCollectibleOrdinal, it relies on btcAddressTaproot that is set in location.state.

If you open a new tab this state is lost and although the modal opens, the btcAddressTaproot is lost so it's unusable.

For now I just send users back to RouteUrls.Home. This could probably be improved by refactoring how we get btcAddressTaproot in ReceiveCollectibleOrdinal

Not a big fan of using a switch(true). Why not just use ifs?

This is better for readability imo. My thoughts map to the first comment of the HN post of a guy who wrote an article doing this (https://news.ycombinator.com/item?id=26777090) re favouring early return.

Separately—while identical behaviour of modals when doing a fresh load of the app would be ideal—if we need to add a lot of logic to handle this, then I wouldn't consider it a dealbreaker if the under layer doesn't load correctly. Contextually, in case of sign out, it isn't so important.

I like using switch(true) as I think its cleaner and more elegant than a lot of if statements. I can change it though and use early returns here and in future to match the code base.

If we are OK with having an empty screen behind all modals opened in a new tab then we can avoid the addition of this logic.

It will look like this, with home tabs but no tab content:

For RouteUrls.ReceiveCollectibleOrdinal we will need to do something though as we lose track of the taproot address when opening in a new tab and the modal shows but is non-functional as there is no address to copy:

pete-watters · 2023-07-28T14:49:01Z

This PR is working and I tested it a lot but it would be good to give it a thorough look.

It still throws a warning in the console due to how I have had to use <Routes nested in home.tsx. It could be something easily fixed that I missed but I'm not sure.

I did some more work refactoring app-routes to use <HashRouter instead of createHashRouter and createRoutesFromElements to try and improve this but I will exclude it from this PR. I'm not sure if we need React Router v6.4 Data APIs. If not this could be a cleaner solution if I can get it to work.

The stumbling block is that we need to use <Routes to be able to pass a dynamic location and to trick the router into thinking we are on the same location when showing a modal above it.

kyranjamie · 2023-07-31T09:53:52Z

src/app/common/hooks/use-background-location-redirect.ts

+      switch (true) {
+        // FIXME ReceiveCollectibleOrdinal loses state?.btcAddressTaproot in a new tab
+        // this can be improved to try and fetch btcAddressTaproot
+        case pathname === RouteUrls.ReceiveCollectibleOrdinal && !state?.btcAddressTaproot:


Not a big fan of using a switch(true). Why not just use ifs?

This is better for readability imo. My thoughts map to the first comment of the HN post of a guy who wrote an article doing this (https://news.ycombinator.com/item?id=26777090) re favouring early return.

Separately—while identical behaviour of modals when doing a fresh load of the app would be ideal—if we need to add a lot of logic to handle this, then I wouldn't consider it a dealbreaker if the under layer doesn't load correctly. Contextually, in case of sign out, it isn't so important.

kyranjamie · 2023-07-31T09:58:26Z

src/app/common/hooks/use-location-state.ts

+export function useLocationState(propName: 'accountIndex'): number;
+export function useLocationState(propName: 'backgroundLocation'): Location;


These changes mean useLocationState is now aware of implementation details of some of the cases in which it's used.

Why not extend the function to support a generic type argument, rather than extending it with use-case specific typings?

I can change this, no problem. I struggled with extending the type initially so opted for this to narrow the type to their key. I'll clean it up

…d routes which are needed for modal

pete-watters · 2023-08-04T14:59:23Z

Closing this in favour of : #4099

pete-watters commented Jul 26, 2023

View reviewed changes

pete-watters force-pushed the fix/4028/send-inscription-routing-clean branch from 716b35a to ca60339 Compare July 26, 2023 13:54

vercel bot temporarily deployed to Preview July 26, 2023 13:54 Inactive

pete-watters force-pushed the fix/4028/send-inscription-routing-clean branch from ca60339 to 498742b Compare July 26, 2023 13:54

vercel bot temporarily deployed to Preview July 26, 2023 13:55 Inactive

pete-watters commented Jul 26, 2023

View reviewed changes

kyranjamie reviewed Jul 26, 2023

View reviewed changes

pete-watters force-pushed the fix/4028/send-inscription-routing-clean branch from 498742b to a78237a Compare July 26, 2023 14:53

vercel bot temporarily deployed to Preview July 26, 2023 14:53 Inactive

vercel bot temporarily deployed to Preview July 26, 2023 15:20 Inactive

vercel bot temporarily deployed to Preview July 26, 2023 15:54 Inactive

pete-watters force-pushed the fix/4028/send-inscription-routing-clean branch from f31f274 to 5f39481 Compare July 27, 2023 08:11

vercel bot temporarily deployed to Preview July 27, 2023 08:11 Inactive

pete-watters force-pushed the fix/4028/send-inscription-routing-clean branch from 5f39481 to d0b9454 Compare July 27, 2023 08:30

vercel bot temporarily deployed to Preview July 27, 2023 08:34 Inactive

fix: use location.state to store background route to prevent unmount …

c269825

…using modals, closes #4028

pete-watters force-pushed the fix/4028/send-inscription-routing-clean branch from d0b9454 to c269825 Compare July 27, 2023 08:42

vercel bot temporarily deployed to Preview July 27, 2023 08:42 Inactive

pete-watters added 2 commits July 27, 2023 09:58

fix: use useLocationState for cleaner code, closes #4028

5c0e4a3

fix: use useLocationState for cleaner code, closes #4028

de2e7c5

vercel bot temporarily deployed to Preview July 27, 2023 09:06 Inactive

fix: WIP with catchall route to handle opening in new tab, closes #4028

406fdd1

pete-watters commented Jul 27, 2023

View reviewed changes

src/app/common/hooks/use-location-state.ts Show resolved Hide resolved

vercel bot temporarily deployed to Preview July 27, 2023 13:16 Inactive

fix: WIP allow modals to be opened in a new tab

b05ef6b

vercel bot temporarily deployed to Preview July 27, 2023 15:18 Inactive

fix: reinstate overloads and update types

b9f772b

vercel bot temporarily deployed to Preview July 28, 2023 08:34 Inactive

fix: gracefully handle opening modals in a new tab by setting backgro…

364a7cd

…undLocation as home

vercel bot temporarily deployed to Preview July 28, 2023 13:34 Inactive

pete-watters commented Jul 28, 2023

View reviewed changes

pete-watters marked this pull request as ready for review July 28, 2023 14:43

pete-watters requested review from kyranjamie, fbwoolf and alter-eggo July 28, 2023 14:58

kyranjamie reviewed Jul 31, 2023

View reviewed changes

pete-watters mentioned this pull request Jul 31, 2023

Fix state change behavior upon selecting "Send" for inscription #4028

Closed

pete-watters linked an issue Jul 31, 2023 that may be closed by this pull request

Fix state change behavior upon selecting "Send" for inscription #4028

Closed

fix: restructure home routes as absolute paths will not work in neste…

2d5b02b

…d routes which are needed for modal

pete-watters linked an issue Aug 2, 2023 that may be closed by this pull request

Wallet always opens to Balances tab #4086

Closed

vercel bot temporarily deployed to Preview August 4, 2023 10:59 Inactive

pete-watters mentioned this pull request Aug 4, 2023

Fix/4028/wallet modal routing #4099

Closed

pete-watters closed this Aug 4, 2023

pete-watters mentioned this pull request Oct 9, 2023

Fix/4028/leather wallet modal routing #4325

Merged

pete-watters deleted the fix/4028/send-inscription-routing-clean branch February 7, 2024 09:27

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

fix: use location.state to store background route to prevent unmount #4062

fix: use location.state to store background route to prevent unmount #4062

pete-watters commented Jul 26, 2023 •

edited by github-actions bot

Loading

pete-watters Jul 26, 2023

kyranjamie Jul 26, 2023

pete-watters Jul 26, 2023

pete-watters Jul 26, 2023

kyranjamie Jul 26, 2023

pete-watters Jul 28, 2023

kyranjamie Jul 31, 2023 •

edited

Loading

pete-watters Jul 31, 2023

pete-watters Jul 28, 2023

kyranjamie Jul 31, 2023

pete-watters Jul 31, 2023

pete-watters commented Jul 28, 2023

kyranjamie Jul 31, 2023

kyranjamie Jul 31, 2023

pete-watters Jul 31, 2023

pete-watters commented Aug 4, 2023

		export function useLocationState(propName: 'accountIndex'): number;
		export function useLocationState(propName: 'backgroundLocation'): Location;

fix: use location.state to store background route to prevent unmount #4062

fix: use location.state to store background route to prevent unmount #4062

Conversation

pete-watters commented Jul 26, 2023 • edited by github-actions bot Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

kyranjamie Jul 31, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

pete-watters commented Jul 28, 2023

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

pete-watters commented Aug 4, 2023

pete-watters commented Jul 26, 2023 •

edited by github-actions bot

Loading

kyranjamie Jul 31, 2023 •

edited

Loading