[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

[Dosant]

Summary

close #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

---

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:

error.mov

The error is thrown from here in the reducer:

kibana/src/plugins/vis_default_editor/public/components/sidebar/state/reducers.ts

Line 82 in 23e0e1e

const parsedAgg = agg.serialize();

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:

kibana/src/plugins/data/common/search/aggs/buckets/lib/time_buckets/time_buckets.ts

Lines 200 to 202 in 5ed6981

	if (!isDurationInterval(this._i)) {
	throw new TypeError('"' + this._i + '" is not a valid interval.');
	}

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:

kibana/src/plugins/data/common/search/aggs/buckets/date_histogram.ts

Lines 111 to 121 in 8afbbc0

	if (buckets) return buckets;
	buckets = new TimeBuckets({
	'histogram:maxBars': getConfig(UI_SETTINGS.HISTOGRAM_MAX_BARS),
	'histogram:barTarget': getConfig(UI_SETTINGS.HISTOGRAM_BAR_TARGET),
	dateFormat: getConfig('dateFormat'),
	'dateFormat:scaled': getConfig('dateFormat:scaled'),
	});
	updateTimeBuckets(this, calculateBounds, buckets);
	return buckets;

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.

[elasticmachine]

Pinging @elastic/appex-sharedux (Team:SharedUX)

[Dosant]

@elasticmachine merge upstream

[elasticmachine]

💔 Build Failed

• Buildkite Build
• Commit: 3ddba7e

Failed CI Steps

• FTR Configs #80
• Checks

Test Failures

• [job] [logs] FTR Configs #80 / home app sample data installing should install logs sample data set

Metrics [docs]

Page load bundle

Size of the bundles that are downloaded on every page load. Target size is below 100kb

id	before	after	diff
data	419.9KB	419.9KB	+39.0B

Unknown metric groups

ESLint disabled line counts

id	before	after	diff
data	55	56	+1

Total ESLint disabled count

id	before	after	diff
data	57	58	+1

History

[Dosant]

@elasticmachine merge upstream

[Dosant]

I think this is safe and shouldn't break anything because this is inside the accessor and previosly threw error only on a first attempt to access. Now at least it would be consistent

[davismcphee]

Code-only review, change LGTM from the Data Discovery side. We should also get approval from @elastic/kibana-visualizations too.

[Dosant]

@elasticmachine merge upstream

[kibanamachine]

Starting backport for target branches: 8.x

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

[kibanamachine]

💚 All backports created successfully

Status	Branch	Result
✅	8.x

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

Questions ?

Please refer to the Backport tool documentation