github-mirrors/kibana
1
0
Fork 0
mirror of https://github.com/elastic/kibana.git synced 2025-04-18 23:21:39 -04:00
Code Issues Projects Releases Packages Wiki Activity

[embeddable] fix race condition in useStateFromPublishingSubject (#216522)

Browse source 
Related to https://github.com/elastic/kibana/pull/216399

PR
* updates `useStateFromPublishingSubject` to require `subject`, thus,
removing complexities of setting up subscription when `subject` is
optionally provided.
* Updates `useStateFromPublishingSubject` to setup subscription with
`useMemo` to avoid timing issues.

---------

Co-authored-by: kibanamachine <42973632+kibanamachine@users.noreply.github.com>
Co-authored-by: Elastic Machine <elasticmachine@users.noreply.github.com>
This commit is contained in:
Nathan Reese 2025-04-03 09:35:59 -06:00 • committed by GitHub
parent 4c3274d3a2
commit 5a3c2c0f05
No known key found for this signature in database
GPG key ID: B5690EEEBB952194
11 changed files with 102 additions and 96 deletions
Show stats Download patch file Download diff file Expand all files Collapse all files

6
examples/embeddable_examples/public/react_embeddables/eui_markdown/eui_markdown_react_embeddable.tsx
View file

@ -12,8 +12,8 @@ import { css } from '@emotion/react';
import { ReactEmbeddableFactory } from '@kbn/embeddable-plugin/public';
import { i18n } from '@kbn/i18n';
import {
getViewModeSubject,
initializeTitleManager,
useInheritedViewMode,
useStateFromPublishingSubject,
} from '@kbn/presentation-publishing';
import React from 'react';
@ -78,7 +78,9 @@ export const markdownEmbeddableFactory: ReactEmbeddableFactory<
Component: () => {
// get state for rendering
const content = useStateFromPublishingSubject(content$);
const viewMode = useInheritedViewMode(api) ?? 'view';
const viewMode = useStateFromPublishingSubject(
getViewModeSubject(api) ?? new BehaviorSubject('view')
);
const { euiTheme } = useEuiTheme();
return viewMode === 'edit' ? (

1
src/platform/packages/shared/presentation/presentation_publishing/index.ts
View file

@ -21,7 +21,6 @@ export {
apiCanAccessViewMode,
getInheritedViewMode,
getViewModeSubject,
useInheritedViewMode,
type CanAccessViewMode,
} from './interfaces/can_access_view_mode';
export {

11
src/platform/packages/shared/presentation/presentation_publishing/interfaces/can_access_view_mode.ts
View file

@ -7,7 +7,6 @@
* License v3.0 only", or the "Server Side Public License, v 1".
*/
import { useStateFromPublishingSubject } from '../publishing_subject';
import { apiHasParentApi, HasParentApi } from './has_parent_api';
import { apiPublishesViewMode, PublishesViewMode } from './publishes_view_mode';
@ -42,13 +41,3 @@ export const getViewModeSubject = (api?: CanAccessViewMode) => {
return api.parentApi.viewMode$;
}
};
/**
* A hook that gets a view mode from this API or its parent as a reactive variable which will cause re-renders on change.
* if this api has a view mode AND its parent has a view mode, we consider the APIs version the source of truth.
*/
export const useInheritedViewMode = <ApiType extends CanAccessViewMode = CanAccessViewMode>(
api: ApiType | undefined
) => {
return useStateFromPublishingSubject(getViewModeSubject(api));
};

122
src/platform/packages/shared/presentation/presentation_publishing/publishing_subject/publishing_subject.test.tsx
View file

@ -7,7 +7,7 @@
* License v3.0 only", or the "Server Side Public License, v 1".
*/
import React, { useState } from 'react';
import React, { useMemo, useState } from 'react';
import { BehaviorSubject } from 'rxjs';
import { render, screen, waitFor } from '@testing-library/react';
import '@testing-library/jest-dom';
@ -17,9 +17,71 @@ import {
useBatchedOptionalPublishingSubjects,
} from './publishing_batcher';
import { useStateFromPublishingSubject } from './publishing_subject';
import { PublishingSubject } from './types';
describe('publishing subject', () => {
describe('setup', () => {
let subject1: BehaviorSubject<number>;
beforeEach(() => {
subject1 = new BehaviorSubject<number>(0);
});
function emitEvent() {
subject1.next(subject1.getValue() + 1);
}
test('useStateFromPublishingSubject should synchronously subscribe to observables to avoid race conditions', async () => {
function Component() {
const value1 = useStateFromPublishingSubject(subject1);
// synchronously emit new value for observable
// this will cause test to fail if subscriptions are not setup synchronously
useMemo(() => {
emitEvent();
}, []);
return (
<>
<span>{`value1: ${value1}`}</span>
</>
);
}
render(<Component />);
await waitFor(() => {
expect(
// If there is a race condition, then 'value1: 0' will be rendered
// because value1 will have the original value '0' instead of latest value
screen.getByText('value1: 1')
).toBeInTheDocument();
});
});
test('useBatchedPublishingSubjects should synchronously subscribe to observables to avoid race conditions', async () => {
function Component() {
const [value1] = useBatchedPublishingSubjects(subject1);
// synchronously emit new value for observable
// this will cause test to fail if subscriptions are not setup synchronously
useMemo(() => {
emitEvent();
}, []);
return (
<>
<span>{`value1: ${value1}`}</span>
</>
);
}
render(<Component />);
await waitFor(() => {
expect(
// If there is a race condition, then 'value1: 0' will be rendered
// because value1 will have the original value '0' instead of latest value
screen.getByText('value1: 1')
).toBeInTheDocument();
});
});
});
describe('render', () => {
let subject1: BehaviorSubject<number>;
let subject2: BehaviorSubject<number>;
@ -118,30 +180,6 @@ describe('publishing subject', () => {
expect(renderCount).toBe(2);
});
test('useBatchedPublishingSubjects should synchronously subscribe to observables to avoid race conditions', async () => {
function Component() {
const [value1] = useBatchedPublishingSubjects(subject1);
// synchronously emit new values for observables
// this will cause test to fail if subscriptions are not setup synchronously
incrementAll();
return (
<>
<span>{`value1: ${value1}`}</span>
</>
);
}
render(<Component />);
await waitFor(() => {
expect(
// If there is a race condition, then 'value1: 0' will be rendered
// because value1 will have the original value '0' instead of latest value
screen.getByText('value1: 1')
).toBeInTheDocument();
});
});
test('should batch state updates when using useBatchedOptionalPublishingSubjects', async () => {
let renderCount = 0;
function Component() {
@ -259,37 +297,5 @@ describe('publishing subject', () => {
});
expect(renderCount).toBe(4);
});
test('useStateFromPublishingSubject should update state when publishing subject is provided', async () => {
let renderCount = 0;
function Component() {
// When subject is expected to change, subject must be part of react state.
const [subjectFoo, setSubjectFoo] = useState<PublishingSubject<string> | undefined>(
undefined
);
const valueFoo = useStateFromPublishingSubject(subjectFoo);
renderCount++;
return (
<>
<button
onClick={() => {
setSubjectFoo(new BehaviorSubject<string>('foo'));
}}
/>
<span>{`valueFoo: ${valueFoo}`}</span>
</>
);
}
render(<Component />);
await waitFor(() => {
expect(screen.getByText('valueFoo: undefined')).toBeInTheDocument();
});
await userEvent.click(screen.getByRole('button'));
await waitFor(() => {
expect(screen.getByText('valueFoo: foo')).toBeInTheDocument();
});
expect(renderCount).toBe(3);
});
});
});

27
src/platform/packages/shared/presentation/presentation_publishing/publishing_subject/publishing_subject.ts
View file

@ -7,7 +7,7 @@
* License v3.0 only", or the "Server Side Public License, v 1".
*/
import { useEffect, useMemo, useRef, useState } from 'react';
import { useEffect, useMemo, useState } from 'react';
import { BehaviorSubject, skip } from 'rxjs';
import { PublishingSubject, ValueFromPublishingSubject } from './types';
@ -31,26 +31,21 @@ export const usePublishingSubject = <T extends unknown = unknown>(
/**
* Declares a state variable that is synced with a publishing subject value.
* @param subject Publishing subject.
* When 'subject' is expected to change, 'subject' must be part of component react state.
*/
export const useStateFromPublishingSubject = <
SubjectType extends PublishingSubject<any> | undefined = PublishingSubject<any> | undefined
>(
export const useStateFromPublishingSubject = <SubjectType extends PublishingSubject<any>>(
subject: SubjectType
): ValueFromPublishingSubject<SubjectType> => {
const isFirstRender = useRef(true);
const [value, setValue] = useState<ValueFromPublishingSubject<SubjectType>>(subject?.getValue());
useEffect(() => {
if (!isFirstRender.current) {
setValue(subject?.getValue());
} else {
isFirstRender.current = false;
}
const [value, setValue] = useState<ValueFromPublishingSubject<SubjectType>>(subject.getValue());
if (!subject) return;
const subscription = useMemo(() => {
// When a new observer subscribes to a BehaviorSubject, it immediately receives the current value. Skip this emit.
const subscription = subject.pipe(skip(1)).subscribe((newValue) => setValue(newValue));
return subject.pipe(skip(1)).subscribe((newValue) => setValue(newValue));
// eslint-disable-next-line react-hooks/exhaustive-deps
}, []);
useEffect(() => {
return () => subscription.unsubscribe();
}, [subject]);
}, [subscription]);
return value;
};

4
src/platform/plugins/private/presentation_panel/public/panel_component/presentation_panel_error_internal.tsx
View file

@ -14,7 +14,7 @@ import { ErrorLike } from '@kbn/expressions-plugin/common';
import { EmbeddableApiContext, useStateFromPublishingSubject } from '@kbn/presentation-publishing';
import { renderSearchError } from '@kbn/search-errors';
import { Markdown } from '@kbn/shared-ux-markdown';
import { Subscription, switchMap } from 'rxjs';
import { BehaviorSubject, Subscription, switchMap } from 'rxjs';
import { i18n } from '@kbn/i18n';
import { useErrorTextStyle } from '@kbn/react-hooks';
import { ActionExecutionMeta } from '@kbn/ui-actions-plugin/public';
@ -62,7 +62,7 @@ export const PresentationPanelErrorInternal = ({ api, error }: PresentationPanel
});
}, [api, isEditable]);
const panelTitle = useStateFromPublishingSubject(api?.title$);
const panelTitle = useStateFromPublishingSubject(api?.title$ ?? new BehaviorSubject(undefined));
const ariaLabel = useMemo(
() =>
panelTitle

5
src/platform/plugins/shared/controls/public/controls/data_controls/options_list_control/components/options_list_popover_invalid_selections.tsx
View file

@ -24,6 +24,7 @@ import {
useStateFromPublishingSubject,
} from '@kbn/presentation-publishing';
import { BehaviorSubject } from 'rxjs';
import { useOptionsListContext } from '../options_list_context_provider';
import { OptionsListStrings } from '../options_list_strings';
@ -34,7 +35,9 @@ export const OptionsListPopoverInvalidSelections = () => {
api.invalidSelections$,
api.fieldFormatter
);
const defaultPanelTitle = useStateFromPublishingSubject(api.defaultTitle$);
const defaultPanelTitle = useStateFromPublishingSubject(
api.defaultTitle$ ?? new BehaviorSubject(undefined)
);
const [selectableOptions, setSelectableOptions] = useState<EuiSelectableOption[]>([]); // will be set in following useEffect
useEffect(() => {

5
x-pack/platform/plugins/shared/cases/public/components/visualizations/actions/open_modal.tsx
View file

@ -10,6 +10,7 @@ import { unmountComponentAtNode } from 'react-dom';
import type { LensApi } from '@kbn/lens-plugin/public';
import { toMountPoint } from '@kbn/react-kibana-mount';
import { apiPublishesTimeRange, useStateFromPublishingSubject } from '@kbn/presentation-publishing';
import { BehaviorSubject } from 'rxjs';
import { ActionWrapper } from './action_wrapper';
import type { CasesActionContextProps, Services } from './types';
import type { CaseUI } from '../../../../common';
@ -31,7 +32,9 @@ const AddExistingCaseModalWrapper: React.FC<Props> = ({ lensApi, onClose, onSucc
const timeRange = useStateFromPublishingSubject(lensApi.timeRange$);
const parentTimeRange = useStateFromPublishingSubject(
apiPublishesTimeRange(lensApi.parentApi) ? lensApi.parentApi?.timeRange$ : undefined
apiPublishesTimeRange(lensApi.parentApi)
? lensApi.parentApi?.timeRange$
: new BehaviorSubject(undefined)
);
const absoluteTimeRange = convertToAbsoluteTimeRange(timeRange);
const absoluteParentTimeRange = convertToAbsoluteTimeRange(parentTimeRange);

7
x-pack/platform/plugins/shared/lens/public/app_plugin/shared/edit_on_the_fly/use_esql_variables.ts
View file

@ -8,6 +8,7 @@
import { useMemo, useCallback } from 'react';
import { v4 as uuidv4 } from 'uuid';
import { useStateFromPublishingSubject } from '@kbn/presentation-publishing';
import { BehaviorSubject } from 'rxjs';
import {
isApiESQLVariablesCompatible,
TypedLensSerializedState,
@ -25,10 +26,12 @@ export const useESQLVariables = ({
closeFlyout?: () => void;
}) => {
const dashboardPanels = useStateFromPublishingSubject(
isApiESQLVariablesCompatible(parentApi) ? parentApi?.children$ : undefined
isApiESQLVariablesCompatible(parentApi) ? parentApi?.children$ : new BehaviorSubject(undefined)
);
const controlGroupApi = useStateFromPublishingSubject(
isApiESQLVariablesCompatible(parentApi) ? parentApi?.controlGroupApi$ : undefined
isApiESQLVariablesCompatible(parentApi)
? parentApi?.controlGroupApi$
: new BehaviorSubject(undefined)
);
const panel = useMemo(() => {

5
x-pack/platform/plugins/shared/lens/public/editor_frame_service/editor_frame/config_panel/esql_editor.tsx
View file

@ -16,6 +16,7 @@ import type { ESQLControlVariable } from '@kbn/esql-types';
import { i18n } from '@kbn/i18n';
import React from 'react';
import { DataViewSpec } from '@kbn/data-views-plugin/common';
import { BehaviorSubject } from 'rxjs';
import { useCurrentAttributes } from '../../../app_plugin/shared/edit_on_the_fly/use_current_attributes';
import { getActiveDataFromDatatable } from '../../../state_management/shared_logic';
import type { Simplify } from '../../../types';
@ -106,7 +107,9 @@ export function ESQLEditor({
const previousAdapters = useRef<Partial<DefaultInspectorAdapters> | undefined>(lensAdapters);
const esqlVariables = useStateFromPublishingSubject(
isApiESQLVariablesCompatible(parentApi) ? parentApi?.esqlVariables$ : undefined
isApiESQLVariablesCompatible(parentApi)
? parentApi?.esqlVariables$
: new BehaviorSubject(undefined)
);
const dispatch = useLensDispatch();

5
x-pack/solutions/security/plugins/security_solution/public/dashboards/components/dashboard_tool_bar.test.tsx
View file

@ -15,6 +15,7 @@ import { APP_NAME } from '../../../common/constants';
import { NavigationProvider, SecurityPageName } from '@kbn/security-solution-navigation';
import { TestProviders } from '../../common/mock';
import { useNavigation } from '../../common/lib/kibana';
import { BehaviorSubject } from 'rxjs';
const mockDashboardTopNav = DashboardTopNav as jest.Mock;
@ -33,7 +34,9 @@ jest.mock('@kbn/dashboard-plugin/public', () => ({
const mockCore = coreMock.createStart();
const mockNavigateTo = jest.fn();
const mockGetAppUrl = jest.fn();
const mockDashboardContainer = {} as unknown as DashboardApi;
const mockDashboardContainer = {
viewMode$: new BehaviorSubject('view'),
} as unknown as DashboardApi;
const wrapper = ({ children }: { children: React.ReactNode }) => (
<TestProviders>