I think we don't need to check the type of value anymore, I fixed it last week and will return either true or false

I'm removing this part. 👍
Just to confirm, Is it going to return false if a user has not opt-in/opt-out yet?

For the initial start of the app(first time running app) it will not return false
Because there won't be any stored Redux state, this is the reason we've introduced defaultValue property to this method.

But on the second time when user execute the app, it will definitely return false

Amazing that is exactly what we will need.

Would it be better if we also set stored events to the empty array?
this.store.set('eventsBeforeMetricsOptIn', []);
It could get overloaded if the user switch back and forth

100% I'm changing it now.

Would this be more useful after constructing eventToTrack as it would contain more info?

Given we explicitly want to use the three states here, I think this is a little dangerous as we have to be exact with all our conditions despite it returning a boolean.

Perhaps it would be safer and more obvious if we return an enum such as MetricsDecision with values like ENABLED, DISABLED, and PENDING?

Great point, I'm pushing the changes you suggested for this method.

getMetricsDecision?

Do we not need the inverse condition here also to clear the array if we get a confirmed negative? Otherwise the pending events will just sit in memory until the app is restarted?

yeap, I just changed it while I was refactoring this method. 💜

sendPendingEvents?

Why are we saving every event? Is this part of the first time checks? Won't we only need to save unique event names?

This was actually one of the behaviours that I've mimed from the extensions. Initially, I thought would be complementary to the first-time check solution but turns out it end up being much more simple the solution PR I can remove it for simplicity in the PR 483.

Will we ever have more than one analytics method or are we just matching the format of the store bridges?

If so, shall we push for simplicity and just have a track method directly on the electronBridge?

In the PR to track first-time events I'm including identify. There are also page and screen that the extension uses and we might need to integrate them depending on how we move further with the metrics.

If we think of the UI as purely a display of the background state, then we don't want to couple our metrics to the UI, but to the underlying background logic whenever possible. This ensures we're tracking events as low as possible in case we remove it from the UI and forget, or the UI failures for whatever reason.

This invalid OTP event for example should be fired from: packages/app/src/app/pairing.ts.

Totally, I'm removing it from the UI this was more to test the bridge.

Doesn't analytics.identify also send a request to Segment?

If so, shouldn't we also maintain an array of these identify requests while pending to be sent later?

Yes, It sends to Segment but only when users agree. In the PR infrastructure for the first time event I'm saving the traits locally because traits are attached whenever we send an event using track method. Currently, that is the important part for us, but we still need to discuss the usage of identity with the Data & Analytics team.

Sorry if i'm missing something but won't the current code send requests to Segment if the decision is still pending?

@vinistevam what @matthewwalsh0 is saying is that even on that PR we are sending an identity event to Segment even if the MetricsDecision.PENDING.. we should only send when MetricsDecision.ENABLED.

Sorry guys, totally my fault. I pushed a fix for that only allowing identify to send to segment when MetricsDecision.ENABLED

We should validate right at the top of this method. There's no point in setting a desktopMetricsId, or building the context on the event to track, if the user does not want to be tracked.
Plus having early returns makes the code much easier to read.

if (metricsDecision === MetricsDecision.DISABLED) {
    return;
}
... code ...

if (metricsDecision === MetricsDecision.PENDING) {
    this.eventsSavedBeforeMetricsDecision.push(eventToTrack);
    return;
}

...

Great point, I'm pushing this change.

This seems overly complex and has some side effects. getMetricsDecision should not be responsible for sendPendingEvents or cleanPendingEvents..
Those are consequences of the decision. We should move those out of this method (probably to the place where the desktopMetricsOptIn value is stored.. like when user opts in/out we should get an event on the main process notifying it to send or clean pending events.. Probably the event is already getting to the main process on ipcMain.handle(${name}-store-set`. Not sure if reusing that would be the best though or if we should add another specific event for it.)

Removing those would also make this method much much simpler. Could be something like this

const defaultValue = undefined;

...

if (desktopMetricsOptIn === defaultValue) {
  return MetricsDecision.PENDING;
}

return desktopMetricsOptIn === true ? MetricsDecision.ENABLED : MetricsDecision.DISABLED;

Agree, I created a new event in the electron bridge to be invoked when users take the metrics decision and that will be responsible to send or clean the events saved before the user's decision.

Remove else if return. We want to enable no-else-return (https://eslint.org/docs/latest/rules/no-else-return). (I though that we had it enabled.. but apparently not).

if (...) {
    return;
}

if (...) {
    return
}

return

thanks for pointing that out.
Just to keep documented here as well, it was fixed on commit: dc0c381