[Dashboard] Composable content management transforms by nickpeihl · Pull Request #213831 · elastic/kibana

nickpeihl · 2025-03-10T20:55:23Z

Summary

Fixes a bug where a control having a width of "auto" causes the dashboard to be invalid.

This bug only surfaced to users as a warning of an Invalid dashboard in Kibana server logs. In development environments (such as yarn start or running functional tests locally) the bug would have thrown an error. CI would not catch the bug due to running functional tests using a production build of Kibana.

As part of this fix, I've updated the Dashboard content management transforms to use smaller, composable functions. This makes it easier to read, add, or update transforms for dashboards. Additionally, this adds a transform function that fixes the bug in #211113 at runtime rather than via a saved object migration which are no longer supported.

Checklist

Check the PR satisfies following conditions.

Reviewers should verify this PR satisfies this list as well.

Unit or functional tests were updated or added to match the most common scenarios
Flaky Test Runner was used on any tests changed Flaky test runner
The PR description includes the appropriate Release Notes section, and the correct release_note:* label is applied per the guidelines

Identify risks

Does this PR introduce any risks? For example, consider risks like hard to test bugs, performance regression, potential of data loss.

Describe the risk, its severity, and mitigation for each identified risk. Invite stakeholders and evaluate how to proceed before merging.

See some risk examples
...

…contract" This reverts commit b3d4e48.

…th-auto

nickpeihl · 2025-03-11T19:26:06Z

+    if (control.width === 'auto') {
+      return { ...control, width: DEFAULT_CONTROL_WIDTH, grow: true };
+    }


This is the transform that fixes #211113.

elasticmachine · 2025-03-12T18:39:29Z

Pinging @elastic/kibana-presentation (Team:Presentation)

kibanamachine · 2025-03-12T21:39:24Z

Flaky Test Runner Stats

🎉 All tests passed! - kibana-flaky-test-suite-runner#8024

[✅] x-pack/test/functional/apps/dashboard/group2/config.ts: 100/100 tests passed.

see run history

thomasneirynck

Isolating each step into a function makes sense to me.

Do we know why x-pack/test/functional/apps/dashboard/group2/migration_smoke_tests/controls_migration_smoke_test.ts not capture this? Should that test be expanded to capture this bug in the future?

nickpeihl · 2025-03-20T20:42:34Z

Do we know why x-pack/test/functional/apps/dashboard/group2/migration_smoke_tests/controls_migration_smoke_test.ts not capture this? Should that test be expanded to capture this bug in the future?

CI runs functional tests in against a Kibana build, not against a dev server from source. Since #166919, any server validation errors are not surfaced to the user in Kibana builds, but logged as a warning in the server logs. So the CI test renders the page without errors.

However, server validation errors are thrown when running in dev mode and typically functional tests are run locally against a dev server. Since validation fails due to an unrecognized key, the server throws an error. And due to #211659, a 404 page is rendered.

The smoke test only checks for embeddable errors, of which there are none on a 404 page. 😄

I've added 7f2a22d which might improve this test.

ThomThomson

This is very well tested, and well organized! Great to see a formalized transforms system in place, and I'm excited to apply it to Embeddables as well.

Tested this locally by importing the controls smoke test, and also looked through the code.

Would it be possible to name the files with the in and out transforms in a way that is more friendly to searching in vscode using command p? Something like control_group_out_transforms.ts.

Left a few questions, but nothing urgent to change. LGTM!

ThomThomson · 2025-03-20T21:44:12Z

+  return defaults(ignoreParentSettings, DEFAULT_IGNORE_PARENT_SETTINGS);
+}
+
+function transformIgnoreParentSettingsProperties({


Is this just a noop? Why do we need it?

I'm using it to extract and provide only supported properties from the saved object. It guards against any unsupported properties that may exist in the saved object. I may be overly cautious here. 🤷

Added some documentation in 460bf34.

That makes sense. We know how possible it is for strange unsupported keys to end up in the saved object as holdovers from older versions or bugs...

ThomThomson · 2025-03-20T21:48:41Z

+export const transformControlsState: (
+  serializedControlState: string
+) => ControlGroupAttributes['controls'] = flow(
+  JSON.parse,


I wonder if there is any way to give flow explicit types - because right now it seems to pass implicit anys which need to be typed in each individual function.

It isn't necessary to fix this now, but IMO this leaves room for errors because the output of one function isn't guaranteed to be the type that the next function takes. I wonder how RXJS' pipe function handles its strict typing.

I think we'd have to add a bunch of one-off types to provide to flow.

I've seen core using the similar fp-ts library which seems to infer types. But that whole library seems like overkill. I'd rather just write the functions like transformC(transformB(transformA(input))). 😆

ThomThomson · 2025-03-20T21:49:34Z

+// TODO We may want to remove setting defaults in the future
+export function transformControlsSetDefaults(controls: SerializableRecord[]) {
+  return controls.map((control) =>
+    defaults(control, { grow: DEFAULT_CONTROL_GROW, width: DEFAULT_CONTROL_WIDTH })


I wonder why the usage of lodash defaults here rather than a spread? Is it just for understandability?

Good old fashioned laziness. But I think it is better to be consistent and spread works just as well without needing to use lodash defaults. Updated in 1f430b0.

nickpeihl · 2025-03-21T13:07:21Z

Would it be possible to name the files with the in and out transforms in a way that is more friendly to searching in vscode using command p? Something like control_group_out_transforms.ts.

Good idea. Changed in 00da442.

elasticmachine · 2025-03-21T20:25:02Z

💚 Build Succeeded

Buildkite Build
Commit: 261a057

Metrics [docs]

✅ unchanged

History

💚 Build #286138 succeeded a5e94e0
💚 Build #283427 succeeded c2448cf
💔 Build #282968 failed 092c58a
💔 Build #282478 failed a0092bc4a5de5ca195cd9923dcbd7c9a34a3010d

kibanamachine · 2025-03-21T22:04:55Z

Starting backport for target branches: 8.18, 8.x, 9.0

https://github.com/elastic/kibana/actions/runs/14001650501

(cherry picked from commit f123b50)

kibanamachine · 2025-03-21T22:10:50Z

💔 Some backports could not be created

Status	Branch	Result
❌	8.18	Backport failed because of merge conflicts
✅	8.x
❌	9.0	Backport failed because of merge conflicts

Note: Successful backport PRs will be merged automatically after passing CI.

Manual backport

To create the backport manually run:

node scripts/backport --pr 213831

Questions ?

Please refer to the Backport tool documentation

…215581) # Backport This will backport the following commits from `main` to `8.x`: - [[Dashboard] Composable content management transforms (#213831)](#213831)  ### Questions ? Please refer to the [Backport tool documentation](https://github.com/sorenlouv/backport)  Co-authored-by: Nick Peihl <nick.peihl@elastic.co>

nickpeihl added 9 commits March 11, 2025 14:47

Expose content management request method on Dashboard server contract

5d8e12c

Migrate CM transforms to composable functions

811e15e

Add TODO

0d6e682

Revert "Expose content management request method on Dashboard server …

05b0f7b

…contract" This reverts commit b3d4e48.

Use lodash for setting default values

7a2272c

Fix tests

82ecb5f

Pick only supported ignore parent settings on control group

7e3fe72

Adjust types on control state

73f5c5b

Merge remote-tracking branch 'upstream/main' into controls-handle-wid…

092c58a

…th-auto

nickpeihl force-pushed the controls-handle-width-auto branch from a0092bc to 092c58a Compare March 11, 2025 19:15

nickpeihl added Team:Presentation Presentation Team for Dashboard, Input Controls, and Canvas t// release_note:fix labels Mar 11, 2025

nickpeihl commented Mar 11, 2025

View reviewed changes

nickpeihl added 2 commits March 11, 2025 16:45

Fix type

f84a4d5

Explicitly define out properties from saved object

c2448cf

nickpeihl added v9.0.0 v8.18.0 v9.1.0 v8.19.0 backport:version Backport to applied version labels labels Mar 12, 2025

nickpeihl marked this pull request as ready for review March 12, 2025 18:39

nickpeihl requested a review from a team as a code owner March 12, 2025 18:39

thomasneirynck self-requested a review March 17, 2025 14:34

thomasneirynck reviewed Mar 19, 2025

View reviewed changes

ThomThomson self-requested a review March 19, 2025 14:35

Update smoke test to check for working controls

7f2a22d

ThomThomson approved these changes Mar 20, 2025

View reviewed changes

nickpeihl added 4 commits March 21, 2025 08:23

Rename modules

00da442

Document transform

460bf34

Prefer spread operators over lodash defaults

1f430b0

Fix import

a5e94e0

Merge branch 'main' into controls-handle-width-auto

261a057

nickpeihl merged commit f123b50 into elastic:main Mar 21, 2025
10 checks passed

kibanamachine pushed a commit to kibanamachine/kibana that referenced this pull request Mar 21, 2025

[Dashboard] Composable content management transforms (elastic#213831)

05a2142

(cherry picked from commit f123b50)

kibanamachine mentioned this pull request Mar 21, 2025

[8.x] [Dashboard] Composable content management transforms (#213831) #215581

Merged

JoseLuisGJ pushed a commit to JoseLuisGJ/kibana that referenced this pull request Mar 24, 2025

[Dashboard] Composable content management transforms (elastic#213831)

aa2b7ca

cqliu1 pushed a commit to cqliu1/kibana that referenced this pull request Mar 31, 2025

[Dashboard] Composable content management transforms (elastic#213831)

7664f0e

Conversation

nickpeihl commented Mar 10, 2025 • edited by kibanamachine Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Summary

Checklist

Identify risks

Uh oh!

nickpeihl Mar 11, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

elasticmachine commented Mar 12, 2025

Uh oh!

kibanamachine commented Mar 12, 2025

Flaky Test Runner Stats

🎉 All tests passed! - kibana-flaky-test-suite-runner#8024

Uh oh!

thomasneirynck left a comment

Choose a reason for hiding this comment

Uh oh!

nickpeihl commented Mar 20, 2025

Uh oh!

ThomThomson left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

ThomThomson Mar 20, 2025

Choose a reason for hiding this comment

Uh oh!

nickpeihl Mar 21, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

ThomThomson Mar 21, 2025

Choose a reason for hiding this comment

Uh oh!

ThomThomson Mar 20, 2025

Choose a reason for hiding this comment

Uh oh!

nickpeihl Mar 21, 2025

Choose a reason for hiding this comment

Uh oh!

ThomThomson Mar 20, 2025

Choose a reason for hiding this comment

Uh oh!

nickpeihl Mar 21, 2025

Choose a reason for hiding this comment

Uh oh!

nickpeihl commented Mar 21, 2025

Uh oh!

elasticmachine commented Mar 21, 2025

💚 Build Succeeded

Metrics [docs]

History

Uh oh!

Uh oh!

kibanamachine commented Mar 21, 2025

Uh oh!

kibanamachine commented Mar 21, 2025

💔 Some backports could not be created

Manual backport

Questions ?

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

nickpeihl commented Mar 10, 2025 •

edited by kibanamachine

Loading

nickpeihl Mar 11, 2025 •

edited

Loading

nickpeihl Mar 21, 2025 •

edited

Loading