Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Handle timeouts in monitor queries in the UI #5519

Merged
merged 10 commits into from
Nov 20, 2024
1 change: 1 addition & 0 deletions CHANGELOG.md
Original file line number Diff line number Diff line change
Expand Up @@ -34,6 +34,7 @@ The types of changes are:
- Allow hiding systems via a `hidden` parameter and add two flags on the `/system` api endpoint; `show_hidden` and `dnd_relevant`, to display only systems with integrations [#5484](https://github.com/ethyca/fides/pull/5484)
- Updated POST taxonomy endpoints to handle creating resources without specifying fides_key [#5468](https://github.com/ethyca/fides/pull/5468)
- Disabled connection pooling for task workers and added retries and keep-alive configurations for database connections [#5448](https://github.com/ethyca/fides/pull/5448)
- Added timeout handling in the UI for async discovery monitor-related queries [#5519](https://github.com/ethyca/fides/pull/5519)

### Developer Experience
- Fixing BigQuery integration tests [#5491](https://github.com/ethyca/fides/pull/5491)
Expand Down
Original file line number Diff line number Diff line change
@@ -1,13 +1,18 @@
import { AntButton as Button, Flex, Text, Tooltip } from "fidesui";
import { AntButton as Button, Flex, Text, Tooltip, useToast } from "fidesui";

import FidesSpinner from "~/features/common/FidesSpinner";
import { usePaginatedPicker } from "~/features/common/hooks/usePicker";
import QuestionTooltip from "~/features/common/QuestionTooltip";
import { DEFAULT_TOAST_PARAMS } from "~/features/common/toast";
import MonitorDatabasePicker from "~/features/integrations/configure-monitor/MonitorDatabasePicker";
import useCumulativeGetDatabases from "~/features/integrations/configure-monitor/useCumulativeGetDatabases";
import { MonitorConfig } from "~/types/api";

const TOOLTIP_COPY =
"Selecting a project will monitor all current and future datasets within that project.";
const TIMEOUT_COPY =
"Loading resources is taking longer than expected. The monitor has been saved and is tracking all available resources. You can return later to limit its scope if needed";

const ConfigureMonitorDatabasesForm = ({
monitor,
isEditing,
Expand All @@ -23,13 +28,26 @@ const ConfigureMonitorDatabasesForm = ({
onSubmit: (monitor: MonitorConfig) => void;
onClose: () => void;
}) => {
const toast = useToast();

const handleTimeout = () => {
onSubmit({ ...monitor, databases: [] });
toast({
...DEFAULT_TOAST_PARAMS,
status: "info",
description: TIMEOUT_COPY,
});
onClose();
};

const {
databases,
totalDatabases: totalRows,
fetchMore,
reachedEnd,
isLoading: refetchPending,
} = useCumulativeGetDatabases(integrationKey);
initialIsLoading,
} = useCumulativeGetDatabases(integrationKey, handleTimeout);

const initialSelected = monitor?.databases ?? [];

Expand Down Expand Up @@ -58,6 +76,10 @@ const ConfigureMonitorDatabasesForm = ({

const saveIsDisabled = !allSelected && selected.length === 0;

if (initialIsLoading) {
return <FidesSpinner my={12} />;
}
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

is this meant to center it? If so, wouldn't using auto margins be a safer bet there?

Suggested change
return <FidesSpinner my={12} />;
return <FidesSpinner my="auto" />;

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is just to give it some room to breathe when it's the only thing in the modal.

No margin or auto:
Screenshot 2024-11-20 at 11 29 15

Constant margin:
Screenshot 2024-11-20 at 11 29 30

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Oh man. My bad. I confused my x and y lol.


return (
<>
<Flex p={4} direction="column">
Expand Down
Original file line number Diff line number Diff line change
@@ -1,8 +1,9 @@
import { UseDisclosureReturn } from "fidesui";
import { UseDisclosureReturn, useToast } from "fidesui";

import FidesSpinner from "~/features/common/FidesSpinner";
import useQueryResultToast from "~/features/common/form/useQueryResultToast";
import FormModal from "~/features/common/modals/FormModal";
import { DEFAULT_TOAST_PARAMS } from "~/features/common/toast";
import {
useGetDatabasesByConnectionQuery,
usePutDiscoveryMonitorMutation,
Expand All @@ -14,7 +15,11 @@ import {
ConnectionSystemTypeMap,
MonitorConfig,
} from "~/types/api";
import { isErrorResult } from "~/types/errors";
import { isErrorResult, RTKResult } from "~/types/errors";

const TIMEOUT_DELAY = 5000;
const TIMEOUT_COPY =
"Saving this monitor is taking longer than expected. Fides will continue processing it in the background, and you can check back later to view the updates.";

const ConfigureMonitorModal = ({
isOpen,
Expand Down Expand Up @@ -44,6 +49,8 @@ const ConfigureMonitorModal = ({

const databasesAvailable = !!databases && !!databases.total;

const toast = useToast();

const { toastResult } = useQueryResultToast({
defaultSuccessMsg: `Monitor ${
isEditing ? "updated" : "created"
Expand All @@ -54,10 +61,24 @@ const ConfigureMonitorModal = ({
});

const handleSubmit = async (values: MonitorConfig) => {
const result = await putMonitorMutationTrigger(values);
toastResult(result);
if (!isErrorResult(result)) {
onClose();
let result: RTKResult | undefined;
const timeout = setTimeout(() => {
if (!result) {
toast({
...DEFAULT_TOAST_PARAMS,
status: "info",
description: TIMEOUT_COPY,
});
onClose();
}
}, TIMEOUT_DELAY);
result = await putMonitorMutationTrigger(values);
if (result) {
clearTimeout(timeout);
toastResult(result);
if (!isErrorResult(result)) {
onClose();
}
}
};

Expand Down
Original file line number Diff line number Diff line change
@@ -1,10 +1,12 @@
import { useState } from "react";
import { useEffect, useRef, useState } from "react";

import {
useGetDatabasesByConnectionQuery,
useLazyGetDatabasesByConnectionQuery,
} from "~/features/data-discovery-and-detection/discovery-detection.slice";

const TIMEOUT_DELAY = 5000;

const EMPTY_RESPONSE = {
items: [] as string[],
total: 1,
Expand All @@ -13,22 +15,45 @@ const EMPTY_RESPONSE = {
pages: 0,
};

const useCumulativeGetDatabases = (integrationKey: string) => {
const useCumulativeGetDatabases = (
integrationKey: string,
onTimeout?: () => void,
) => {
const [nextPage, setNextPage] = useState(2);

const { data: initialResult, isLoading: initialIsLoading } =
useGetDatabasesByConnectionQuery({
page: 1,
size: 25,
connection_config_key: integrationKey,
});

const initialLoadingRef = useRef(initialIsLoading);

const { items: initialDatabases, total: totalDatabases } =
initialResult ?? EMPTY_RESPONSE;

const reachedEnd = !!initialResult?.pages && nextPage > initialResult.pages;

const [databases, setDatabases] = useState<string[]>(initialDatabases);

useEffect(() => {
initialLoadingRef.current = initialIsLoading;
// this needs to be in this hook or else it will be set to [] instead of the actual result
setDatabases(initialDatabases);
}, [initialIsLoading, initialDatabases]);

useEffect(() => {
const t = setTimeout(() => {
if (initialLoadingRef.current && onTimeout) {
onTimeout();
}
}, TIMEOUT_DELAY);
return () => clearTimeout(t);
// this should only run once on mount
// eslint-disable-next-line react-hooks/exhaustive-deps
}, []);

const [
refetchTrigger,
{ isLoading: refetchIsLoading, isFetching: refetchIsFetching },
Expand All @@ -52,7 +77,14 @@ const useCumulativeGetDatabases = (integrationKey: string) => {
setDatabases([...databases, ...(result.data?.items ?? [])]);
};

return { databases, totalDatabases, fetchMore, isLoading, reachedEnd };
return {
databases,
totalDatabases,
fetchMore,
initialIsLoading,
isLoading,
reachedEnd,
};
};

export default useCumulativeGetDatabases;
Loading