[Dashboard] Update Index Patterns when Child Index Patterns Change

[Dosant]

Closes #76171
(alternative to #76191)

Summary

Makes sure dashboard app controller updates the list of index patterns any time the indexPatterns key in the output of a child embeddable changes. This allows embeddables to lazily load the list of index patterns instead of requiring them upfront.

Skipping the test for this change, because dashboard_app_controller code is not testable (no units there 😢 ) and from functional test perspective this can be only covered in future lensByValue pr. @ThomThomson, maybe this is something you could try to cover in your future functional test?

[ThomThomson]

Copying our discussion here for discoverability:

This fix unfortunately does not work with the Lens by Value code as it is.

How I tested:

1. Merge changes into the lensByValue branch.
2. Create a new dashboard, see that the available filters are related to the default index pattern
3. Add a lens embeddable that uses a non-default index-pattern
4. See that the available filters have changed to reflect the non-default index pattern

Why doesn't this work?

In the Lens by value PR, some of the setup has been shifted from the lens embeddable factory, to the lens embeddable itself. This means that the embeddable does not know its index patterns fully at construction time. The super call looks like this now:

// in constructor
super(
  initialInput,
  {
    editApp: 'lens',
    editable: deps.editable,
  },
  parent
);

The index patterns are fetched later async, and updated via updateOutput:

this.updateOutput({
  ...this.getOutput(),
  defaultTitle: this.savedVis.title,
  title,
  editPath: getEditPath(savedObjectId),
  editUrl: this.deps.basePath.prepend(`/app/lens${getEditPath(savedObjectId)}`),
  indexPatterns,
});

Because this change doesn't cause the dashboard's getOutput$ or getInput$ to emit a new value, the change is currently lost.

Next steps
We will move towards a solution similar to #76191, but hopefully the RXJS implementation can be simplified.

[ThomThomson]

This is definitely getting closer. It initially didn't update properly due to being one behind on the childIds (i.e. not reflecting the most recent change) I fixed this by switching the subscription to getOutput$() instead of getInput$().

Even after that change, it appeared not to be picking up output updates on children between dashboard output updates - which is where the lens embeddable now adds its indexPatterns.

I was able to fix that, by switching out merge for combineLatest. After both changes, the dashboard picks up on indexPattern changes as expected. Here is the outputSubscription as it looks currently.

outputSubscription = merge(
  // output of dashboard container itself
  dashboardContainer.getOutput$(),
  // plus output of dashboard container children,
  // children may change, so make sure we subscribe/unsubscribe with switchMap
  dashboardContainer.getOutput$().pipe(
    map(() => dashboardContainer!.getChildIds()),
    distinctUntilChanged(deepEqual),
    switchMap((newChildIds: string[]) =>
      combineLatest(
        newChildIds.map((childId) => dashboardContainer!.getChild(childId).getOutput$())
      )
    )
  )
)

This is simpler than my original implementation, but I would still appreciate a once-over. Is combineLatest the right choice here? Why doesn't merge behave as expected in this case? I will do a little bit more research

Thanks again for doing this @Dosant

[Dosant]

@ThomThomson, besides combineLatest, I see you've also changed getInput -> getOutput in second source. is this right? Or is this a mistake in your snippet?

[ThomThomson]

This is on purpose, getInput was giving a childIds array that was lagging behind what was actually there. GetOutput has the up to date version. Not sure if getInput is supposed to be up to date, but it makes sense to me that it wouldn't be, because the input observable might fire before the panel is finalized, but the output observable would only fire after.

[ThomThomson]

@elasticmachine merge upstream

[ThomThomson]

Tested in Chrome, works as expected!

Good catch with the merge / combineLatest syntax confusion, and this is a much simpler solution overall.

[kibanamachine]

💚 Build Succeeded

• continuous-integration/kibana-ci/pull-request
• Commit: d6140fb

Build metrics

@kbn/optimizer bundle module count

id	value	diff	baseline
dashboard	178	+1	177

page load bundle size

id	value	diff	baseline
dashboard	709.8KB	+1.6KB	708.2KB

History

• 💚 Build #72571 succeeded db4fb1f
• 💚 Build #71824 succeeded 2ededee
• 💚 Build #71510 succeeded 49f20e3

To update your PR or re-run it, just comment with:
@elasticmachine merge upstream

[ThomThomson]

I will add a functional test to the LensByValue PR to make sure that the index patterns are reflected correctly. This test won't rely on the feature flag because it will work the same way regardless of 'by value' or 'by reference'