[Alerting v2] Form/YAML toggle for Discover flyout#265929
Conversation
Thread includeYaml through the public Discover-facing flyout API (default true) so the existing toggle renders inside the create-from-Discover flyout. Drop icons from toggle and update section/flyout titles per design. Refs elastic#265912. WIP — pending plan for continuous YAML→form state sync. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
github-actions
Bot
added
the
author:actionable-obs
Move the YAML string state out of YamlRuleForm's local useState into RuleFormContent so it survives the unmount that happens when the user toggles back to Form. Lazy-init from current form values via serializeFormToYaml(getValues()). Test: YAML edits persist across Form↔YAML toggle. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Watch evaluation.query.base via useWatch and re-serialize the YAML buffer from current form values whenever it changes. ES|QL editor edits propagate to the YAML view immediately; any user-typed YAML content is intentionally clobbered (per design). Test: query setValue → YAML buffer reflects new query. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Forward Monaco's onDidBlurEditorText to a new optional onBlur callback so consumers can react when the user pauses editing the YAML buffer. The handler reads from a ref so the Monaco listener (set up once on mount) always invokes the latest callback without re-attaching. End-to-end coverage of the full YAML-on-blur flow follows in the next commit, where YamlRuleForm wires this up. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Wire YamlRuleEditor's onBlur to parseYamlToFormValues and apply the
parsed values to React Hook Form via reset(values, { keepDirty: true,
keepDefaultValues: true }), wrapped in an applyYamlValuesToForm helper
to make the intent clear at the call site.
When YAML is invalid on blur, the error is shown but form state is
left untouched and the YAML buffer is preserved (lifted) so the user
can fix and re-blur.
Tests:
- valid blur applies parsed values to form state
- invalid blur shows error, leaves form unchanged
- round-trip stability: parse(serialize(values)) preserves load-bearing
fields
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
![macroscopeapp[bot]](https://avatars.githubusercontent.com/in/900172?s=60&v=4){kind=small}
The API's metadataSchema is .strict() and accepts only
{ name, description?, owner?, tags? }. The 'enabled' field lives at
the top level of update/response schemas, never under metadata, and
isn't part of the create payload at all. We were emitting
metadata.enabled into the YAML, producing output that wouldn't
validate against the real schema.
The form keeps its own metadata.enabled for the Enabled toggle UI;
the request mappers strip it before the API call. The YAML view now
mirrors the actual API contract.
The parser still tolerates a stray metadata.enabled if someone hand-
authors YAML with it, defaulting to true when absent.
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Replace the single-field useWatch on evaluation.query.base with a callback-form watch() subscription that fires on any form field change. This fixes Form→YAML sync for name, tags, schedule, etc., which previously only updated YAML when the query changed. The callback variant of watch() runs as a side-effect rather than causing RuleFormContent to re-render on every keystroke, so this is slightly cheaper than the prior single-field useWatch even though the surface area widens. Tests: regen on metadata.name change; regen on metadata.tags change; existing query-change test still passes. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
The Monaco editor's onDidBlurEditorText can race with the toggle's
mode change: when setEditMode('form') re-renders, YamlRuleForm
unmounts, and the blur listener is disposed before its callback
fires. The result was YAML edits getting dropped when the user
clicked the Form toggle directly from the editor.
Fix by syncing YAML→Form explicitly inside handleModeChange when
leaving YAML mode, independent of editor blur. The blur path still
exists for cases like clicking Save without leaving YAML mode first;
both call sites are idempotent so concurrent firing is fine.
Test: edit YAML, click Form toggle, assert form state reflects YAML
edits (name and tags).
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Spell out the three sync mechanisms (blur, mode toggle, submit) and why blur alone isn't sufficient. Future contributors adding new unmount paths or YAML-mode actions must flush explicitly. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
| const handleModeChange = useCallback( | ||
| (newMode: EditMode) => { | ||
| if (newMode === editMode) return; | ||
| // When leaving YAML mode, eagerly flush YAML→Form so the form view | ||
| // shows the user's YAML edits. Don't rely on the Monaco editor's blur | ||
| // event — it can race with the unmount triggered by setEditMode and | ||
| // get dropped along with the disposed listener. If the YAML is invalid | ||
| // we keep the lifted yamlText buffer and skip the form update; the user | ||
| // can fix the YAML and try again. | ||
| if (editMode === 'yaml' && newMode === 'form') { | ||
| const result = parseYamlToFormValues(yamlText); | ||
| if (result.values) { | ||
| reset(result.values, { keepDirty: true, keepDefaultValues: true }); | ||
| } | ||
| } | ||
| setEditMode(newMode); | ||
| }, | ||
| [editMode] | ||
| [editMode, yamlText, reset] | ||
| ); |
There was a problem hiding this comment.
🟠 High form/rule_form.tsx:132
In handleModeChange, when switching from YAML to Form mode with invalid YAML, the mode switch proceeds unconditionally via setEditMode(newMode) even though the YAML parse failed. The invalid YAML edits in yamlText are immediately lost because once in Form mode, the watch subscription triggers on the first form interaction and overwrites yamlText with re-serialized stale form values. The user cannot "fix the YAML and try again" as the comment claims because the YAML buffer has been destroyed. Consider blocking the mode switch when YAML parsing fails, or keep the YAML buffer intact so the user can return to YAML mode to fix their edits.
- const handleModeChange = useCallback(
- (newMode: EditMode) => {
- if (newMode === editMode) return;
- // When leaving YAML mode, eagerly flush YAML→Form so the form view
- // shows the user's YAML edits. Don't rely on the Monaco editor's blur
- // event — it can race with the unmount triggered by setEditMode and
- // get dropped along with the disposed listener. If the YAML is invalid
- // we keep the lifted yamlText buffer and skip the form update; the user
- // can fix the YAML and try again.
- if (editMode === 'yaml' && newMode === 'form') {
- const result = parseYamlToFormValues(yamlText);
- if (result.values) {
- reset(result.values, { keepDirty: true, keepDefaultValues: true });
- }
- }
- setEditMode(newMode);
- },
- [editMode, yamlText, reset]
- );🤖 Copy this AI Prompt to have your agent fix this:
In file x-pack/platform/packages/shared/response-ops/alerting-v2-rule-form/form/rule_form.tsx around lines 132-150:
In `handleModeChange`, when switching from YAML to Form mode with invalid YAML, the mode switch proceeds unconditionally via `setEditMode(newMode)` even though the YAML parse failed. The invalid YAML edits in `yamlText` are immediately lost because once in Form mode, the `watch` subscription triggers on the first form interaction and overwrites `yamlText` with re-serialized stale form values. The user cannot "fix the YAML and try again" as the comment claims because the YAML buffer has been destroyed. Consider blocking the mode switch when YAML parsing fails, or keep the YAML buffer intact so the user can return to YAML mode to fix their edits.
Evidence trail:
x-pack/platform/packages/shared/response-ops/alerting-v2-rule-form/form/rule_form.tsx lines 88, 96-100 (watch subscription that overwrites yamlText on any form change), lines 132-148 (handleModeChange function that always calls setEditMode(newMode) at line 147 regardless of YAML parse result)
There was a problem hiding this comment.
Interesting thoughts, likely we need some kind of "clear edits" option but for now I don't mind how it works? we could block the toggle, but then the user has to know how to revert their YAML changes or be stuck forever. Storing the buffer sounds ... no thanks
Drop { keepDirty: true, keepDefaultValues: true } from both the
toggle-flush (handleModeChange) and the blur sync
(applyYamlValuesToForm). With those options RHF was skipping the
value update for clean fields, which manifested as the YAML editor
briefly flashing back to old values right before the form toggle
took effect.
Plain reset(values) — the same pattern handleYamlSubmit already uses
on Save — applies the values reliably. Side effect: form is marked
clean after sync (defaults updated to match new values). This is
fine for our flow; isDirty isn't used to gate Save.
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
…ed setValue
The useForm `values` prop with `resetOptions: { keepDirtyValues: true }`
was silently blocking manual reset() calls from updating fields that had
been dirtied by form interaction — making YAML edits revert to old form
values on toggle. Replace with defaultValues + a useEffect that calls
setValue for the only externally-driven field (evaluation.query.base).
Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
ApprovabilityVerdict: Needs human review This PR introduces a new Form/YAML toggle feature with significant state management changes across 15 files, none of which are owned by the author. Additionally, there is an unresolved high-severity comment identifying a potential bug where YAML edits are lost during mode switching. You can customize Macroscope's approvability policy. Learn more. |
jasonrhodes
added
release_note:skip
yiannisnikolopoulos
left a comment
yiannisnikolopoulos
left a comment
There was a problem hiding this comment.
Toggle UX feels really smooth nice work on the sync mechanics. I've documented some manual testing findings:
1. state_transition silently dropped on Form→YAML→Form round-trip
formValuesToYamlObject does not serialize state_transition (pending_count, pending_timeframe, recovering_count, recovering_timeframe). However, parseYamlToFormValues does parse it. This means:
- Set alert delay to 3 breaches in Form mode
- Toggle to YAML — no
state_transitionblock appears - Toggle back to Form (or blur) —
reset(parsedValues)overwritesstateTransitionwithundefined - State transition settings are silently lost
The round-trip test in the PR includes stateTransition in its input but never asserts it in the output, so it passes despite the data loss.
2. state_transition changes are being reset to default on Form→YAML→Form round-trip
- Set alert delay to 3 breaches in Form mode
- Toggle to YAML and make an edit
- Toggle back to Form
- Alert delay is reset back to default, which is Immediate
This happens for both Alert and Recovery delay
3. recovery_policy cleared on any Form→YAML→Form round-trip (causes Bad Request on submit)
Repro: Open create rule flyout → do nothing → toggle to YAML → toggle back to Form → add a name → submit → 400 Bad Request.
recovery_policy is neither serialized nor parsed in yaml_form_utils.ts. The form defaults initialize it as { type: 'no_breach' }, but after a YAML round-trip reset(parsedValues) sets it to undefined. The API rejects the missing field.
This happens even if you never edit anything in YAML mode just toggling there and back is enough to wipe it.
…round-trip
Both fields were parsed from YAML but never serialized, causing silent
data loss on Form→YAML→Form toggles. recovery_policy was also missing
from the parser, defaulting to undefined instead of { type: 'no_breach' }.
Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Replace Record<string, unknown> return types with concrete interfaces so TypeScript catches key typos and shape mismatches at compile time. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Remove 12 redundant tests that duplicate coverage already provided by other tests in the same file or by unit tests in yaml_form_utils.test.ts. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
… cuts The previous commit over-aggressively removed 13 pre-existing tests from rule_form.test.tsx. Those tests weren't introduced by this PR and shouldn't be removed in a feature PR — even if low-value, that's a separate cleanup. This restores all pre-existing tests. The only remaining cuts are 3 tests in yaml_rule_form.test.tsx that we introduced in this PR and deemed redundant with unit-level coverage in yaml_form_utils.test.ts: - "renders the form label and help text" — static UI, no conditional logic - "displays error for missing required fields" — parse error paths fully covered by yaml_form_utils.test.ts unit tests - "has correct data-test-subj attribute" — asserted implicitly by every other test that queries this element Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
💛 Build succeeded, but was flaky
Failed CI StepsMetrics [docs]Async chunks
Page load bundle
Unknown metric groupsAPI count
History
|
yiannisnikolopoulos
left a comment
yiannisnikolopoulos
left a comment
There was a problem hiding this comment.
Retested the manual testing findings, those cases work fine now 👍
One thing that I noticed (non-blocking):
When editing a rule that was saved with both alert and recovery delay set to "immediate," switching to YAML shows:
state_transition:
pending_count: 0
recovering_count: 0This happens because:
mapStateTransitionexplicitly writespending_count: 0 / recovering_count: 0to the API for "immediate" modemapRuleResponseToFormValuesreads those back intostateTransition: { pendingCount: 0, recoveringCount: 0 }- The serializer includes them since
0 != nullis true
Since "immediate" is the default and a missing state_transition already parses as immediate, this block is noise and could confuse the user.
|
|
||
| const serializeStateTransition = (st?: StateTransition): YamlStateTransition | undefined => { | ||
| if (!st) return undefined; | ||
| const out: YamlStateTransition = {}; |
There was a problem hiding this comment.
cosmetic nit: we could avoid a bit of repetition (and perhaps accidental keys misassignments) by using pickBy
const out: YamlStateTransition = pickBy(
{
pending_count: st.pendingCount,
pending_timeframe: st.pendingTimeframe,
recovering_count: st.recoveringCount,
recovering_timeframe: st.recoveringTimeframe,
},
v => v != null
);There was a problem hiding this comment.
Good suggestion. I looked at this but pickBy returns Dictionary<T> which loses the typed keys we get from YamlStateTransition — we'd need a cast to get the type safety back. Since it's only 4 lines and each one is doing an explicit camelCase→snake_case mapping, I'll leave it as-is for now.
umbopepato
left a comment
umbopepato
left a comment
There was a problem hiding this comment.
LGTM! 🚀
Desk testing works correctly, just left a small nit comment
4 participants
Summary
Closes #265912
Closes elastic/rna-program#275
Adds a Form/YAML toggle to the v2 alerting flyout opened from Discover. Both views share a single React Hook Form (RHF) form state so edits in either mode are preserved across toggles.
What changed
includeYamlprop threaded through the flyout API (package defaultfalse, plugin defaulttrue). Discover's existingCreateESQLRuleFlyoutpicks it up automatically.RuleFormContentso the text buffer survives the unmount when toggling back to Form mode.watch(callback)subscription — any form field change regenerates the YAML buffer. Required because Discover's external ES|QL editor is visible alongside the flyout in YAML mode.reset(values).DynamicRuleFormsimplified — replaced RHF'svaluesprop +resetOptions: { keepDirtyValues: true }withdefaultValues+ a targeteduseEffectcallingsetValuefor the query field (the only externally-driven field). This was the root cause of a sync bug wherekeepDirtyValuessilently blockedreset()from updating fields dirtied by form interaction. Note: this was the most complicated part of this PR, see further explanation below, and please ping me if you want a walkthrough of this change and the reasoning, etc.onBlurprop added to@kbn/yaml-rule-editor, wired to Monaco'sonDidBlurEditorText.metadata.enableddropped from YAML serialization —metadataSchemais.strict()and rejects it. Rules are always created enabled;enabledis only on the update API.Why the
DynamicRuleFormchange mattersRHF's
valuesprop +keepDirtyValues: truewas meant to protect user edits from external query changes. ButkeepDirtyValuesapplied globally to allreset()calls — including our explicit YAML→Form flushes. When a field was dirtied via the GUI, subsequentreset(yamlValues)silently skipped it, and thewatchcallback serialized the stale value back into the YAML buffer.The fix recognizes that only the query field is externally driven.
defaultValues+setValuehandles that case directly, andreset()works as a plain bulk-update everywhere else. 8 lines added, 27 removed.Test plan