Expensify / App

Welcome to New Expensify: a complete re-imagination of financial collaboration, centered around chat. Help us build the next generation of Expensify by sharing feedback and contributing to the code.
https://new.expensify.com
MIT License
3.55k stars 2.9k forks source link

[HybridApp] Investigate improvement to HybridApp TTI #49844

Open Julesssss opened 1 month ago

Julesssss commented 1 month ago

P/S authored by @staszekscp here

Problem

The TTI on HybridApp is longer than on a standalone NewDot application. In order to determine if a specific user should go to NewDot, HybridApp has to boot OldDot code first. The business logic for OldDot was written in JS in a framework called YAPL, and it runs on JSC.

Solution

Let’s replace JSC with Hermes! Hermes is known for its advantage over JSC in initial boot time, and it became a standard in the React Native world. Since YAPL is written in JS, theoretically should be able to do that, and get rid of JSC from the project. As bonus the app size would become smaller, too. :muscle::skin-tone-2: :rocket:

However, I’d like to emphasise that, since YAPL has been written in legacy JavaScript (mostly pre ES2015) there is a chance that the migration may become very time-consuming. Nevertheless I think it is worth to do at least a research, and see how far we can get, because it will speed up the both, OldDot, and HybridApp!

Mateusz' updated Solution

Adjust the current authentication flow on HybridApp to replace the OldDot sign-in page with the NewDot version.

  1. Currently, we perform authentication on the OldDot side and pass all necessary credentials to enable the New Experience and handle authentication/re-authentication. We could reverse this by using the NewDot sign-in page to generate all required credentials. These credentials could then be used to authenticate on the OldDot side. To trigger authentication on YAPL side, native methods can be used.
  2. We also need to decide at the start of sign-in whether the user should be redirected to OldDot or NewDot. The SignInUser command should return an additional field with nvp_tryNewDot, allowing us to determine based on the value of the dismissed key whether to stay on NewDot or transition to OldDot.
  3. Another important aspect from the user’s perspective is error handling. OldDot initialisation might fail, so we should be able to display errors on the SignIn page, just as we do when NewDot authentication fails. We can track OldDot initialisation through native events and store the current state in Onyx. Errors returned by OldDot’s API can then be passed via these events and displayed on the NewDot SignIn page
Issue OwnerCurrent Issue Owner: @staszekscp
staszekscp commented 1 month ago

Hey! With @WoLewicki we'll be working on this issue!

Julesssss commented 1 month ago

Work is ongoing.

Julesssss commented 3 weeks ago

In progress

melvin-bot[bot] commented 2 weeks ago

This issue has not been updated in over 15 days. @Julesssss, @staszekscp eroding to Monthly issue.

P.S. Is everyone reading this sure this is really a near-term priority? Be brave: if you disagree, go ahead and close it out. If someone disagrees, they'll reopen it, and if they don't: one less thing to do!

Julesssss commented 1 week ago

Hey @staszekscp & @WoLewicki. It sounds like you've been making progress here.

Let's use this issue to track the plan and the backend changes that you'll need me to implement. I'm currently struggling to find the 3 point plan that was shared recently. I asked here and will update this issue once reviewed.

Julesssss commented 1 week ago

I added Mateusz' most recent Solution to the issue description.

In order to return the value of nvp_tryNewDot I think I would need to return it from both SigninUser and SigninUserWithLink (for magic links). I'm going to look into this this afternoon.

Another thought is whether the full NVP would be useful, or if we should just return a boolean determining the desired experience.

melvin-bot[bot] commented 4 days ago

@Julesssss, @staszekscp 6 days overdue. This is scarier than being forced to listen to Vogon poetry!

Julesssss commented 4 days ago

I have started to submit the WIP PRs for the following backend change that will start returning the necessary field:

We also need to decide at the start of sign-in whether the user should be redirected to OldDot or NewDot. The SignInUser command should return an additional field with nvp_tryNewDot, allowing us to determine based on the value of the dismissed key whether to stay on NewDot or transition to OldDot.

melvin-bot[bot] commented 3 days ago

@Julesssss, @staszekscp 6 days overdue. This is scarier than being forced to listen to Vogon poetry!

war-in commented 3 days ago

Hi :wave:

While working on the new sign-in page for HybridApp, I encountered a situation I'd like your input on.

When the nvp is set to open ND, we sign the user into the new app and simultaneously attempt an OD sign-in in the background to expedite the process. However, the OD sign-in can occasionally fail, leaving us in a state where the ND app is active, but OD is not signed in.

If the user decides to switch back to the classic experience, we must reattempt the OD sign-in, which can take a few seconds.

Given this scenario, what would be the best user experience? Should we display a new page notifying them of the ongoing switch, or (my personal preference) show a loading indicator next to the button (as shown in the screen below)?

Any input is highly appreciated :pray:

image cc @Expensify/design

melvin-bot[bot] commented 3 days ago

Triggered auto assignment to @dannymcclain (Design), see these Stack Overflow questions for more details.

dannymcclain commented 3 days ago

show a loading indicator next to the button (as shown in the screen below)

@war-in are you talking about something like this screenshot? (Grabbed it from the related Slack thread)

image

I think something like that is good—and I think we agree it'd be a lot nicer to just add a little loading indicator than take the user to a new page. Let's just make sure we're using our standard spinner/loader component for that little guy.

Julesssss commented 3 days ago

Hey @dannymcclain, I'm struggling to locate an example of a loading spinner in Figma or within the codebase, would you mind sharing an example so we can find the existing component? Thanks

dannymcclain commented 3 days ago

I'm honestly not sure that we have super good examples in Figma. And I know it's appearance changes a little bit between web and native apps...

This is what we have in Figma currently (look for Loader / Spinner):

CleanShot 2024-11-12 at 08 23 08@2x

@Expensify/design can either of you think of any recent examples of us implementing the spinner that Jules could check out? I'm coming up blank!

Julesssss commented 3 days ago

Ah yes I immediately remember that now I see it 😄

I think FullScreenLoadingIndicator is where we would start. It might even be the default React native ActivityIndicator component

war-in commented 3 days ago

It might even be the default React native ActivityIndicator component

Actually, I used ActivityIndicator in my example but I'll double check before creating a PR

Julesssss commented 3 days ago

Oh, cool, maybe it's something to do with theme.spinner. Anyway, let us know if you need any more help

war-in commented 2 days ago

And another question on the Switch to Expensify Classic button.

What should we do when the API call fails again after clicking the button? Should we show some error message under the button?

image

Or maybe an error icon?

image

However, both have disadvantages:

https://github.com/user-attachments/assets/0a42f04e-f1c7-44fc-961c-7c860d4c2359

Please share your thoughts 🙏

cc @Julesssss @dannymcclain

dannymcclain commented 2 days ago

Going to cc in the @Expensify/design team for thoughts on this. I wonder if it would be better to just pop a little modal if there's an error letting the user know what happened.

shawnborton commented 2 days ago

Either that, or some kind of dismissible RBR error message right under the option row could work too? That would be pretty consistent with the rest of NewDot.

dannymcclain commented 1 day ago

Yeah I could see that too. 🤔

Here's a dismissible error in the LHN and a modal. How are we feeling @Expensify/design?

image

Obviously we would get the proper copy for either approach. 😅

shawnborton commented 1 day ago

Thanks for sharing these. After seeing both, it feels like the centered dialog feels more appropriate here? What do you think?

dubielzyk-expensify commented 1 day ago

+1 to dialog

dannymcclain commented 1 day ago

Yeah I think the dialog feels cleaner.

Julesssss commented 1 day ago

Quick update about the backend changes. The first PRs has been merged and the second is in review. They should be live early next week. The flag will be tryNewDot:

Screenshot 2024-11-14 at 15 31 07