Skip to content
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

[Web/mWeb] URL / Address bar is not updated after login or landing on root #2325

Closed
2 tasks done
kidroca opened this issue Apr 9, 2021 · 10 comments · Fixed by #2346
Closed
2 tasks done

[Web/mWeb] URL / Address bar is not updated after login or landing on root #2325

kidroca opened this issue Apr 9, 2021 · 10 comments · Fixed by #2346
Assignees
Labels
External Added to denote the issue can be worked on by a contributor

Comments

@kidroca
Copy link
Contributor

kidroca commented Apr 9, 2021

If you haven’t already, check out our contributing guidelines for onboarding and email contributors@expensify.com to request to join our Slack channel!


Expected Result:

When a conversation is loaded it's ID should be a part of the URL (route) in the address bar e.g. expensify.com/r/71403817

Actual Result:

  • After login in the route stays /r even though a conversation is loaded
  • After navigation to the home page / or /r the last conversation is loaded but the route does not update

Action Performed:

Login Flow

  1. Have an account with some conversations
  2. Logout
  3. Reset the address to the root of the app /
    • this might be another issue as after logout the route is not updated as well and keeps displaying the last report path
  4. Login again
  5. Observe your last conversation is selected but the route is just /r and does not contain the ID

Authenticated Landing / Refresh flow

  1. Have an account with some conversations
  2. You should be logged in
  3. Close the tab/window
  4. Open expensify from the root domain e.g. localhost:8080 for local development
  5. Observe your last conversation is selected but the route is just /r and does not contain the ID

Workaround:

Tap on any conversation from the sidebar should load and populate the address bar correctly

Platform:

Where is this issue occurring?

  • Web
  • iOS
  • Android
  • Desktop App
  • Mobile Web

Version Number: 1.0.17-3
Logs: https://stackoverflow.com/c/expensify/questions/4856
Notes/Photos/Videos: Any additional supporting documentation

Expensify.cash.-.Google.Chrome.2021-04-08.21-53-29.mp4

Expensify/Expensify Issue URL: N/A

@kidroca
Copy link
Contributor Author

kidroca commented Apr 9, 2021

Proposal

It should be possible to simplify the logic here

https://github.com/Expensify/Expensify.cash/blob/1359f9fc5906e31975d4d68327d943a6090f6c2c/src/libs/Navigation/NavigationRoot.js#L44-L83

To just

const path = getPathName(initialUrl);
let initialState = getStateFromPath(path, linkingConfig.config);
setCurrentURL(path);

By providing initialParams for the home screen here:

https://github.com/Expensify/Expensify.cash/blob/68169d3a7f1383533d3faae01d0d4589f7cd3887/src/libs/Navigation/AppNavigator/AuthScreens.js#L143-L151

Like this:

<RootStack.Screen
    name="Home"
    options={{
        headerShown: false,
        title: 'Expensify.cash',
    }}
    component={MainDrawerNavigator}
    initialParams={{
        screen: 'Report',
        params: {reportID: this.props.reportID || ''},
    }}
/>

AuthScreens can retrieve any last viewed reportID from Onyx


A alternative is to let the ReportScreen update the route if that's necessary

Common case:

  1. The app starts.
  2. The user ends up on the Report Screen as this is the default home screen.
  3. Attach to the navigation lifecycle (using a listener or via HOC, navigation prop is available as it's a screen component)
  4. When the screen is focused, update the route to the currently viewed report
    • only update navigation state if it doesn't represent the current route.

The app starts from a deep link

  1. The app starts.
  2. A deep link is resolved
  3. The user is navigated to the deep linked screen
  4. Since the user is not ending up on the Home/Report screen - it's not focused so it won't run the logic to override the current route
  5. Because Home is at the root of the stack it will still be appended before the deeplinked path and going back would return you "home", then if the route/path needs updates they will be addressed by the common case logic

Edit: I think just following the guide here can sort things out and would replace the entire componentDidMount logic: https://reactnavigation.org/docs/configuring-links/

  • this is in line with what I'm proposing at the top, so I can start with this

@marcaaron marcaaron added the External Added to denote the issue can be worked on by a contributor label Apr 9, 2021
@MelvinBot
Copy link

Triggered auto assignment to @Christinadobrzyn (External), see https://stackoverflow.com/c/expensify/questions/8582 for more details.

@marcaaron
Copy link
Contributor

Proposal looks good to me if you want to send a PR 👍

@Christinadobrzyn can you create an E/E issues + Upwork job please and hire @kidroca?

We don't need to add the Exported label in this case since it is already triaged.

@MelvinBot
Copy link

Triggered auto assignment to @marcaaron (Engineering), see https://stackoverflow.com/c/expensify/questions/4319 for more details.

@Christinadobrzyn
Copy link
Contributor

Ack! My bad marcaaron I added an engineering label to this, please disregard the ping!

@Christinadobrzyn
Copy link
Contributor

Christinadobrzyn commented Apr 12, 2021

Expensify/Expensify issue - https://github.com/Expensify/Expensify/issues/160016
Job posting in Upwork - https://www.upwork.com/jobs/~01b52f69509ac6e225

Sent a hire request to @kidroca in Upwork

@kidroca
Copy link
Contributor Author

kidroca commented Apr 12, 2021

@Christinadobrzyn
Thank you, I've responded on upwork. I'll post updates here on github

@kidroca
Copy link
Contributor Author

kidroca commented Apr 20, 2021

@Christinadobrzyn
Hey, sorry to bring this up here but I didn't receive a bonus for finding this issue and documenting it in a ticket as per proposing-a-job-that-expensify-hasnt-posted

For example my other ticket did receive a bonus #2109

@Julesssss
Copy link
Contributor

FYI, I've left a note on our internal issue here that we should pay an additional bonus when completing this outstanding Upwork Contract

So this ^ should be taken care of soon.

@Christinadobrzyn
Copy link
Contributor

Thanks for the heads up @Julesssss - I'm paying the contributor today so I'll add the bonus you requested here to the payment.

Contractor paid!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
External Added to denote the issue can be worked on by a contributor
Projects
None yet
Development

Successfully merging a pull request may close this issue.

5 participants