feat(app): Add Error Recovery orchestrator #15077
Conversation
There was a problem hiding this comment.
This is generally good, but depending on how often we want to reuse step visualization I wonder if we might want a separate per-route orchestrator or something. I'm a little worried about encoding the paths between steps as an array because I could certainly see a case where that flow is not linear even within a route, which we'd only be able to model by breaking down routes more and more pathologically.
If instead, the top-level orchestrator hosted some route object (one for each route), and each route object hosted the appropriate step object, we might be able to admit more complex step sequencing behavior in code in the route objects while keeping that behavior localizxed just to the route objects.
| @@ -1,3 +1,19 @@ | |||
| { | |||
| "run_paused": "Run paused" | |||
| "are_you_sure_you_want_to_resume": "Are you sure you want to resume?", | |||
There was a problem hiding this comment.
I think we need an anonymized version of this, right @brenthagen
There was a problem hiding this comment.
Yeah @brenthagen let me know what I need to do for localization/OEM stuff in general, since I'm pretty out of the loop on that.
| return createPortal( | ||
| <Flex | ||
| flexDirection={DIRECTION_COLUMN} | ||
| width="992px" |
There was a problem hiding this comment.
do we have a place to derive these? i guess we use them in very few places
There was a problem hiding this comment.
I should have left a comment about this - we don't have a unified component for this currently, but it's becoming common enough that we need one IMO. I have a running list of components that should be unified and this one is on it. I plan on giving that list to Mel for feedback when we formally start unification work.
| alignItems={ALIGN_CENTER} | ||
| gridGap={SPACING.spacing24} | ||
| height="100%" | ||
| width="848px" |
There was a problem hiding this comment.
can we derive this from somewhere?
There was a problem hiding this comment.
I don't think so. This is special-cased by design. My guess is Sue didn't like the way the copy looks in the modal if we use the default width, so there's a hardcoded one-off adjustment to it.
Thanks for the feedback. I'll mull this over for a bit. |
|
After in person convo with Seth, we've decided we'll keep this for now and most likely refactor the route/step state as a directed graph when the time is right. For now though, this is sufficient. |
Closes EXEC-389
Overview
Error Recovery Flows are "special and unique"™ compared to other wizard flows. This PR addresses the underlying structure of ER and how to best solve for that uniqueness. I've outlined some of that uniqueness below and appreciate feedback/thoughts!
We May (or Not) Support Keeping Apps in Sync
Because there may be a world down the road in which we want to keep users across multiple desktop apps/the ODD in sync, we need some sort of way for the client to report where it is in ER. To that end, I propose breaking all content within ER flows into recovery routes and recovery steps. A recovery route is a logically-related collection of recovery steps or a single step if unrelated to any existing recovery route. A recovery step is analogous to a "step" in other wizard flows - a deterministic ordering of content for a given route.
While it would be great if we could use React Router to create our own mini-router, in practice this doesn't work. There is only one route at any one time (as far as React Router is concerned), and we can't simply use a subset of routes within a modal (React router will redirect the page). While this could work if all apps were the ODD, where we could effectively make the modal a page), we can't do this with desktop apps, so this option is out.
My solution to the above has been to recreate react-router without using react-router. We navigate around the modal when it's opened using a route, and the step applies changes to the view when needed. What this means is that when we decide to keep clients in sync across apps, we just need to create some hook that creates a URL-esque string from the route and step.
The major drawback of this approach is that we don't really do this anywhere else on the FE, so it's a new thing to learn. I've tried to compensate for the newness by enforcing strict type checking, so as soon as someone tries adding a new route somewhere, lint should be helpful.
ER Flows are Non-Deterministic
Unlike other flows, ER does not use a step counter. This is because a user may select from one or more recovery options, each with a variable number of steps. These recovery options all branch from a central node, the
SelectRecoveryOptionsroute. The major utility to help navigate these flows isuseRouteUpdateActions.Despite being non-determinstic include a lot of "Go back." The way we handle this in other flows is to return the previous step within a series of steps, which I've also emulated when we are in a particular route. The drawback of this is that there are a bunch of routes, so that requires us to always account for the ordering of steps for each route, even if the route has one step (which is pretty common at the moment, but I don't want to make assumptions with how things will turn out when designs are still changing). When we can't "go back" anymore, we return to the
SelectRecoveryOptionsby default.ER Should be DRY
One issue I've noticed in other wizards in the past is that we break the wizard down into steps, but then each step is independent from other steps entirely in terms of the components that comprise the step. This means that if the same button that's used in every page needs a change, every single component needs to change: we create a relatively large bug surface. To attempt to address this, I've introduced a
shareddirectory withinRecoveryOptions. I may include asharedin the broaderErrorRecoveryFlowsdir if needed. The point is that things that get reused a lot should probably be grouped. I've tried encapsulating exports as well so only the things that should be used by higher-order components are exported.This being said, I'm open to other ideas. I admit that these footer buttons require some props for overriding behavior, but I still think this is better than doing nothing...it also has the added benefit of keeping things more testable/readable.
Screen.Recording.2024-05-02.at.2.56.52.PM.mov
Test Plan
Changelog
Review requests
While the focus of this PR is more on overall structure and approach rather than emulating wireframe designs (these designs aren't finalized, and that's why I'm using crazy colors), high-level design feedback is welcome!
Risk assessment
low