From feb5b79a96162b0259ecfd5d8216050cd074a3a4 Mon Sep 17 00:00:00 2001
From: Anton Dosov <anton.dosov@elastic.co>
Date: Tue, 22 Oct 2024 11:50:15 +0200
Subject: [PATCH] [React@18 failing test] fix visualize app - new charts
 library visualize area charts date histogram when no time filter interval
 errors should show error when calendar interval invalid (#196308)

## Summary

close https://github.com/elastic/kibana/issues/196303

We're working on upgrading Kibana to React@18 (in Legacy Mode). There
are a couple failing tests when running React@18 in Legacy mode and this
is one of them

visualize app - new charts library visualize area charts date histogram
when no time filter interval errors should show error when calendar
interval invalid.
[Failure](https://buildkite.com/elastic/kibana-pull-request/builds/236562#019222ec-e8d1-4465-ada3-fd923283b6f4)


![Image](https://github.com/user-attachments/assets/2cdaca3c-9ccf-4208-b6f3-6975588eb5fe)


----


I investigated the problem and understand what is not working and
suggesting this simple fix, but open to any other approaches or
suggestions.


To Reproduce the failing tests:

1. Create aggregation based viz, e.g. Area Chart
2. Add data histogram agg
3. Change minimum interval to a invalid value (for example, "f")
4. Change minimum interval to another invalid value ("ff")

React@18 failure:


https://github.com/user-attachments/assets/f8684b48-fb24-4500-a762-2a116ed55297

The error is thrown from here in the reducer:


https://github.com/elastic/kibana/blob/23e0e1e61c6df451cc38763b53a6e2db5518b9f4/src/plugins/vis_default_editor/public/components/sidebar/state/reducers.ts#L82

When we try to update the state using 2nd invalid value, the error is
thrown when we try to serialize the current agg with previous invalid
value.

This code is exececuted when we call `agg.serialize`:


https://github.com/elastic/kibana/blob/5ed698182887e18d2aa6c4b6782cc636a45a1472/src/plugins/data/common/search/aggs/buckets/lib/time_buckets/time_buckets.ts#L200-L202


**Why don't we see this failure in React@17?**

In React@17 we don't see an error screen, but we only see a log in the
console.

> TypeError: "f" is not a valid interval.

It turns out that React@17 consistently executed that reducer twice.
first time during dispatch and second time during rendering. This
shouldn't be a problem because reducers are supposed to be pure (without
side-effects). **But in this case calling `agg.serialize` only throws an
error when called the first time**! So in React@17 the reducer was
called the first time, the error was swallowed, then it was called the
2nd time and, since the `TimeBucket` was cached, there was no error
anymore, so it never bubbled up during rendering.

The root cause of inconsitent behaviour is here:


https://github.com/elastic/kibana/blob/8afbbc008222dee377aab568a639466d49c56306/src/plugins/data/common/search/aggs/buckets/date_histogram.ts#L111-L121

when `get()` called first time we create buckets and cache them. but we
cache them before calling `updateTimeBuckets` which is where the error
happens.


To fix this issue, we should make the reducer pure. One approach is to
swallow that error so that the call to `agg.serialize()` is consistent.
Another approach could be to always throw that error, but then a larger
refactor is needed and this likely a more risky and impactfull change.
---
 .../data/common/search/aggs/buckets/date_histogram.ts    | 9 ++++++++-
 1 file changed, 8 insertions(+), 1 deletion(-)

diff --git a/src/plugins/data/common/search/aggs/buckets/date_histogram.ts b/src/plugins/data/common/search/aggs/buckets/date_histogram.ts
index 66820ff0d3e94..314ee4d03042c 100644
--- a/src/plugins/data/common/search/aggs/buckets/date_histogram.ts
+++ b/src/plugins/data/common/search/aggs/buckets/date_histogram.ts
@@ -116,7 +116,14 @@ export const getDateHistogramBucketAgg = ({
               dateFormat: getConfig('dateFormat'),
               'dateFormat:scaled': getConfig('dateFormat:scaled'),
             });
-            updateTimeBuckets(this, calculateBounds, buckets);
+
+            try {
+              updateTimeBuckets(this, calculateBounds, buckets);
+            } catch (e) {
+              // swallow the error even though the agg is misconfigured
+              // eslint-disable-next-line no-console
+              console.error(e);
+            }
 
             return buckets;
           },