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

🪟 🐛 Fix loading state of "Launch" button in connection table #22002

Merged
merged 10 commits into from
Jan 31, 2023
6 changes: 2 additions & 4 deletions airbyte-webapp/src/components/EntityTable/ConnectionTable.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -22,10 +22,9 @@ interface IProps {
data: ITableDataItem[];
entity: "source" | "destination" | "connection";
onClickRow?: (data: ITableDataItem) => void;
onSync: (id: string) => void;
}

const ConnectionTable: React.FC<IProps> = ({ data, entity, onClickRow, onSync }) => {
const ConnectionTable: React.FC<IProps> = ({ data, entity, onClickRow }) => {
const navigate = useNavigate();
const query = useQuery<{ sortBy?: string; order?: SortOrderEnum }>();
const allowAutoDetectSchema = useFeature(FeatureItem.AllowAutoDetectSchema);
Expand Down Expand Up @@ -162,7 +161,6 @@ const ConnectionTable: React.FC<IProps> = ({ data, entity, onClickRow, onSync })
id={row.original.connectionId}
isSyncing={row.original.isSyncing}
isManual={row.original.scheduleType === ConnectionScheduleType.manual}
onSync={onSync}
hasBreakingChange={allowAutoDetectSchema && row.original.schemaChange === SchemaChange.breaking}
allowSync={allowSync}
/>
Expand All @@ -175,7 +173,7 @@ const ConnectionTable: React.FC<IProps> = ({ data, entity, onClickRow, onSync })
Cell: ({ cell }: CellProps<ITableDataItem>) => <ConnectionSettingsCell id={cell.value} />,
},
],
[sortBy, sortOrder, entity, onSortClick, onSync, allowSync, allowAutoDetectSchema]
[sortBy, sortOrder, entity, onSortClick, allowSync, allowAutoDetectSchema]
);

return <Table columns={columns} data={sortingData} onClickRow={onClickRow} erroredRows testId="connectionsTable" />;
Expand Down
Original file line number Diff line number Diff line change
@@ -1,8 +1,20 @@
import { render } from "@testing-library/react";
import { TestWrapper } from "test-utils/testutils";
import { render, waitFor } from "@testing-library/react";
import { TestWrapper, TestSuspenseBoundary } from "test-utils";

import { StatusCell } from "./StatusCell";

jest.mock("hooks/services/useConnectionHook", () => ({
useConnectionList: jest.fn(() => ({
connections: [],
})),
useEnableConnection: jest.fn(() => ({
mutateAsync: jest.fn(),
})),
useSyncConnection: jest.fn(() => ({
mutateAsync: jest.fn(),
})),
}));

const mockId = "mock-id";

jest.doMock("hooks/services/useConnectionHook", () => ({
Expand All @@ -14,35 +26,52 @@ jest.doMock("hooks/services/useConnectionHook", () => ({

describe("<StatusCell />", () => {
it("renders switch when connection has schedule", () => {
const { getByTestId } = render(<StatusCell id={mockId} onSync={jest.fn()} allowSync enabled />, {
wrapper: TestWrapper,
});
const { getByTestId } = render(
<TestSuspenseBoundary>
<StatusCell id={mockId} allowSync enabled />
</TestSuspenseBoundary>,
{
wrapper: TestWrapper,
}
);

const switchElement = getByTestId("enable-connection-switch");

expect(switchElement).toBeEnabled();
expect(switchElement).toBeChecked();
});

it("renders button when connection does not have schedule", () => {
const { getByTestId } = render(<StatusCell id={mockId} onSync={jest.fn()} allowSync enabled isManual />, {
wrapper: TestWrapper,
});
it("renders button when connection does not have schedule", async () => {
const { getByTestId } = render(
<TestSuspenseBoundary>
<StatusCell id={mockId} allowSync enabled isManual />
</TestSuspenseBoundary>,
{
wrapper: TestWrapper,
}
);

expect(getByTestId("manual-sync-button")).toBeEnabled();
await waitFor(() => expect(getByTestId("manual-sync-button")).toBeEnabled());
});

it("disables switch when hasBreakingChange is true", () => {
const { getByTestId } = render(<StatusCell id={mockId} onSync={jest.fn()} allowSync hasBreakingChange />, {
wrapper: TestWrapper,
});
const { getByTestId } = render(
<TestSuspenseBoundary>
<StatusCell id={mockId} allowSync hasBreakingChange />
</TestSuspenseBoundary>,
{
wrapper: TestWrapper,
}
);

expect(getByTestId("enable-connection-switch")).toBeDisabled();
});

it("disables manual sync button when hasBreakingChange is true", () => {
const { getByTestId } = render(
<StatusCell id={mockId} onSync={jest.fn()} allowSync hasBreakingChange enabled isManual />,
<TestSuspenseBoundary>
<StatusCell id={mockId} allowSync hasBreakingChange enabled isManual />
</TestSuspenseBoundary>,
{
wrapper: TestWrapper,
}
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -14,7 +14,6 @@ interface StatusCellProps {
isSyncing?: boolean;
isManual?: boolean;
id: string;
onSync: (id: string) => void;
schemaChange?: SchemaChange;
}

Expand All @@ -23,7 +22,6 @@ export const StatusCell: React.FC<StatusCellProps> = ({
isManual,
id,
isSyncing,
onSync,
allowSync,
schemaChange,
hasBreakingChange,
Expand All @@ -37,7 +35,6 @@ export const StatusCell: React.FC<StatusCellProps> = ({
id={id}
isSyncing={isSyncing}
isManual={isManual}
onSync={onSync}
hasBreakingChange={hasBreakingChange}
allowSync={allowSync}
/>
Expand Down
Original file line number Diff line number Diff line change
@@ -0,0 +1,5 @@
.inProgressLabel {
height: 32px; /** Needs to match our Button height to avoid jumping around */
Copy link
Contributor

Choose a reason for hiding this comment

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

Should we make this a variable? I've also had another occasion where I needed the button height

Copy link
Contributor Author

@josephkmh josephkmh Jan 31, 2023

Choose a reason for hiding this comment

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

The reason I didn't is that IMO we should not really need to pass around the height of the button. The table row should have an appropriate (larger) height so that it's not reflowing depending on the content. But of course that's a little out of scope. On second thought, a variable is at least better than a comment like this, so I'll add the button heights to _variables.scss

aeb9344

display: flex;
align-items: center;
}
Original file line number Diff line number Diff line change
@@ -1,12 +1,12 @@
import React from "react";
import { FormattedMessage } from "react-intl";
import { useAsyncFn } from "react-use";
import styled from "styled-components";

import { Button } from "components/ui/Button";
import { Switch } from "components/ui/Switch";

import { useEnableConnection } from "hooks/services/useConnectionHook";
import { useConnectionList, useEnableConnection, useSyncConnection } from "hooks/services/useConnectionHook";

import styles from "./StatusCellControl.module.scss";

interface StatusCellControlProps {
allowSync?: boolean;
Expand All @@ -15,30 +15,28 @@ interface StatusCellControlProps {
isSyncing?: boolean;
isManual?: boolean;
id: string;
onSync: (id: string) => void;
}

const ProgressMessage = styled.div`
padding: 7px 0;
`;

export const StatusCellControl: React.FC<StatusCellControlProps> = ({
enabled,
isManual,
id,
isSyncing,
onSync,
allowSync,
hasBreakingChange,
}) => {
const { connections } = useConnectionList();
const { mutateAsync: enableConnection, isLoading } = useEnableConnection();
const [{ loading }, OnLaunch] = useAsyncFn(
async (event: React.SyntheticEvent) => {
event.stopPropagation();
onSync(id);
},
[id]
);
const { mutateAsync: syncConnection, isLoading: isSyncStarting } = useSyncConnection();

const onRunManualSync = (event: React.SyntheticEvent) => {
event.stopPropagation();

const connection = connections.find((c) => c.connectionId === id);
if (connection) {
syncConnection(connection);
}
};
Comment on lines +32 to +39
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 function was basically constructed in three places before (SourceConnectionTable, DestinationConnectionTable and ConnectionTable) and drilled down as prop called onSync. Instead we can just create the function here and remove all that intermediate code.


if (!isManual) {
const onSwitchChange = async (event: React.SyntheticEvent) => {
Expand Down Expand Up @@ -69,21 +67,20 @@ export const StatusCellControl: React.FC<StatusCellControlProps> = ({

if (isSyncing) {
return (
<ProgressMessage>
<FormattedMessage id="tables.progress" />
</ProgressMessage>
<div className={styles.inProgressLabel}>
<FormattedMessage id="connection.syncInProgress" />
</div>
);
}

return (
<Button
size="xs"
onClick={OnLaunch}
isLoading={loading}
disabled={!allowSync || !enabled || hasBreakingChange}
onClick={onRunManualSync}
isLoading={isSyncStarting}
disabled={!allowSync || !enabled || hasBreakingChange || isSyncStarting}
data-testid="manual-sync-button"
>
<FormattedMessage id="tables.launch" />
<FormattedMessage id="connection.startSync" />
</Button>
);
};
16 changes: 0 additions & 16 deletions airbyte-webapp/src/components/EntityTable/hooks.tsx

This file was deleted.

Original file line number Diff line number Diff line change
@@ -1,8 +1,7 @@
import React, { useCallback } from "react";
import React from "react";
import { useNavigate } from "react-router-dom";

import { ConnectionTable } from "components/EntityTable";
import useSyncActions from "components/EntityTable/hooks";
import { ITableDataItem } from "components/EntityTable/types";
import { getConnectionTableData } from "components/EntityTable/utils";

Expand All @@ -17,25 +16,14 @@ interface IProps {

export const DestinationConnectionTable: React.FC<IProps> = ({ connections }) => {
const navigate = useNavigate();
const { syncManualConnection } = useSyncActions();

const data = getConnectionTableData(connections, "destination");

const onSync = useCallback(
async (connectionId: string) => {
const connection = connections.find((item) => item.connectionId === connectionId);
if (connection) {
await syncManualConnection(connection);
}
},
[connections, syncManualConnection]
);

const clickRow = (source: ITableDataItem) => navigate(`../../../${RoutePaths.Connections}/${source.connectionId}`);

return (
<div className={styles.content}>
<ConnectionTable data={data} onClickRow={clickRow} entity="destination" onSync={onSync} />
<ConnectionTable data={data} onClickRow={clickRow} entity="destination" />
</div>
);
};
59 changes: 45 additions & 14 deletions airbyte-webapp/src/hooks/services/useConnectionHook.tsx
Original file line number Diff line number Diff line change
@@ -1,6 +1,9 @@
import { useCallback } from "react";
import { useIntl } from "react-intl";
import { useMutation, useQueryClient } from "react-query";

import { ToastType } from "components/ui/Toast";

import { Action, Namespace } from "core/analytics";
import { getFrequencyFromScheduleData } from "core/analytics/utils";
import { SyncSchema } from "core/domain/catalog";
Expand All @@ -9,6 +12,8 @@ import { ConnectionService } from "core/domain/connection/ConnectionService";
import { useInitService } from "services/useInitService";

import { useAnalyticsService } from "./Analytics";
import { useAppMonitoringService } from "./AppMonitoringService";
import { useNotificationService } from "./Notification";
import { useCurrentWorkspace } from "./useWorkspace";
import { useConfig } from "../../config";
import {
Expand Down Expand Up @@ -74,20 +79,43 @@ export function useConnectionService() {

export const useSyncConnection = () => {
const service = useConnectionService();
const webConnectionService = useWebConnectionService();
const { trackError } = useAppMonitoringService();
const queryClient = useQueryClient();
const analyticsService = useAnalyticsService();
const notificationService = useNotificationService();
const workspaceId = useCurrentWorkspace();
const { formatMessage } = useIntl();

return useMutation((connection: WebBackendConnectionRead | WebBackendConnectionListItem) => {
analyticsService.track(Namespace.CONNECTION, Action.SYNC, {
actionDescription: "Manual triggered sync",
connector_source: connection.source?.sourceName,
connector_source_definition_id: connection.source?.sourceDefinitionId,
connector_destination: connection.destination?.destinationName,
connector_destination_definition_id: connection.destination?.destinationDefinitionId,
frequency: getFrequencyFromScheduleData(connection.scheduleData),
});

return service.sync(connection.connectionId);
});
return useMutation(
(connection: WebBackendConnectionRead | WebBackendConnectionListItem) => {
analyticsService.track(Namespace.CONNECTION, Action.SYNC, {
actionDescription: "Manual triggered sync",
connector_source: connection.source?.sourceName,
connector_source_definition_id: connection.source?.sourceDefinitionId,
connector_destination: connection.destination?.destinationName,
connector_destination_definition_id: connection.destination?.destinationDefinitionId,
frequency: getFrequencyFromScheduleData(connection.scheduleData),
});

return service.sync(connection.connectionId);
},
{
onError: (error: Error) => {
trackError(error);
notificationService.registerNotification({
id: `tables.startSyncError.${error.message}`,
text: `${formatMessage({ id: "connection.startSyncError" })}: ${error.message}`,
type: ToastType.ERROR,
});
},
onSuccess: async () => {
webConnectionService
Copy link
Contributor

Choose a reason for hiding this comment

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

We may need to await here. I noticed that the button goes into a non-loading state for the time the list is loading.

Copy link
Contributor Author

@josephkmh josephkmh Jan 31, 2023

Choose a reason for hiding this comment

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

good point! fixed ✅ 5dcaf69

.list(workspaceId)
.then((updatedConnections) => queryClient.setQueryData(connectionsKeys.lists(), updatedConnections));
},
}
);
};

export const useResetConnection = () => {
Expand Down Expand Up @@ -239,9 +267,12 @@ export const useRemoveConnectionsFromList = (): ((connectionIds: string[]) => vo
const useConnectionList = (payload: Pick<WebBackendConnectionListRequestBody, "destinationId" | "sourceId"> = {}) => {
const workspace = useCurrentWorkspace();
const service = useWebConnectionService();
const REFETCH_CONNECTION_LIST_INTERVAL = 60_000;

return useSuspenseQuery(connectionsKeys.lists(payload.destinationId ?? payload.sourceId), () =>
service.list({ ...payload, workspaceId: workspace.workspaceId })
return useSuspenseQuery(
connectionsKeys.lists(payload.destinationId ?? payload.sourceId),
() => service.list({ ...payload, workspaceId: workspace.workspaceId }),
{ refetchInterval: REFETCH_CONNECTION_LIST_INTERVAL }
Copy link
Contributor Author

@josephkmh josephkmh Jan 27, 2023

Choose a reason for hiding this comment

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

Re-fetching the connections every 60 seconds lets us show updated progress in the connection table. That way syncs don't stay "pending" until you refresh the page. p99 latency for this endpoint is under 100ms, so I don't think it's a performance problem.

);
};

Expand Down
Loading