Skip to content

[Alerting v2] Compose Discover follow-up: post-merge review fixes#269011

Merged
jasonrhodes merged 4 commits into
elastic:mainfrom
jasonrhodes:rna/pr-b-followup
May 13, 2026
Merged

[Alerting v2] Compose Discover follow-up: post-merge review fixes#269011
jasonrhodes merged 4 commits into
elastic:mainfrom
jasonrhodes:rna/pr-b-followup

Conversation

@jasonrhodes
Copy link
Copy Markdown
Member

Summary

Follow-up to #268774 addressing feedback from Bailey and Yiannis left after merge.

Changes

Bug fix (Yiannis): metadata.tags ?? [] revert — the API schema marks tags as optional but requires .min(1) if present, so sending [] is rejected. The key is now omitted entirely when tags is absent or empty.

Nits (Bailey):

  • Merge duplicate @kbn/code-editor imports in compose_discover_child.tsx and query_summary.tsx
  • Use paths.ruleCreate constant in rules_list_page.tsx instead of the inline URL string (consistent with the rest of the plugin)
  • Replace the inline AstNode interface + unsafe cast in compose_discover_form.tsx with isOptionNode type guard from @elastic/esql

Scout test tracking

The deleted quick_edit_rule.spec.ts tests are tracked in #268958 (child of M2 epic rna-program#482).

🤖 Generated with Claude Code

- revert metadata.tags ?? [] — API rejects empty array; omit the key
  entirely when tags is absent or empty (tags only valid with ≥1 item)
- merge duplicate @kbn/code-editor imports in child and query_summary
- use paths.ruleCreate constant instead of inline URL string in
  rules_list_page (consistent with rest of plugin)
- replace inline AstNode interface with isOptionNode type guard from
  @elastic/esql, eliminating the unsafe cast

Co-Authored-By: Claude Sonnet 4.6 (1M context) <noreply@anthropic.com>
@jasonrhodes jasonrhodes requested a review from a team as a code owner May 12, 2026 23:15
@github-actions github-actions Bot added the author:actionable-obs PRs authored by the actionable obs team label May 12, 2026
jasonrhodes and others added 2 commits May 12, 2026 19:26
isOptionNode's input type (ESQLAstNode) differs from ESQLAstItem, so
.filter(isOptionNode) on statsCmd.args doesn't narrow the type.
Revert to a local CmdOption alias + explicit cast — same approach that
passed CI in the merged PR.

Co-Authored-By: Claude Sonnet 4.6 (1M context) <noreply@anthropic.com>
@yiannisnikolopoulos
Copy link
Copy Markdown
Contributor

yiannisnikolopoulos commented May 13, 2026

The CI failure is a breaking test in rule_request_mappers.test.ts. The existing test 'strips enabled from metadata (server-managed) but includes description' was asserting tags: [] in the output, written against the old ?? [] behaviour. With the new omit-when-empty spread, tags is absent from the output when the array is empty, so the assertion fails.

Fix:

--- a/x-pack/platform/packages/shared/response-ops/alerting-v2-rule-form/form/utils/rule_request_mappers.test.ts
+++ b/x-pack/platform/packages/shared/response-ops/alerting-v2-rule-form/form/utils/rule_request_mappers.test.ts
@@ -268,9 +268,31 @@ describe('rule_request_mappers', () => {
         name: 'My Rule',
         description: 'A description',
         owner: 'owner',
-        tags: [],
       });
       expect(result.metadata).not.toHaveProperty('enabled');
+      expect(result.metadata).not.toHaveProperty('tags');
+    });
+
+    it('omits tags from metadata when tags array is empty', () => {
+      const formValues: FormValues = {
+        ...baseFormValues,
+        metadata: { ...baseFormValues.metadata, tags: [] },
+      };
+
+      const result = mapFormValuesToRuleRequest(formValues);
+
+      expect(result.metadata).not.toHaveProperty('tags');
+    });
+
+    it('omits tags from metadata when tags is undefined', () => {
+      const formValues: FormValues = {
+        ...baseFormValues,
+        metadata: { ...baseFormValues.metadata, tags: undefined },
+      };
+
+      const result = mapFormValuesToRuleRequest(formValues);
+
+      expect(result.metadata).not.toHaveProperty('tags');
     });

Copy link
Copy Markdown
Contributor

@yiannisnikolopoulos yiannisnikolopoulos left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Lgtm 👍 , posted a comment for the failing test

Empty tags array should produce no tags key in the output (per API
schema which rejects empty arrays). Update test to assert tags is absent
rather than expecting tags: [].

Co-Authored-By: Claude Sonnet 4.6 (1M context) <noreply@anthropic.com>
@jasonrhodes jasonrhodes added release_note:skip Skip the PR/issue when compiling release notes backport:skip This PR does not require backporting v9.5.0 labels May 13, 2026
@jasonrhodes jasonrhodes enabled auto-merge (squash) May 13, 2026 12:55
@kibanamachine
Copy link
Copy Markdown
Contributor

💛 Build succeeded, but was flaky

Failed CI Steps

Test Failures

  • [job] [logs] Scout Lane #6 - stateful-classic / default / local-stateful-classic - CustomStatusAlert - creates a custom status alert rule
  • [job] [logs] Scout Lane #5 - stateful-classic / default / local-stateful-classic - Profiling is setup and data is loaded - Admin user
  • [job] [logs] FTR Configs #42 / Serverless Common UI - Management Data View Management creating and deleting default data view index pattern deletion "before all" hook for "should return to index pattern list"

Metrics [docs]

Async chunks

Total size of all lazy-loaded chunks that will be downloaded as the user navigates the app

id before after diff
alertingVTwo 771.0KB 771.0KB -4.0B

History

@jasonrhodes jasonrhodes merged commit 3f1e056 into elastic:main May 13, 2026
80 checks passed
DennisKo pushed a commit to DennisKo/kibana that referenced this pull request May 13, 2026
…astic#269011)

## Summary

Follow-up to elastic#268774 addressing feedback from Bailey and Yiannis left
after merge.

## Changes

**Bug fix (Yiannis):** `metadata.tags ?? []` revert — the API schema
marks `tags` as optional but requires `.min(1)` if present, so sending
`[]` is rejected. The key is now omitted entirely when tags is absent or
empty.

**Nits (Bailey):**
- Merge duplicate `@kbn/code-editor` imports in
`compose_discover_child.tsx` and `query_summary.tsx`
- Use `paths.ruleCreate` constant in `rules_list_page.tsx` instead of
the inline URL string (consistent with the rest of the plugin)
- Replace the inline `AstNode` interface + unsafe cast in
`compose_discover_form.tsx` with `isOptionNode` type guard from
`@elastic/esql`

## Scout test tracking

The deleted `quick_edit_rule.spec.ts` tests are tracked in elastic#268958
(child of M2 epic rna-program#482).

🤖 Generated with [Claude Code](https://claude.com/claude-code)

---------

Co-authored-by: Claude Sonnet 4.6 (1M context) <noreply@anthropic.com>
Co-authored-by: kibanamachine <42973632+kibanamachine@users.noreply.github.com>
kelvtanv pushed a commit to kelvtanv/kibana that referenced this pull request May 14, 2026
…astic#269011)

## Summary

Follow-up to elastic#268774 addressing feedback from Bailey and Yiannis left
after merge.

## Changes

**Bug fix (Yiannis):** `metadata.tags ?? []` revert — the API schema
marks `tags` as optional but requires `.min(1)` if present, so sending
`[]` is rejected. The key is now omitted entirely when tags is absent or
empty.

**Nits (Bailey):**
- Merge duplicate `@kbn/code-editor` imports in
`compose_discover_child.tsx` and `query_summary.tsx`
- Use `paths.ruleCreate` constant in `rules_list_page.tsx` instead of
the inline URL string (consistent with the rest of the plugin)
- Replace the inline `AstNode` interface + unsafe cast in
`compose_discover_form.tsx` with `isOptionNode` type guard from
`@elastic/esql`

## Scout test tracking

The deleted `quick_edit_rule.spec.ts` tests are tracked in elastic#268958
(child of M2 epic rna-program#482).

🤖 Generated with [Claude Code](https://claude.com/claude-code)

---------

Co-authored-by: Claude Sonnet 4.6 (1M context) <noreply@anthropic.com>
Co-authored-by: kibanamachine <42973632+kibanamachine@users.noreply.github.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

author:actionable-obs PRs authored by the actionable obs team backport:skip This PR does not require backporting release_note:skip Skip the PR/issue when compiling release notes v9.5.0

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants