Radish alpha
r
rad:z4D5UCArafTzTQpDZNQRuqswh3ury
Radicle desktop app
Radicle
Git
Refactor the tauri setup hook into individual commands
Open did:key:z6MkkfM3...sVz5 opened 1 year ago

This allows us for onboarding to e.g. fail if there is no profile around and guide new users through identity creation.

Also it makes the tauri run function easier to understand

Also:

  • Update rust toolchain to 1.84
  • Update rust dependencies

checkcheck-unit-testcheck-e2e

👉 Workflow runs 👉 Branch on GitHub

did:key:z6MkkfM3...sVz5 opened with revision 82d6b3e6 on base 9231d3c6 +876 -500 1 year ago

This allows us for onboarding to e.g. fail if there is no profile around and guide new users through identity creation.

Also it makes the tauri run function easier to understand

Also:

  • Update rust toolchain to 1.84
  • Update rust dependencies

checkcheck-unit-testcheck-e2e

👉 Workflow runs 👉 Branch on GitHub

did:key:z6MkkfM3...sVz5 pushed revision 2 f6e85dea on base c35b4a3d +876 -500 1 year ago

Rebase

did:key:z6MkkfM3...sVz5 pushed revision 3 ddb233ed on base c35b4a3d +879 -503 1 year ago

Fix lifetimes for rust 1.84

did:key:z6MkkfM3...sVz5 pushed revision 4 3f7a7c5d on base c35b4a3d +881 -503 1 year ago
did:key:z6MkkfM3...sVz5 pushed revision 5 58c48b5d on base a918981e +881 -503 1 year ago

Rebase

rudolfs reviewed 1 year ago

Is there a good reason we’re doing this from the front-end and not just automatically in the back-end? We could shave off some startup time if it just automatically happens in the back-end.

    // Load profile into state before using it in other commands.
    await invoke<Config>("load_profile");
    await Promise.all([
      invoke("create_inbox_service"),
      invoke("create_patches_service"),
    ]);
did:key:z6MkkfM3...sVz5 commented on revision 1 1 year ago

I don’t think I’m able to reply to a review summary just yet, so doing it from here.

The main goal of this patch is to reduce the load we are putting on the setup hook and try to initiate the state by commands from the front-end.

Reasons being:

  1. Having control over how and when we try to load a profile, and what to do front-end wise if that fails (e.g. due to being a new user)
  2. If it fails the first time, being able to retry to load the profile without having to restart the app. 2.1. For a new user after the user creates their identity we would somewhere need to load the profile into the app state, which could be the load_profile command

It would be possible to do it in the setup hook, but we would need tauri commands that copy the setup hook for retrying loading a profile.

How much do you think we could shave off from the start up time?

rudolfs commented on revision 1 1 year ago

Ok, so if I understand correctly there would be additional retry logic in the front-end around the load_profile call, that makes sense to me. But would we ever need to manually control the calls to invoke("create_inbox_service"),invoke("create_patches_service") from the front-end?

Start up time is one thing. Somehow the calls without any other logic seem to me like a code smell, but maybe I’m missing something that you’re planning for later.

did:key:z6MkkfM3...sVz5 commented on revision 1 1 year ago

By moving the load_profile to the front-end, we won’t be able to do the create_*_service logic in the back-end since we don’t have the profile ready in the backend until the load_profile command is invoked. The create_*_service calls depend on Profile being in the back-end state, so it needs to be called after we successfully loaded the profile in the front-end. I don’t think we can run code in the back-end without some call from the front-end triggering it. (besides the setup hook)

If we don’t want to have separate create_*_service commands, we could also do a setup_services command that would do both, since we probably won’t call one of them without the other. Another idea could be to emit a profile_loaded event to the back-end once load_profile has been successfull, which would create the services in the back-end

EDIT: ok pushed a new revision with the profile_loaded event

did:key:z6MkkfM3...sVz5 pushed revision 6 6f1f226c on base 4add00d6 +418 -156 1 year ago

Remove create_*_service commands and put them into a profile-loaded event listener.

This allows us to have the create service calls in the back-end, and once more stuff comes along that we want to do once the profile is loaded we can add that into the profile loaded closure

did:key:z6MkkfM3...sVz5 pushed revision 7 8716d8ae on base 4add00d6 +420 -156 1 year ago

Fix failing test

did:key:z6MkkfM3...sVz5 pushed revision 8 e31f3a49 on base 4add00d6 +419 -156 1 year ago

Fix typescript check

Description from revision 6f1f226

Remove create_*_service commands and put them into a profile-loaded event listener.

This allows us to have the create service calls in the back-end, and once more stuff comes along that we want to do once the profile is loaded we can add that into the profile loaded closure

checkcheck-unit-testcheck-e2e

👉 Workflow runs 👉 Branch on GitHub

did:key:z6MkkfM3...sVz5 pushed revision 9 b3cec709 on base 15bb0353 +419 -156 1 year ago

Rebase

rudolfs commented on revision 1 1 year ago

Could we just have one command that does everything - loads the profile and sets up the services? We could separate them later if we need. To me the event from the front-end doesn’t make sense, because the backend already should know when the load profile call finishes. At the very least we can move emitting the event to being fired from the load_profile method in the back-end.

did:key:z6MkkfM3...sVz5 commented on revision 1 1 year ago

I don’t like to have the profile loading in one command with the services setup since they aren’t really related, and having everything in one command seem not very concern separation

rudolfs commented on revision 1 1 year ago

As discussed via call: maybe we keep the profile_loaded command separate and have another command in the backend like setup that calls the profile and other initialization commands? You also mentioned that coupling the onboarding to the front-end breaks some of the back-end tests otherwise.

But if you strongly disagree and prefer to do it this way then we can also merge this and see if we change it later on.