MISC: Throw error if tutorial step changed while tutorial is not running

[branchtheory]

There's currently nothing stopping you incrementing the tutorial step while it's not running. This guards against that to a degree.

Not sure if this is how we generally solve this problem?

EDIT: OK here's what's going on.

When you start the tutorial, that switches an isRunning flag in ITutorial (in InteractiveTutorial.ts) to true, and sets your current tutorial step to Start.

You can then increment / decrement the tutorial step with iTutorialNextStep() and iTutorialPrevStep().

When you hit the last step of the tutorial, that triggers iTutorialEnd(), which switches isRunning to false, and resets the tutorial step to Start as well.

The problem this PR would mitigate is that there's currently no check on calling iTutorialNextStep() or iTutorialPrevStep(). You can call them any time you like, even if the tutorial isn't running.

Having this throw an error would help you spot your mistake.

[E9cipher]

What do you mean by "changing the tutorial step while the tutorial is not running"? Could you provide MRE (so we can understand and test it better)?

[branchtheory]

Sure, updated the first comment

[E9cipher]

Sounds like a good catch to me! Thanks for explaining :)

[branchtheory]

A question for the boots and cats of the world:

In #3079, I've used this pattern a lot:
if (ITutorial.isRunning && ITutorial.currStep === iTutorialSteps.DocumentationInfo)
which I got from the ScriptEditorRoot.tsx file.

For example:

• ITutorial.isRunning && ITutorial.currStep === iTutorialSteps.ScriptEditorEditAndSave ? "info" : "primary"
• ITutorial.isRunning && ITutorial.currStep === iTutorialSteps.ActiveScriptsDescription ...

If we implement this PR though, the only thing the isRunning guards against is clearly bad patterns, like modifying iTutorial.currStep directly.

In these specific cases, if this PR gets approved, would you suggest taking the isRunning conditions out?

[catloversg]

This is unnecessary, or, to be more precise, it's a solution to a "wrong" issue.

How can you increase the tutorial step when the tutorial is not running? If it happens, it's clearly because the tutorial code is buggy. In theory, this would help us catch those bugs, but this is practically useless.

Change iTutorialNextStep like this:

function iTutorialNextStep(): void {
  if (Math.random() < 0.3) {
    throw new Error("Tutorial is not running, but iTutorialNextStep() was called");
  }
  ITutorial.stepIsDone[ITutorial.currStep] = true;
  if (ITutorial.currStep < iTutorialSteps.End) {
    ITutorial.currStep += 1;
  }
  if (ITutorial.currStep === iTutorialSteps.End) iTutorialEnd();
  ITutorialEvents.emit();
}

When you progress through the tutorial, it will simulate a wrong step. However, you will see that the UI does not show that error message.

capture.mp4

In the worst case, if the thrown error is actually "caught" by something, it's likely the React error boundary, which will activate the recovery screen. This is really bad UX.

I'll be frank with you, the tutorial should never have this issue (increase the tutorial step when the tutorial is not running). If it happens, it means either we do not test tutorial code or the tutorial is so unnecessarily complicated that we miss an edge case. The solution is not adding useless checks to iTutorialNextStep and iTutorialPrevStep that actually do not show any feedback but testing the tutorial carefully and/or simplifying the tutorial.

[branchtheory]

Thanks Catlover.

Can you check the commits again? I've updated them with what I think is a better solution.

This issue will never happen provided that:

• if the currSetp is Start, you check that currStep is Start and check isRunning
• if the currStep is not Start, you just check that the currStep is the one you want

Provided you do that, you don't need these throws at all. You don't even isRunning checks of the kind we do in ScriptEditorRoot:
if (ITutorial.isRunning && ITutorial.currStep === iTutorialSteps.ScriptEditorEditAndSave)

[catloversg]

To be honest, I don't think we need to change anything. This kind of change makes me feel as if we are finding an issue for a solution. You can simplify the condition in some cases, so I'm not exactly against that, but that's unnecessary just to enforce a rule with a comment.

Checking ITutorial.isRunning clearly shows the intent that a block is only relevant for tutorial code. Now we need to keep in mind what kind of additional check or specific step check we are performing next so that we can simplify the first check. Doing all of that just to save a check is unnecessary.

In short, there is no need to simplify that check and add a comment explaining when to do so. Just keep things as-is.

[branchtheory]

OK, that makes sense to me. Thanks for the help. I'll make sure to include isRunning checks in my main Tutorial PR.