NEOS-1254: update onboarding flow

[evisdrenova]

I've removed the onboarding checklist in favor of a welcome modal. The welcome modal will pop up for anyone who hasn't either completed it or skipped it. I was considering keeping the onboarding checklist but I didn't think it made much sense. If someone wants to skip the welcome modal, it's likely they don't want the onboarding checklist either. If someone completes the welcome modal, then they would go right into the job creation flow which would pretty much just do everything that the onboarding checklist wanted (with the exception of inviting other people but I don't htink thats a big deal).

As part of this change, I've updated the AccountOnboardingConfig and removed a lot of the code that drove updating the onboarding checklist. For existing users, the pop-up will show since the function looks for the hasCompletedOnboarding key in the onboarding_config and that won't be present. This would mean that existing users who have already dismissed or completed the onboardingChecking will see the welcome modal pop-up and they can either skip it or just click through it. If we didn't want this then there are probably two options: 1. Keep the onboarding_config object as-is and just add on a new key, then update code to manage this where it checks for the presence of the current onboarding_config and then updates the hasCompletedOnboarding value, or 2. leave it as-is and just let people skip or complete the new modal.

Thoughts @nickzelei

Update:

• Added a loom video showing the flow. Clicking on the records button in the tab takes you to the usage page but running it locally and don't have usage so doesn't load but path is correct.
  https://www.loom.com/share/8ba9a12b4ad2441e8d8cdb98e6d5f3c1?sid=33d5b78b-9443-4733-ba9c-388ad3e48b9c

[linear]

NEOS-1254 Build new onboarding flow for new users

[vercel]

The latest updates on your projects. Learn more about Vercel for Git ↗︎

1 Skipped Deployment

Name	Status	Preview	Comments	Updated (UTC)
neosync-docs	⬜️ Ignored (Inspect)	Visit Preview		Oct 1, 2024 9:01pm

[codecov]

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 36.81%. Comparing base (a535d10) to head (ed18235).
Report is 2 commits behind head on main.

Additional details and impacted files

@@            Coverage Diff             @@
##             main    #2352      +/-   ##
==========================================
- Coverage   36.83%   36.81%   -0.03%     
==========================================
  Files         285      285              
  Lines       26774    26768       -6     
==========================================
- Hits         9862     9854       -8     
  Misses      15610    15610              
- Partials     1302     1304       +2     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

[github-actions]

The latest Buf updates on your PR. Results from workflow Buf / buf (pull_request).

Build	Format	Lint	Breaking	Updated (UTC)
✅ passed	✅ passed	✅ passed	✅ passed	Oct 1, 2024, 9:01 PM

[nickzelei]

OK there is a lot here, this is a big PR.

I didnt seen a loom demo so maybe you could attach one or just show me what this looks like tomorrow.

I left a number of comments. A little sketched out by some of the screenshots that I think are going to be present in the onboarding flow. It would be better I think if we could just use the components instead so they don't get out of date.

Either way I think this is enough for a first pass (might have comments on the welcome board itself..didnt really look at that at all with all of the other comments i left in this PR. Maybe we can just talk about that offline.

[nickzelei]

I think we only want to show this if it is neosync cloud

[evisdrenova]

yeah just had this flipped so i could actually test it locally - will reset

[nickzelei]

it is probably better to pass in the case as well as the identifier here. it's not obvious from this interface that it needs to be account id specific. Either that or make this component specific to accounts.

[evisdrenova]

updated with a new component that acts as a wrapper

[nickzelei]

Why do we want to change this to only show if auth is enabled? This will cause it to not show for the pure OSS users, which is what we wanted?

[evisdrenova]

updated

[nickzelei]

Why is this flipped?

[evisdrenova]

same as above - was just for local testing to see the layout - will flip

[nickzelei]

I've been trying to keep the SiteHeader a server-side component. This is needed for line 29 to work properly where we call getSystemAppConfig() directly instead of via the hook.
It's not a big deal if we need to change this, but it will cause a delay in rendering of the site header because we have to call the config on the client side now (or pass it in from the higher level component, but thats kinda out of scope for this PR.)

[evisdrenova]

yeah agreed - reverted back to SSR and moved the records progress stuff to <AccountsRecordProgress.tsx />

[nickzelei]

Was thinking about suggesting that we move away form this numbered system and instead use a narrow string type and transition to different states. That would make this code a lot more readable. So instead of doing "currentStep + 1) it's instead setStep('welcome') setStep('connect') etc.

So instead of having FormStep.name be a wide string, instead make it something like this:

type FormStepName = "welcome" | "connect" | "configure" | "execute" | "router";
export interface FormStep {
  name: FormStepName;
  component: JSX.Element;
}

Then the currentStep becomes:

const [currentStep, setCurrentStep] = useState<FormStepName>('welcome');

A lot more readable.

[evisdrenova]

yeah i think it's more readable, it does add a little bit of an ugly switch to the new callback functions i added, since it's not a simple addition/subtraction anymore, but maybe that's okay since we gain more readability?

[nickzelei]

Most of these forms don't need to know their step or current step. They just need to know onNext() or onBack(), onSkip(), etc, whichever is relevant. Makes this a lot more readable and easier to manage.

[nickzelei]

just wanted to put in here that this kinda sucks if for whatever reason they can't hit our server to set the onboarding config to true. it will stay stuck on the screen until our server comes back online. Not much we can do about taht I suppose unless we were to cache it in local storage or something. Maybe that is a task for another day however.

[evisdrenova]

yeah this could a follow up where we store it in local storage and check that if the db isn't active

[nickzelei]

This is a breaking change and properties in proto shouldnt ever be renamed or re-used.

So what you actually want to do here is to instead deprecate these by marking them as deprecated (you can see in other protos uses of the deprecated comment.

Then the has_completed_onboarding should be a new property that starts at 5;

[nickzelei]

From the outside these can all just default to false and everything is gucci since the frontend will only care about the new property after this.

[nickzelei]

You will also need to update the user service integration tests to accommodate your new changes.

[nickzelei]

This is fine and shouldnt be a breaking change, however it may be worth thinking through the idea that if someone has already completed this v1 of onboarding, we it could be worth somehow marking this new one as true automatically.

We could also say fuck that are just require everyone see the new onboarding guide to keep it simple too.

[nickzelei]

Otherwise these DTO updates are just fine because the old properties will default to false if unspecified.

[nickzelei]

overall looking good on the FE. Left a comment regarding the record count type.

[nickzelei]

Let's make this type safe.

here is an example of how we extract the case for the different connections.

// Helper function to extract the 'case' property from a config type
type ExtractCase<T> = T extends { case: infer U } ? U : never;

// Extraction type that pulls out all of the connection config cases
export type ConnectionConfigCase = NonNullable<
  ExtractCase<ConnectionConfig['config']>
>;

The same can be done for the metric identifiers.

[nickzelei]

I don't think you want this one to be deprecated 😉

[nickzelei]

You'll need to merge in main or rebase and re-run the backend make gen to rebuild your proto changes with the latest.

Please note that the *validate.go files have been removes so do not re-add them.