-
Notifications
You must be signed in to change notification settings - Fork 4.4k
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
🪟 Per-Stream state new flow #14634
🪟 Per-Stream state new flow #14634
Changes from all commits
78c9d6b
efa5c98
6ab269c
654d502
b89cc19
2657d88
6fce4f0
5a9cbcc
13be9e8
7d1fbeb
9266bb5
ee1c329
871df8a
f0515e8
8de44f9
e08d0e5
6fd68b1
9e7fd01
22036c0
4787a78
a0cb0ce
8cbe3fe
7cadda6
f736e2e
4984735
b92807b
1f0b868
5e759ca
95a90b6
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -2,6 +2,6 @@ | |
|
||
.modalBody { | ||
padding: variables.$spacing-lg variables.$spacing-xl; | ||
overflow: scroll; | ||
overflow: auto; | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. ℹ️ @edmundito I've changed this to There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Whoops, yes I am familiar with this. it definitely should be auto! |
||
max-width: 100%; | ||
} |
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -4,7 +4,7 @@ import { | |
webBackendCreateConnection, | ||
webBackendGetConnection, | ||
webBackendListConnectionsForWorkspace, | ||
webBackendUpdateConnection, | ||
webBackendUpdateConnectionNew as webBackendUpdateConnection, | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Yes it is; this endpoint was created so that we could merge backend changes related to this new flow in before the frontend changes were merged. We have a ticket here to remove this temporary endpoint after the launch of per-stream: #14733 |
||
} from "../../request/AirbyteClient"; | ||
import { AirbyteRequestService } from "../../request/AirbyteRequestService"; | ||
|
||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -19,6 +19,7 @@ export const buildConnectionUpdate = ( | |
connection: WebBackendConnectionRead, | ||
connectionUpdate: Partial<WebBackendConnectionUpdate> | ||
): WebBackendConnectionUpdate => ({ | ||
skipReset: true, | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I'm guessing that skipReset is default to There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. That's correct, it defaults to |
||
...toWebBackendConnectionUpdate(connection), | ||
...connectionUpdate, | ||
}); |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,87 @@ | ||
import { render, waitFor } from "@testing-library/react"; | ||
import userEvent from "@testing-library/user-event"; | ||
import { useEffectOnce } from "react-use"; | ||
|
||
import { ModalServiceProvider, useModalService } from "./ModalService"; | ||
import { ModalResult } from "./types"; | ||
|
||
const TestComponent: React.FC<{ onModalResult?: (result: ModalResult<unknown>) => void }> = ({ onModalResult }) => { | ||
const { openModal } = useModalService(); | ||
useEffectOnce(() => { | ||
openModal({ | ||
title: "Test Modal Title", | ||
content: ({ onCancel, onClose }) => ( | ||
<div data-testid="testModalContent"> | ||
<button onClick={onCancel} data-testid="cancel"> | ||
Cancel | ||
</button> | ||
<button onClick={() => onClose("reason1")} data-testid="close-reason1"> | ||
Close Reason 1 | ||
</button> | ||
<button onClick={() => onClose("reason2")} data-testid="close-reason2"> | ||
Close Reason 2 | ||
</button> | ||
</div> | ||
), | ||
}).then(onModalResult); | ||
}); | ||
return null; | ||
}; | ||
|
||
const renderModal = (resultCallback?: (reason: unknown) => void) => { | ||
return render( | ||
<ModalServiceProvider> | ||
<TestComponent onModalResult={resultCallback} /> | ||
</ModalServiceProvider> | ||
); | ||
}; | ||
|
||
describe("ModalService", () => { | ||
it("should open a modal on openModal", () => { | ||
const rendered = renderModal(); | ||
|
||
expect(rendered.getByText("Test Modal Title")).toBeTruthy(); | ||
expect(rendered.getByTestId("testModalContent")).toBeTruthy(); | ||
}); | ||
|
||
it("should close the modal with escape and emit a cancel result", async () => { | ||
const resultCallback = jest.fn(); | ||
|
||
const rendered = renderModal(resultCallback); | ||
|
||
await waitFor(() => userEvent.keyboard("{Escape}")); | ||
|
||
expect(rendered.queryByTestId("testModalContent")).toBeFalsy(); | ||
expect(resultCallback).toHaveBeenCalledWith({ type: "canceled" }); | ||
}); | ||
|
||
it("should allow cancelling the modal from inside", async () => { | ||
const resultCallback = jest.fn(); | ||
|
||
const rendered = renderModal(resultCallback); | ||
|
||
await waitFor(() => userEvent.click(rendered.getByTestId("cancel"))); | ||
|
||
expect(rendered.queryByTestId("testModalContent")).toBeFalsy(); | ||
expect(resultCallback).toHaveBeenCalledWith({ type: "canceled" }); | ||
}); | ||
|
||
it("should allow closing the button with a reason and return that reason", async () => { | ||
const resultCallback = jest.fn(); | ||
|
||
let rendered = renderModal(resultCallback); | ||
|
||
await waitFor(() => userEvent.click(rendered.getByTestId("close-reason1"))); | ||
|
||
expect(rendered.queryByTestId("testModalContent")).toBeFalsy(); | ||
expect(resultCallback).toHaveBeenCalledWith({ type: "closed", reason: "reason1" }); | ||
|
||
resultCallback.mockReset(); | ||
rendered = renderModal(resultCallback); | ||
|
||
await waitFor(() => userEvent.click(rendered.getByTestId("close-reason2"))); | ||
|
||
expect(rendered.queryByTestId("testModalContent")).toBeFalsy(); | ||
expect(resultCallback).toHaveBeenCalledWith({ type: "closed", reason: "reason2" }); | ||
}); | ||
}); |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,64 @@ | ||
import React, { useContext, useMemo, useRef, useState } from "react"; | ||
import { firstValueFrom, Subject } from "rxjs"; | ||
|
||
import { Modal } from "components"; | ||
|
||
import { ModalOptions, ModalResult, ModalServiceContext } from "./types"; | ||
|
||
const modalServiceContext = React.createContext<ModalServiceContext | undefined>(undefined); | ||
|
||
export const ModalServiceProvider: React.FC = ({ children }) => { | ||
// The any here is due to the fact, that every call to open a modal might come in with | ||
// a different type, thus we can't type this with unknown or a generic. | ||
// The consuming code of this service though is properly typed, so that this `any` stays | ||
// encapsulated within this component. | ||
// eslint-disable-next-line @typescript-eslint/no-explicit-any | ||
const [modalOptions, setModalOptions] = useState<ModalOptions<any>>(); | ||
// eslint-disable-next-line @typescript-eslint/no-explicit-any | ||
const resultSubjectRef = useRef<Subject<ModalResult<any>>>(); | ||
|
||
const service: ModalServiceContext = useMemo( | ||
() => ({ | ||
openModal: (options) => { | ||
resultSubjectRef.current = new Subject(); | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. ℹ️ Why solving this with an observable here, despite only needing a one time resolved value. The problem is, that we can't control a |
||
setModalOptions(options); | ||
|
||
return firstValueFrom(resultSubjectRef.current).then((reason) => { | ||
setModalOptions(undefined); | ||
resultSubjectRef.current = undefined; | ||
return reason; | ||
}); | ||
}, | ||
closeModal: () => { | ||
resultSubjectRef.current?.next({ type: "canceled" }); | ||
}, | ||
}), | ||
[] | ||
); | ||
|
||
return ( | ||
<modalServiceContext.Provider value={service}> | ||
{children} | ||
{modalOptions && ( | ||
<Modal | ||
title={modalOptions.title} | ||
size={modalOptions.size} | ||
onClose={() => resultSubjectRef.current?.next({ type: "canceled" })} | ||
> | ||
<modalOptions.content | ||
onCancel={() => resultSubjectRef.current?.next({ type: "canceled" })} | ||
onClose={(reason) => resultSubjectRef.current?.next({ type: "closed", reason })} | ||
/> | ||
</Modal> | ||
)} | ||
</modalServiceContext.Provider> | ||
); | ||
}; | ||
|
||
export const useModalService = () => { | ||
const context = useContext(modalServiceContext); | ||
if (!context) { | ||
throw new Error("Can't use ModalService outside ModalServiceProvider"); | ||
} | ||
return context; | ||
}; |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1 @@ | ||
export * from "./ModalService"; |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,21 @@ | ||
import React from "react"; | ||
|
||
import { ModalProps } from "components/Modal/Modal"; | ||
|
||
export interface ModalOptions<T> { | ||
title: ModalProps["title"]; | ||
size?: ModalProps["size"]; | ||
content: React.ComponentType<ModalContentProps<T>>; | ||
} | ||
|
||
export type ModalResult<T> = { type: "canceled" } | { type: "closed"; reason: T }; | ||
|
||
interface ModalContentProps<T> { | ||
onClose: (reason: T) => void; | ||
onCancel: () => void; | ||
} | ||
|
||
export interface ModalServiceContext { | ||
openModal: <ResultType>(options: ModalOptions<ResultType>) => Promise<ModalResult<ResultType>>; | ||
closeModal: () => void; | ||
} |
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -63,7 +63,7 @@ function useWebConnectionService() { | |
); | ||
} | ||
|
||
function useConnectionService() { | ||
export function useConnectionService() { | ||
const config = useConfig(); | ||
const middlewares = useDefaultRequestMiddlewares(); | ||
return useInitService(() => new ConnectionService(config.apiUrl, middlewares), [config.apiUrl, middlewares]); | ||
|
@@ -190,20 +190,11 @@ const useUpdateConnection = () => { | |
const service = useWebConnectionService(); | ||
const queryClient = useQueryClient(); | ||
|
||
return useMutation( | ||
(connectionUpdate: WebBackendConnectionUpdate) => { | ||
const withRefreshedCatalogCleaned = connectionUpdate.withRefreshedCatalog | ||
? { withRefreshedCatalog: connectionUpdate.withRefreshedCatalog } | ||
: null; | ||
Comment on lines
-195
to
-197
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Why do you think this was here in the first place? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Not really. This was in there since 2 years, so I fear that knowledge is lost. Looking at the old code it seems this was a separate parameter to this hook earlier, and thus spreaded like this into it (trying to preventing actually setting the value when it was undefined), but it's really just guessing. |
||
|
||
return service.update({ ...connectionUpdate, ...withRefreshedCatalogCleaned }); | ||
return useMutation((connectionUpdate: WebBackendConnectionUpdate) => service.update(connectionUpdate), { | ||
onSuccess: (connection) => { | ||
queryClient.setQueryData(connectionsKeys.detail(connection.connectionId), connection); | ||
}, | ||
{ | ||
onSuccess: (connection) => { | ||
queryClient.setQueryData(connectionsKeys.detail(connection.connectionId), connection); | ||
}, | ||
} | ||
); | ||
}); | ||
}; | ||
|
||
const useConnectionList = (): ListConnection => { | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
ℹ️ Changed this to use
key
instead of the deprecatedkeyCode
, so I could test it via@testing-library/user-events
.