mirror of
https://github.com/openobserve/openobserve.git
synced 2026-11-06 00:09:22 +00:00
Omkar Kesarkhane
1ec3b5d0dc
## Problem
Saving a browser check with an invalid step showed only the toast **"Fix
the highlighted fields before saving"** — no step expanded, no field
highlighted. Two separate faults produced that symptom.
### 1. In create mode, nothing reached the journey at all
`OStepper` runs in **wizard mode** here (no `expanded` prop, default
`false`), so `OStep` renders its panel behind `v-if="isActive"` —
**`BrowserJourney` exists only while `currentStep === 1`**. Create
mode's only Save button lives in the `currentStep === 2` footer, so
`journeyRef.value` was `undefined` when `persist()` ran:
```ts
currentStep.value = 1; // not flushed yet
journeyRef.value?.validateStepSelectors?.(); // no-op
journeyRef.value?.setStepFieldErrors?.(result.error.issues); // no-op
toast({ variant: "error", … }); // the only survivor
```
A `nextTick` would not have been enough: a **freshly mounted**
`BrowserJourney` starts with an empty error map, so an imperative push
cannot survive the remount. Edit mode worked because it has a Save on
the Journey step — which is why this read as create-mode-only.
### 2. Even when they arrived, only 2 of 7 rules expanded anything
Auto-expand lives in `validateJourneySteps()`, which knows two rules.
The other five — `stepNameRequired`, `retiredAction`, the navigate URL,
`typeTextRequired`, `expectedRequired` — live in the zod schema and
reached the journey through `setStepFieldErrors()`, which recorded the
message and returned. `getRowStatusColor()` ignored them too, so
"highlighted" was literally false for those rules.
## Fix
- **Issues are parent-owned state passed as the `fieldIssues` prop**,
applied by an `immediate` watcher. Mount order stops mattering — issues
raised while the journey is unmounted wait, and rows open whenever it
next renders (including the Details-error + journey-error case, where
the author lands on Configure and finds the journey already expanded on
switching tabs).
- **Both paths route through `revealErroredSteps()`** — opens every
errored row in journey order, clears an active filter (a filtered-out
row isn't rendered, so expanding it shows nothing), scrolls to the
first. `getRowStatusColor()` reads schema errors too.
- **Stale errors clear as the author edits.** The editor emits no
per-field event for name/value/expected, so those stayed red over
corrected input and kept re-opening a correct row.
- **Stale errors clear on a successful save** — `setStepFieldErrors` was
only ever called on the failure branch.
- **Duplicate error toast gone.** `persist()` no longer calls Path A at
all; the schema already emits both of its rules as field issues.
Removed as dead: `setStepFieldErrors` from `defineExpose`, and the
`silent` option added mid-fix for the duplicate toast.
## Tests
846 passing. Validation had large untested surfaces — which is how
coverage decayed without a single failure. Closed here:
- monitor-level `name`/`url`/`locations` and the **entire gate schema**,
neither ever exercised (`form()` supplies valid values; the view spec
replaces the schema module with a stub)
- `validateJourneySteps`' auto-expand — **untested since the first
commit**
- the `expectedErrorMessage` binding, `.trim()` on type text and
expected, the `\S+$` part of the navigate regex, `element_not_visible` /
`page_title` / `element_attribute`, and the `scroll`/`screenshot`
retired actions
The view spec's `OStep` stub was `v-if="$parent.$parent || true"` — it
rendered **every** panel, keeping `journeyRef` alive on the Configure
step, which is why this spec could not have caught fault 1. It now
mirrors wizard mode, and a create-mode save with a journey error is
asserted end to end.
**Every rule verified by mutation** — disabled at source, suite re-run,
reverted. **33 mutations, all caught.** Three initially ran green and
led to stronger tests: `min(1)` on `url` only governs *which message*
appears (the protocol refine rejects `""` anyway), nothing reached
`\S+$`, and `validateJourneySteps`' expand had no test at all.
## Not fixed — filed for follow-up
Found in the same sweep, deliberately out of scope:
- **`select` / `press` / `upload` accept a blank value.** `v2Value()`
maps all of them to the wire, so they save, run, and **pass** — the
false-green `typeTextRequired` exists to prevent. `type` isn't special;
it just got a rule.
- **A zero-step journey saves.** No `.min(1)`, and the first-step check
treats an empty array as valid.
- **`element_attribute` has no attribute rule**, and no error binding
for that field.
- **`timeout` / `settle.budget_ms` warn but never block.** Needs
`Advanced` to become controlled first, or the rules land inside a
collapsed section and reproduce this same bug one level deeper.
- **Continue and Replay use the 2-rule gate**, not the schema — so a
journey with a blank type text replays and false-greens.
- **A whitespace-only monitor name is accepted** (`.min(1).trim()` vs
the step rule's `.trim().min(1)`), so a check named three spaces saves
and displays blank.
Full write-up with the regression timeline, the mutation table, and why
each layer's tests stayed green is in
`docs/synthetics/002-journey-step-validation-audit.md` (untracked —
`docs/` is gitignored in this repo).
## Verification
- `npx vitest run src/components/synthetics src/views/synthetics` → 43
files, **846 passed**
- `npm run type-check` → clean
- `npx eslint` on changed files → 0 errors (4 pre-existing warnings,
none introduced)
- `npx prettier --check` → clean