Skip to content

Commit

Permalink
[SecuritySolution][Timeline] Remove timeline.isLoading (elastic#198616)
Browse files Browse the repository at this point in the history
## Summary

Trying to answer the question of: "Do we still need
`timeline.isLoading`?"

`timeline.isSaving` should be the only indicator for "loading" states of
timeline itself. All other pieces of state that are associated with
timeline that could have a loading state, have their own loading
indicators (e.g. data providers, alert list etc).

Therefore, this PR removes all references to `timeline.isLoading` and
parts of the UI that depended on it.

Places that `timeline.isLoading` was used
([context](elastic#38185)):
- Blocking drag/drop on data providers
- This is not necessary anymore. Drag/drop works while the underlying
query is being executed.
- Showing a loading state for the alerts table & data provider changes
- Both components have their own loading state, so no extra loading
state is necessary

### Checklist

- [x] [Unit or functional
tests](https://www.elastic.co/guide/en/kibana/master/development-tests.html)
were updated or added to match the most common scenarios

---------

Co-authored-by: Elastic Machine <[email protected]>
  • Loading branch information
janmonschke and elasticmachine authored Nov 18, 2024
1 parent c14c120 commit 4710029
Show file tree
Hide file tree
Showing 28 changed files with 15 additions and 480 deletions.
Original file line number Diff line number Diff line change
Expand Up @@ -15,7 +15,6 @@ interface Props {
children?: React.ReactNode;
droppableId: string;
height?: string;
isDropDisabled?: boolean;
type?: string;
render?: ({ isDraggingOver }: { isDraggingOver: boolean }) => React.ReactNode;
renderClone?: DraggableChildrenFn;
Expand Down Expand Up @@ -90,15 +89,7 @@ const ReactDndDropTarget = styled.div<{ isDraggingOver: boolean; height: string
ReactDndDropTarget.displayName = 'ReactDndDropTarget';

export const DroppableWrapper = React.memo<Props>(
({
children = null,
droppableId,
height = '100%',
isDropDisabled = false,
type,
render = null,
renderClone,
}) => {
({ children = null, droppableId, height = '100%', type, render = null, renderClone }) => {
const DroppableContent = useCallback<DroppableProps['children']>(
(provided, snapshot) => (
<ReactDndDropTarget
Expand All @@ -116,7 +107,6 @@ export const DroppableWrapper = React.memo<Props>(

return (
<Droppable
isDropDisabled={isDropDisabled}
droppableId={droppableId}
direction={'horizontal'}
type={type}
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -358,7 +358,6 @@ export const mockGlobalState: State = {
historyIds: [],
isFavorite: false,
isLive: false,
isLoading: false,
kqlMode: 'filter',
kqlQuery: { filterQuery: null },
loadingEventIds: [],
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -1899,7 +1899,6 @@ export const mockTimelineModel: TimelineModel = {
indexNames: [],
isFavorite: false,
isLive: false,
isLoading: false,
isSaving: false,
isSelectAllChecked: false,
kqlMode: 'filter',
Expand Down Expand Up @@ -2084,7 +2083,6 @@ export const defaultTimelineProps: CreateTimelineProps = {
indexNames: [],
isFavorite: false,
isLive: false,
isLoading: false,
isSaving: false,
isSelectAllChecked: false,
itemsPerPage: 25,
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -30,7 +30,7 @@ import {
mockGetOneTimelineResult,
mockTimelineData,
} from '../../../common/mock';
import type { CreateTimeline, UpdateTimelineLoading } from './types';
import type { CreateTimeline } from './types';
import type { EcsSecurityExtension as Ecs } from '@kbn/securitysolution-ecs';
import type { DataProvider } from '../../../../common/types/timeline';
import { TimelineTypeEnum, TimelineStatusEnum } from '../../../../common/api/timeline';
Expand Down Expand Up @@ -127,7 +127,6 @@ describe('alert actions', () => {
const anchor = '2020-03-01T17:59:46.349Z';
const unix = moment(anchor).valueOf();
let createTimeline: CreateTimeline;
let updateTimelineIsLoading: UpdateTimelineLoading;
let searchStrategyClient: jest.Mocked<ISearchStart>;
let clock: sinon.SinonFakeTimers;
let mockKibanaServices: jest.Mock;
Expand Down Expand Up @@ -270,7 +269,6 @@ describe('alert actions', () => {
mockGetExceptionFilter = jest.fn().mockResolvedValue(undefined);

createTimeline = jest.fn() as jest.Mocked<CreateTimeline>;
updateTimelineIsLoading = jest.fn() as jest.Mocked<UpdateTimelineLoading>;
mockKibanaServices = KibanaServices.get as jest.Mock;

fetchMock = jest.fn();
Expand All @@ -296,28 +294,10 @@ describe('alert actions', () => {

describe('sendAlertToTimelineAction', () => {
describe('timeline id is NOT empty string and apollo client exists', () => {
test('it invokes updateTimelineIsLoading to set to true', async () => {
await sendAlertToTimelineAction({
createTimeline,
ecsData: mockEcsDataWithAlert,
updateTimelineIsLoading,
searchStrategyClient,
getExceptionFilter: mockGetExceptionFilter,
});

expect(mockGetExceptionFilter).not.toHaveBeenCalled();
expect(updateTimelineIsLoading).toHaveBeenCalledTimes(1);
expect(updateTimelineIsLoading).toHaveBeenCalledWith({
id: TimelineId.active,
isLoading: true,
});
});

test('it invokes createTimeline with designated timeline template if "timelineTemplate" exists', async () => {
await sendAlertToTimelineAction({
createTimeline,
ecsData: mockEcsDataWithAlert,
updateTimelineIsLoading,
searchStrategyClient,
getExceptionFilter: mockGetExceptionFilter,
});
Expand Down Expand Up @@ -407,7 +387,6 @@ describe('alert actions', () => {
indexNames: [],
isFavorite: false,
isLive: false,
isLoading: false,
isSaving: false,
isSelectAllChecked: false,
itemsPerPage: 25,
Expand Down Expand Up @@ -477,7 +456,6 @@ describe('alert actions', () => {
await sendAlertToTimelineAction({
createTimeline,
ecsData: mockEcsDataWithAlert,
updateTimelineIsLoading,
searchStrategyClient,
getExceptionFilter: mockGetExceptionFilter,
});
Expand All @@ -496,7 +474,6 @@ describe('alert actions', () => {
await sendAlertToTimelineAction({
createTimeline,
ecsData: mockEcsDataWithAlert,
updateTimelineIsLoading,
searchStrategyClient,
getExceptionFilter: mockGetExceptionFilter,
});
Expand All @@ -505,14 +482,6 @@ describe('alert actions', () => {
delete defaultTimelinePropsWithoutNote.ruleNote;
delete defaultTimelinePropsWithoutNote.ruleAuthor;

expect(updateTimelineIsLoading).toHaveBeenCalledWith({
id: TimelineId.active,
isLoading: true,
});
expect(updateTimelineIsLoading).toHaveBeenCalledWith({
id: TimelineId.active,
isLoading: false,
});
expect(mockGetExceptionFilter).not.toHaveBeenCalled();
expect(createTimeline).toHaveBeenCalledTimes(1);
expect(createTimeline).toHaveBeenCalledWith({
Expand Down Expand Up @@ -544,15 +513,13 @@ describe('alert actions', () => {
await sendAlertToTimelineAction({
createTimeline,
ecsData: ecsDataMock,
updateTimelineIsLoading,
searchStrategyClient,
getExceptionFilter: mockGetExceptionFilter,
});

const expectedTimelineProps = structuredClone(defaultTimelineProps);
expectedTimelineProps.timeline.excludedRowRendererIds = [];

expect(updateTimelineIsLoading).not.toHaveBeenCalled();
expect(mockGetExceptionFilter).not.toHaveBeenCalled();
expect(createTimeline).toHaveBeenCalledTimes(1);
expect(createTimeline).toHaveBeenCalledWith(expectedTimelineProps);
Expand All @@ -574,15 +541,13 @@ describe('alert actions', () => {
await sendAlertToTimelineAction({
createTimeline,
ecsData: ecsDataMock,
updateTimelineIsLoading,
searchStrategyClient,
getExceptionFilter: mockGetExceptionFilter,
});

const expectedTimelineProps = structuredClone(defaultTimelineProps);
expectedTimelineProps.timeline.excludedRowRendererIds = [];

expect(updateTimelineIsLoading).not.toHaveBeenCalled();
expect(mockGetExceptionFilter).not.toHaveBeenCalled();
expect(createTimeline).toHaveBeenCalledTimes(1);
expect(createTimeline).toHaveBeenCalledWith(expectedTimelineProps);
Expand All @@ -608,12 +573,10 @@ describe('alert actions', () => {
await sendAlertToTimelineAction({
createTimeline,
ecsData: ecsDataMock,
updateTimelineIsLoading,
searchStrategyClient,
getExceptionFilter: mockGetExceptionFilter,
});

expect(updateTimelineIsLoading).not.toHaveBeenCalled();
expect(mockGetExceptionFilter).not.toHaveBeenCalled();
expect(createTimeline).toHaveBeenCalledTimes(1);
expect(createTimeline).toHaveBeenCalledWith({
Expand Down Expand Up @@ -655,12 +618,10 @@ describe('alert actions', () => {
await sendAlertToTimelineAction({
createTimeline,
ecsData: ecsDataMock,
updateTimelineIsLoading,
searchStrategyClient,
getExceptionFilter: mockGetExceptionFilter,
});

expect(updateTimelineIsLoading).not.toHaveBeenCalled();
expect(mockGetExceptionFilter).not.toHaveBeenCalled();
expect(createTimeline).toHaveBeenCalledTimes(1);
expect(createTimeline).toHaveBeenCalledWith(expectedTimelineProps);
Expand Down Expand Up @@ -732,15 +693,13 @@ describe('alert actions', () => {
await sendAlertToTimelineAction({
createTimeline,
ecsData: ecsDataMockWithNoTemplateTimeline,
updateTimelineIsLoading,
searchStrategyClient,
getExceptionFilter: mockGetExceptionFilter,
});

const expectedFrom = '2021-01-10T21:11:45.839Z';
const expectedTo = '2021-01-10T21:12:45.839Z';

expect(updateTimelineIsLoading).not.toHaveBeenCalled();
expect(mockGetExceptionFilter).toHaveBeenCalled();
expect(createTimeline).toHaveBeenCalledTimes(1);
expect(createTimeline).toHaveBeenCalledWith({
Expand Down Expand Up @@ -861,7 +820,6 @@ describe('alert actions', () => {
await sendAlertToTimelineAction({
createTimeline,
ecsData: ecsDataMockWithNoTemplateTimelineAndNoFilters,
updateTimelineIsLoading,
searchStrategyClient,
getExceptionFilter: mockGetExceptionFilter,
});
Expand All @@ -886,15 +844,13 @@ describe('alert actions', () => {
await sendAlertToTimelineAction({
createTimeline,
ecsData: ecsDataMockWithTemplateTimeline,
updateTimelineIsLoading,
searchStrategyClient,
getExceptionFilter: mockGetExceptionFilter,
});

const expectedFrom = '2021-01-10T21:11:45.839Z';
const expectedTo = '2021-01-10T21:12:45.839Z';

expect(updateTimelineIsLoading).toHaveBeenCalled();
expect(mockGetExceptionFilter).toHaveBeenCalled();
expect(createTimeline).toHaveBeenCalledTimes(1);
expect(createTimeline).toHaveBeenCalledWith({
Expand Down Expand Up @@ -1046,7 +1002,6 @@ describe('alert actions', () => {
await sendAlertToTimelineAction({
createTimeline,
ecsData: ecsDataMockWithNoTemplateTimeline,
updateTimelineIsLoading,
searchStrategyClient,
getExceptionFilter: mockGetExceptionFilter,
});
Expand Down Expand Up @@ -1141,7 +1096,6 @@ describe('alert actions', () => {
await sendAlertToTimelineAction({
createTimeline,
ecsData: ecsDataMockWithNoTemplateTimeline,
updateTimelineIsLoading,
searchStrategyClient,
getExceptionFilter: mockGetExceptionFilter,
});
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -936,7 +936,6 @@ export const sendBulkEventsToTimelineAction = async (
export const sendAlertToTimelineAction = async ({
createTimeline,
ecsData: ecs,
updateTimelineIsLoading,
searchStrategyClient,
getExceptionFilter,
}: SendAlertToTimelineActionProps) => {
Expand All @@ -962,7 +961,6 @@ export const sendAlertToTimelineAction = async ({
// For now we do not want to populate the template timeline if we have alertIds
if (!isEmpty(timelineId)) {
try {
updateTimelineIsLoading({ id: TimelineId.active, isLoading: true });
const [responseTimeline, eventDataResp] = await Promise.all([
getTimelineTemplate(timelineId),
lastValueFrom(
Expand Down Expand Up @@ -1092,7 +1090,6 @@ export const sendAlertToTimelineAction = async ({
} catch (error) {
/* eslint-disable-next-line no-console */
console.error(error);
updateTimelineIsLoading({ id: TimelineId.active, isLoading: false });
return createTimeline({
from,
notes: null,
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -23,7 +23,6 @@ import { useTimelineEventsHandler } from '../../../../timelines/containers';
import { eventsViewerSelector } from '../../../../common/components/events_viewer/selectors';
import type { State } from '../../../../common/store/types';
import { useUpdateTimeline } from '../../../../timelines/components/open_timeline/use_update_timeline';
import { timelineActions } from '../../../../timelines/store';
import { useCreateTimeline } from '../../../../timelines/hooks/use_create_timeline';
import { INVESTIGATE_BULK_IN_TIMELINE } from '../translations';
import { TimelineId } from '../../../../../common/types/timeline';
Expand Down Expand Up @@ -141,18 +140,11 @@ export const useAddBulkToTimelineAction = ({
timelineType: TimelineTypeEnum.default,
});

const updateTimelineIsLoading = useCallback(
(payload: Parameters<typeof timelineActions.updateIsLoading>[0]) =>
dispatch(timelineActions.updateIsLoading(payload)),
[dispatch]
);

const updateTimeline = useUpdateTimeline();

const createTimeline = useCallback(
async ({ timeline, ruleNote, timeline: { filters: eventIdFilters } }: CreateTimelineProps) => {
await clearActiveTimeline();
updateTimelineIsLoading({ id: TimelineId.active, isLoading: false });
updateTimeline({
duplicate: true,
from,
Expand All @@ -168,7 +160,7 @@ export const useAddBulkToTimelineAction = ({
ruleNote,
});
},
[updateTimeline, updateTimelineIsLoading, clearActiveTimeline, from, to]
[updateTimeline, clearActiveTimeline, from, to]
);

const sendBulkEventsToTimelineHandler = useCallback(
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -5,7 +5,6 @@
* 2.0.
*/
import { useCallback, useMemo } from 'react';
import { useDispatch } from 'react-redux';

import { i18n } from '@kbn/i18n';
import { ALERT_RULE_EXCEPTIONS_LIST, ALERT_RULE_PARAMETERS } from '@kbn/rule-data-utils';
Expand All @@ -23,7 +22,6 @@ import { createHistoryEntry } from '../../../../common/utils/global_query_string
import { useKibana } from '../../../../common/lib/kibana';
import { TimelineId } from '../../../../../common/types/timeline';
import { TimelineTypeEnum } from '../../../../../common/api/timeline';
import { timelineActions } from '../../../../timelines/store';
import { sendAlertToTimelineAction } from '../actions';
import { useUpdateTimeline } from '../../../../timelines/components/open_timeline/use_update_timeline';
import { useCreateTimeline } from '../../../../timelines/hooks/use_create_timeline';
Expand Down Expand Up @@ -98,7 +96,6 @@ export const useInvestigateInTimeline = ({
const {
data: { search: searchStrategyClient },
} = useKibana().services;
const dispatch = useDispatch();
const { startTransaction } = useStartTransaction();

const { services } = useKibana();
Expand Down Expand Up @@ -133,12 +130,6 @@ export const useInvestigateInTimeline = ({
[addError, getExceptionFilterFromIds]
);

const updateTimelineIsLoading = useCallback(
(payload: Parameters<typeof timelineActions.updateIsLoading>[0]) =>
dispatch(timelineActions.updateIsLoading(payload)),
[dispatch]
);

const clearActiveTimeline = useCreateTimeline({
timelineId: TimelineId.active,
timelineType: TimelineTypeEnum.default,
Expand All @@ -153,7 +144,6 @@ export const useInvestigateInTimeline = ({
!newColumns || isEmpty(newColumns) ? defaultUdtHeaders : newColumns;

await clearActiveTimeline();
updateTimelineIsLoading({ id: TimelineId.active, isLoading: false });
updateTimeline({
duplicate: true,
from: fromTimeline,
Expand All @@ -173,12 +163,11 @@ export const useInvestigateInTimeline = ({
ruleNote,
});
},
[updateTimeline, updateTimelineIsLoading, clearActiveTimeline]
[updateTimeline, clearActiveTimeline]
);

const investigateInTimelineAlertClick = useCallback(async () => {
createHistoryEntry();

startTransaction({ name: ALERTS_ACTIONS.INVESTIGATE_IN_TIMELINE });
if (onInvestigateInTimelineAlertClick) {
onInvestigateInTimelineAlertClick();
Expand All @@ -188,7 +177,6 @@ export const useInvestigateInTimeline = ({
createTimeline,
ecsData: ecsRowData,
searchStrategyClient,
updateTimelineIsLoading,
getExceptionFilter,
});
}
Expand All @@ -198,7 +186,6 @@ export const useInvestigateInTimeline = ({
ecsRowData,
onInvestigateInTimelineAlertClick,
searchStrategyClient,
updateTimelineIsLoading,
getExceptionFilter,
]);

Expand Down
Loading

0 comments on commit 4710029

Please sign in to comment.