Feat: Uptime Progress Indicator#3673
Conversation
There was a problem hiding this comment.
Visually looks good.
There are a couple of bugs to work out.
-
I can create invalid monitor types by manipulating the form steps. The form should be reset on changing monitor type.
-
A blank third screen is rendered for many monitor types, we should skip rendering the advanced third page if it is not used for that monitor type
I think other than that things look good!
| const background = | ||
| index < current | ||
| ? completedBg | ||
| : index === current | ||
| ? activeBg | ||
| : theme.palette.divider; |
There was a problem hiding this comment.
Can we please extract this logic? Nested ternaries aren't my favourite
| }, [defaults, form]); | ||
|
|
||
| // Multi-step wizard (uptime create only). Other flows keep the flat form. | ||
| const isWizard = showTypeSelector; |
There was a problem hiding this comment.
isWizard just shadows showTypeSelector 🤔 I assume we can just remove this one the other pages also implement the wizard flow?
| if (isValid) { | ||
| setCurrentStep((step) => Math.min(step + 1, TOTAL_STEPS - 1)); | ||
| } | ||
| }; | ||
| const handleBack = () => setCurrentStep((step) => Math.max(step - 1, 0)); |
There was a problem hiding this comment.
Are these clamps here to avoid issues with keyboard shortcuts? You should never be able to advance past the page limit or go back past page 0 via the UI controls since there are render guards in place already.
There was a problem hiding this comment.
I think most of the issues are resolved now.
My only real remaining concern is that there is an implicit and unstated dependency between the zod validation schema and the array of field names in the create monitor page.
I think this relationship should be made explicit so that there will not be divergence between the schema and the fields in the array in the future.
| [ | ||
| "type", | ||
| "url", | ||
| "name", | ||
| "port", | ||
| "gameId", | ||
| "grpcServiceName", | ||
| "dnsServer", | ||
| "dnsRecordType", | ||
| ], | ||
| ["interval", "statusWindowSize", "statusWindowThreshold", "notifications", "tags"], | ||
| [ | ||
| "ignoreTlsErrors", | ||
| "useAdvancedMatching", | ||
| "matchMethod", | ||
| "expectedValue", | ||
| "jsonPath", | ||
| "geoCheckEnabled", | ||
| "geoCheckLocations", | ||
| "geoCheckInterval", | ||
| ], | ||
| ]; |
There was a problem hiding this comment.
I think there is probably a better approach here. This is quite fragile as it relies on unstated assumptions about how the zod schema is set up.
For example, if we rename one of the fields in the validation schema/db model, someone needs to rememeber to also update these arrays.
This is an unstated dependency and will be difficult to track down later on.
A more robust approach is probably to derive these fields from the monitor type so there is an explicit dependency.
In other words, the set of fields to validate should not be the superset of all fields available for STEP[n], it should be the set of exactly the fields available for the selected monitor type.
It also relies on an unstated assumption that the order of the array is the same order as the steps. Not a huge deal, but this is a code smell, so I'd avoid it and be more explicit. A map or set is probably a better choice here 🤔
2 participants
Describe your changes
Splits the overwhelming single-page "Create Uptime Monitor" form into a focused 3-step wizard (Basics → Monitoring → Advanced) with a slim segmented progress bar in Checkmate's green palette, validating each step before advancing. Scoped to uptime create only — edit/configure, pagespeed, and hardware flows keep the existing flat form unchanged.
Please ensure all items are checked off before requesting a review. "Checked off" means you need to add an "x" character between brackets so they turn into checkmarks.
<div>Add</div>, use):npm run formatin server and client directories, which automatically formats your code.