[Connector Builder] Display server errors and handle empty streams list #19461
Conversation
octavia-squidington-iv
added
area/platform
| selectedStream: StreamsListReadStreamsItem; | ||
| } | ||
|
|
||
| export const StreamTester: React.FC<StreamTesterProps> = ({ selectedStream }) => { |
There was a problem hiding this comment.
Most of the code here is moved over from StreamTestingPanel and TestControls, so this isn't totally net-new. But it does contain some changes, mainly around handling errors from the useReadStream hook.
This component was created because I needed a single component containing the URL display, test button, and result display, so that the entire thing could be conditionally rendered only if there are streams available. If there are no streams detected, this entire component is not rendered, as desired.
I'm open to naming suggestions on this component, since it feels too close to StreamTestingPanel at the moment.
| setConfigString: (configString: string) => void; | ||
| } | ||
|
|
||
| export const ConnectorBuilderStateContext = React.createContext<Context | null>(null); | ||
|
|
||
| const useJsonManifest = () => { | ||
| export const ConnectorBuilderStateProvider: React.FC<React.PropsWithChildren<unknown>> = ({ children }) => { |
There was a problem hiding this comment.
Most of the changes here are just rearranging some code. I realized that the way I had this class set up before (with each of the various states being managed by separate hooks) had the fundamental issue that different users of this context would end up with different jsonManifest states.
Since the intention here was to have all of this context's state shared across all users of it, I moved that state code into this function directly, which seems to have fixed the problem.
| @@ -103,3 +86,35 @@ export const useConnectorBuilderState = (): Context => { | |||
|
|
|||
| return connectorBuilderState; | |||
| }; | |||
|
|
|||
| export const useSelectedPageAndSlice = () => { | |||
There was a problem hiding this comment.
This is a new hook separated from the rest, because it is only used in one component (ResultDisplay) to manage the selected page and slice per stream, and that component is only rendered if there is a selected stream.
| .errorNum.errorNum { | ||
| color: colors.$white; | ||
| } |
There was a problem hiding this comment.
As discussed offline: Contrast ratio is too low that way.
| <div className={styles.noStreamsContainer}> | ||
| <FontAwesomeIcon icon={faWarning} className={styles.noStreamsIcon} size="lg" /> | ||
| <Text className={styles.noStreamsText} size="lg"> | ||
| No streams detected |
airbyte-webapp/src/services/connectorBuilder/ConnectorBuilderStateService.tsx
Show resolved
Hide resolved
lmossman
force-pushed
the
lmossman/connector-builder-error-handling
branch
from
0086571 to
a7b0b60
Compare
|
|
octavia-squidington-iv
added
area/api
|
Sorry for the noise on this PR. I forgot to change the base branch to master on Github before I rebased it on master locally and force-pushed the changes, so it considered the entire repo as "new changes" for a little bit, causing all of our code owners to be requested 😓 |
|
Everyone should feel free to just ignore this PR if you got pinged |
lmossman
force-pushed
the
lmossman/connector-builder-error-handling
branch
from
a7b0b60 to
55c6051
Compare
…st (#19461) * save error handling progress * display error from stream read * rearrange components in stream testing panel * fix state/requests/error handling and properly handle empty streams list * remove commented code, unused components, and console logs * use unknown error in the case of empty error message * don't override numberbadge style * move 'No streams detected' into en.json * handle list streams error better * add loading states for yaml editor and side panel * add state service changes to support loading states * fix manifest template * fix promise * fix api override * undo change to apiOverride
What
This PR makes changes to the connector builder to properly handle and display errors from the connector builder server when testing.
It also does some refactoring so that the case where no streams are detected in the manifest is correctly handled, e.g. by displaying a warning like this:
This required moving some code into different components and conditionally rendering them only if a stream is currently selected (which can now only be true if there are streams to select).
Here is a screenshot of what an error looks like when testing the stream:
How
I left some comments in the code indicating why the main changes were made