-
Notifications
You must be signed in to change notification settings - Fork 4.1k
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
🪟🧹 Connector builder: Fast fields performance improvements #20957
Conversation
…nector-form-types
…nector-form-types
…or-form-loading-state
…or-form-name-override
…connector-form-unifinished-flows
…ector-form-remove-ui-widget-state
….module.scss Co-authored-by: Tim Roes <[email protected]>
…or-form-loading-state
…irbytehq/airbyte into flash1293/connector-form-loading-state
…connector-form-name-override
…connector-form-unifinished-flows
…or-form-loading-state
…connector-form-name-override
…connector-form-unifinished-flows
…or-form-loading-state
…connector-form-name-override
…connector-form-unifinished-flows
…-form-loading-state
selectedTab: "configuration" | "schema"; | ||
}) => { | ||
const { formatMessage } = useIntl(); | ||
const [field, , helpers] = useField<BuilderStream[]>("streams"); |
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.
Splitting out the controls to isolate the re-render to this component instead of re-rendering the full stream config view.
@@ -60,29 +61,53 @@ interface KeyValueListFieldProps { | |||
export const KeyValueListField: React.FC<KeyValueListFieldProps> = ({ path, label, tooltip }) => { | |||
const [{ value: keyValueList }, , { setValue: setKeyValueList }] = useField<Array<[string, string]>>(path); |
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.
Can't get rid of the "useField" here, but we can memoize everything happening within the component if the key value list didn't actually change
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.
Why can't we use a FastField here?
const { formatMessage } = useIntl(); | ||
const [modalOpen, setModalOpen] = useState(false); | ||
const { builderFormValues } = useConnectorBuilderFormState(); |
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.
We can't get rid of subscribing to the builder form state, but we can memoize building the actual options list and memoize the actual listbox component based on that.
)} | ||
<Formik | ||
initialValues={initialFormValues.current} | ||
validateOnBlur={false} |
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.
Noticed that formik would still re-trigger renders if all validateOn* are explicitly disabled - this is a big saving.
export const BuilderOneOf: React.FC<BuilderOneOfProps> = (props) => { | ||
return ( | ||
<FastField name={`${props.path}.type`}> | ||
{(fastFieldProps: FastFieldProps<string>) => <InnerBuilderOneOf {...props} {...fastFieldProps} />} |
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.
Wrap the oneOf block in its own fastfield
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.
My understanding here is that it is okay to have the name
of this FastField be props.path.type
because the logic in this class only needs to run when type of the oneOf changes. Anything else changing inside the oneOf will be handled by the FastFields in the children.
Is this correct?
export const BuilderField: React.FC<BuilderFieldProps> = (props) => { | ||
return ( | ||
<FastField name={props.path}> | ||
{({ field, form, meta }: FastFieldProps<unknown>) => ( |
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.
Wrap each builder field in a fast field instead of using useField
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.
@flash1293 I pushed one change here and left a few other comments as well. I'm going to merge this as is once the build is green in order to unblock the release, so if there are any other follow-up changes you want to make based on the comments I left then we can just do that in a follow-up PR.
After pushing my fix, it seems to be working as expected from my local testing, and it seems to be nice and snappy. Nice work!
export const BuilderOneOf: React.FC<BuilderOneOfProps> = (props) => { | ||
return ( | ||
<FastField name={`${props.path}.type`}> | ||
{(fastFieldProps: FastFieldProps<string>) => <InnerBuilderOneOf {...props} {...fastFieldProps} />} |
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.
My understanding here is that it is okay to have the name
of this FastField be props.path.type
because the logic in this class only needs to run when type of the oneOf changes. Anything else changing inside the oneOf will be handled by the FastFields in the children.
Is this correct?
@@ -60,29 +61,53 @@ interface KeyValueListFieldProps { | |||
export const KeyValueListField: React.FC<KeyValueListFieldProps> = ({ path, label, tooltip }) => { | |||
const [{ value: keyValueList }, , { setValue: setKeyValueList }] = useField<Array<[string, string]>>(path); |
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.
Why can't we use a FastField here?
@@ -111,7 +111,9 @@ export const BuilderSidebar: React.FC<BuilderSidebarProps> = React.memo(({ class | |||
<FontAwesomeIcon icon={faUser} /> | |||
<FormattedMessage | |||
id="connectorBuilder.userInputs" | |||
values={{ number: values.inputs.length + getInferredInputs(values).length }} | |||
values={{ | |||
number: values.inputs.length + getInferredInputs(values.global, values.inferredInputOverrides).length, |
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.
Any reason the getInferredInputs
result is not being memoized in this component?
@@ -115,8 +115,8 @@ export const DEFAULT_BUILDER_STREAM_VALUES: Omit<BuilderStream, "id"> = { | |||
}, | |||
}; | |||
|
|||
function getInferredInputList(values: BuilderFormValues): BuilderFormInput[] { | |||
if (values.global.authenticator.type === "ApiKeyAuthenticator") { | |||
function getInferredInputList(global: BuilderFormValues["global"]): BuilderFormInput[] { |
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.
Is there a reason you chose to have this taken in the entire global
object instead of just the global.authenticator
, or even global.authenticator.type
? Since it seems like that is the only value from global
that is used in this function
@@ -119,9 +120,9 @@ export const ConnectorBuilderFormStateProvider: React.FC<React.PropsWithChildren | |||
editorView !== "ui" | |||
? jsonManifest | |||
: builderFormValues === lastValidBuilderFormValues | |||
? jsonManifest | |||
? derivedJsonManifest |
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.
FYI I pushed this change because I noticed when testing this branch that the stream URL in the testing panel was not being updated when I would change the URL path of the stream.
After some digging, I figured out that this should be using the derivedJsonManifest
in this case, because jsonManifest
just contains the local storage JSON value of the yaml editor, which means it contained outdated info.
The only reason this was working on master is because we were doing the merge()
call above (which was removed in this PR), which caused builderFormValues === lastValidBuilderFormValues
to always evaluate to false
. Now that the merge is removed, that evaluates to true when there are no errors, so it was returning the jsonManifest
value which was outdated, resulting in weird behavior.
* improve some types * improve further * clean up a bit more * refactor loading state * move loading state up * remove isLoading references * remove unused props and make fetch connector error work * remove special component for name * remove top level state for unifinished flows * start removing uiwidget * Update airbyte-webapp/src/views/Connector/ConnectorCard/ConnectorCard.module.scss Co-authored-by: Tim Roes <[email protected]> * remove undefined option for selected id * remove unused prop * fix types * remove uiwidget state * clean up * adjust comment * handle errors in a nice way * do not respect default on oneOf fields * rename to formblock * reduce re-renders * pass error to secure inputs * simplify and improve styling * align top * code review * remove comment * review comments * rename file * be strict about boolean values * add example * track form error in error boundary * review comments * handle unexpected cases better * speed up some bits * more changes * enrich error with connector id * 🪟🎉 Add copy stream button (#20577) * add copy stream button * review comments * rename prop * 🪟🎉 Connector builder: Integrate connector form for test input (#20385) * move connector builder components into the same shared components/connectorBuilder directory * move diff over from poc branch * save current progress * add modal for adding streams * focus stream after adding and reset button style * add reset confirm modal and select view on add * style global config and streams buttons * styling improvements * handle long stream names better * pull in connector manifest schema directly * add box shadows to resizable panels * upgrade orval and use connector manifest schema directly * remove airbyte protocol from connector builder api spec * generate python models from openapi change * fix position of yaml toggle * handle no stream case with better looking message * group global fields into single object and fix console error * confirmation modal on toggling dirty form + cleanup * fix connector name display * undo change to manifest schema * remove commented code * remove unnecessary change * fix spacing * use shadow mixin for connector img * add comment about connector img * change onSubmit to no-op * remove console log * clean up styling * simplify sidebar to remove StreamSelectButton component * swap colors of toggle * move FormikPatch to src/core/form * move types up to connectorBuilder/ level * use grid display for ui yaml toggle button * use spread instead of setting array index directly * add intl in missing places * pull connector manifest schema in through separate openapi spec * use correct intl string id * throttle setting json manifest in yaml editor * use button prop instead of manually styling * consolidate AddStreamButton styles * fix sidebar flex styles * use specific flex properties instead of flex * clean up download and reset button styles * use row-reverse for yaml editor download button * fix stream selector styles to remove margins * give connector setup guide panel same corner and shadow styles * remove blur from page display * set view to stream when selected in test panel * add placeholder when stream name is empty * switch to index-based stream selection to preserve testing panel selected stream on rename * handle empty name in stream selector * make connector form work in connector builder * fix small stuff * add warning label * review comments * adjust translation Co-authored-by: lmossman <[email protected]> * use request_body_json instead of request_body_data * 🪟 🎨 Move `Add` button into the line of Connector Builder key value list fields (#20699) * move add button into line * add stories for empty with control, and content + control * change button name to Control * 🪟🎉 Connector builder: Allow defining inputs (#20431) * move connector builder components into the same shared components/connectorBuilder directory * move diff over from poc branch * save current progress * add modal for adding streams * focus stream after adding and reset button style * add reset confirm modal and select view on add * style global config and streams buttons * styling improvements * handle long stream names better * pull in connector manifest schema directly * add box shadows to resizable panels * upgrade orval and use connector manifest schema directly * remove airbyte protocol from connector builder api spec * generate python models from openapi change * fix position of yaml toggle * handle no stream case with better looking message * group global fields into single object and fix console error * confirmation modal on toggling dirty form + cleanup * fix connector name display * undo change to manifest schema * remove commented code * remove unnecessary change * fix spacing * use shadow mixin for connector img * add comment about connector img * change onSubmit to no-op * remove console log * clean up styling * simplify sidebar to remove StreamSelectButton component * swap colors of toggle * move FormikPatch to src/core/form * move types up to connectorBuilder/ level * use grid display for ui yaml toggle button * use spread instead of setting array index directly * add intl in missing places * pull connector manifest schema in through separate openapi spec * use correct intl string id * throttle setting json manifest in yaml editor * use button prop instead of manually styling * consolidate AddStreamButton styles * fix sidebar flex styles * use specific flex properties instead of flex * clean up download and reset button styles * use row-reverse for yaml editor download button * fix stream selector styles to remove margins * give connector setup guide panel same corner and shadow styles * remove blur from page display * set view to stream when selected in test panel * add placeholder when stream name is empty * switch to index-based stream selection to preserve testing panel selected stream on rename * handle empty name in stream selector * make connector form work in connector builder * wip * fix small stuff * add basic input UI * user inputs * make most of inputs configuration work * fix a bunch of stuff * handle unknown config types * add warning label * fix label * fix some styling * review comments * improve state management and error handling * handle stored form values that don't contain new fields properly * Update airbyte-webapp/src/locales/en.json Co-authored-by: Lake Mossman <[email protected]> * Update airbyte-webapp/src/components/connectorBuilder/Builder/InputsView.tsx Co-authored-by: Lake Mossman <[email protected]> * inputs editing weirdness * input form reset * using the Label component * 🪟🎉 Connector builder authentication (#20645) * allow auth configuration * check for conflicts with the inferred inputs * fix invisible inputs * reduce redundancy and hide advanced input options for inferred inputs * unnecessary validation * typo * unnecessary effect hook * build spec even for invalid forms but do not update stream list * fix keys * 🪟🎉 Connector builder: Session token and oauth authentication (#20712) * session token and oauth authentication * fill in session token variable * typos * make sure validation error does not go away * 🪟🎉 Connector builder: Always validate inputs form (#20664) * validate user input outside of form * review comments Co-authored-by: lmossman <[email protected]> Co-authored-by: lmossman <[email protected]> * fix merge conflict with dropdown prop being renamed to control * [Connector Builder] Add paginator (#20698) * move connector builder components into the same shared components/connectorBuilder directory * move diff over from poc branch * save current progress * add modal for adding streams * focus stream after adding and reset button style * add reset confirm modal and select view on add * style global config and streams buttons * styling improvements * handle long stream names better * pull in connector manifest schema directly * add box shadows to resizable panels * upgrade orval and use connector manifest schema directly * remove airbyte protocol from connector builder api spec * generate python models from openapi change * fix position of yaml toggle * handle no stream case with better looking message * group global fields into single object and fix console error * confirmation modal on toggling dirty form + cleanup * fix connector name display * undo change to manifest schema * remove commented code * remove unnecessary change * fix spacing * use shadow mixin for connector img * add comment about connector img * change onSubmit to no-op * remove console log * clean up styling * simplify sidebar to remove StreamSelectButton component * swap colors of toggle * move FormikPatch to src/core/form * move types up to connectorBuilder/ level * use grid display for ui yaml toggle button * use spread instead of setting array index directly * add intl in missing places * pull connector manifest schema in through separate openapi spec * use correct intl string id * throttle setting json manifest in yaml editor * use button prop instead of manually styling * consolidate AddStreamButton styles * fix sidebar flex styles * use specific flex properties instead of flex * clean up download and reset button styles * use row-reverse for yaml editor download button * fix stream selector styles to remove margins * give connector setup guide panel same corner and shadow styles * remove blur from page display * set view to stream when selected in test panel * add placeholder when stream name is empty * switch to index-based stream selection to preserve testing panel selected stream on rename * handle empty name in stream selector * make connector form work in connector builder * wip * fix small stuff * add basic input UI * user inputs * make most of inputs configuration work * fix a bunch of stuff * handle unknown config types * add warning label * fix label * fix some styling * review comments * improve state management and error handling * allow auth configuration * check for conflicts with the inferred inputs * fix invisible inputs * handle stored form values that don't contain new fields properly * session token and oauth authentication * fill in session token variable * fix merge of default values * add primaryKey and cursorField to builder types, and consolidate default valeues to types.ts * add cursor and primary key fields to ui * save * add page size and token option inputs * fixes after rebase * add pagination * fix pagination types * handle empty field_name better * Update airbyte-webapp/src/locales/en.json Co-authored-by: Lake Mossman <[email protected]> * Update airbyte-webapp/src/components/connectorBuilder/Builder/InputsView.tsx Co-authored-by: Lake Mossman <[email protected]> * inputs editing weirdness * input form reset * using the Label component * reduce redundancy and hide advanced input options for inferred inputs * unnecessary validation * typo * unnecessary effect hook * build spec even for invalid forms but do not update stream list * typos * make sure validation error does not go away * make primary key and cursor optional, and reorder * save toggle group progress * fix style of toggle label * handle empty values better * fix page size/token option field validation and rendering * handle cursor pagination page size option correctly Co-authored-by: Joe Reuter <[email protected]> * [Connector Builder] Add stream slicer (#20748) * move connector builder components into the same shared components/connectorBuilder directory * move diff over from poc branch * save current progress * add modal for adding streams * focus stream after adding and reset button style * add reset confirm modal and select view on add * style global config and streams buttons * styling improvements * handle long stream names better * pull in connector manifest schema directly * add box shadows to resizable panels * upgrade orval and use connector manifest schema directly * remove airbyte protocol from connector builder api spec * generate python models from openapi change * fix position of yaml toggle * handle no stream case with better looking message * group global fields into single object and fix console error * confirmation modal on toggling dirty form + cleanup * fix connector name display * undo change to manifest schema * remove commented code * remove unnecessary change * fix spacing * use shadow mixin for connector img * add comment about connector img * change onSubmit to no-op * remove console log * clean up styling * simplify sidebar to remove StreamSelectButton component * swap colors of toggle * move FormikPatch to src/core/form * move types up to connectorBuilder/ level * use grid display for ui yaml toggle button * use spread instead of setting array index directly * add intl in missing places * pull connector manifest schema in through separate openapi spec * use correct intl string id * throttle setting json manifest in yaml editor * use button prop instead of manually styling * consolidate AddStreamButton styles * fix sidebar flex styles * use specific flex properties instead of flex * clean up download and reset button styles * use row-reverse for yaml editor download button * fix stream selector styles to remove margins * give connector setup guide panel same corner and shadow styles * remove blur from page display * set view to stream when selected in test panel * add placeholder when stream name is empty * switch to index-based stream selection to preserve testing panel selected stream on rename * handle empty name in stream selector * make connector form work in connector builder * wip * fix small stuff * add basic input UI * user inputs * make most of inputs configuration work * fix a bunch of stuff * handle unknown config types * add warning label * fix label * fix some styling * review comments * improve state management and error handling * allow auth configuration * check for conflicts with the inferred inputs * fix invisible inputs * handle stored form values that don't contain new fields properly * session token and oauth authentication * fill in session token variable * fix merge of default values * add primaryKey and cursorField to builder types, and consolidate default valeues to types.ts * add cursor and primary key fields to ui * save * add page size and token option inputs * fixes after rebase * add pagination * fix pagination types * handle empty field_name better * Update airbyte-webapp/src/locales/en.json Co-authored-by: Lake Mossman <[email protected]> * Update airbyte-webapp/src/components/connectorBuilder/Builder/InputsView.tsx Co-authored-by: Lake Mossman <[email protected]> * inputs editing weirdness * input form reset * using the Label component * reduce redundancy and hide advanced input options for inferred inputs * unnecessary validation * typo * unnecessary effect hook * build spec even for invalid forms but do not update stream list * typos * make sure validation error does not go away * make primary key and cursor optional, and reorder * save toggle group progress * fix style of toggle label * handle empty values better * fix page size/token option field validation and rendering * handle cursor pagination page size option correctly * save stream slicer progress * finish stream slicer * fix stream slicer fields and validation Co-authored-by: Joe Reuter <[email protected]> * debounce form builder values update to reduce load * 🪟🔧 Connector builder: use new lowcode manifest (#20715) * use new manifest yaml * Update airbyte-webapp/src/components/connectorBuilder/types.ts Co-authored-by: Lake Mossman <[email protected]> * use updated manifest types Co-authored-by: Lake Mossman <[email protected]> * debounce validation as well * akways show stream test button in error state if there are errors * fix type of oauth input * review comments * fix more * start implementing fast fields * a few more fastfields * fix some stuff * complete fast fields work * memoize a bit more * remove irrelevant change * fix types * some more improvements * prevent all out-of-line validation calls * use derivedJsonManifest when in UI view Co-authored-by: Tim Roes <[email protected]> Co-authored-by: lmossman <[email protected]>
Fixes #20947
This PR is introducing fast field and memoization in some key places to speed up the perceived performance of the UI.
I left some comments in the code to explain a bit in greater detail what the individual improvements are about. However, these are the ones I added:
Measurements below are using a complex form with slicer, paginator and multiple request params, 4x CPU slowdown and heavy instrumentation - production build is much faster:
Before when typing quickly (with the other performance PR merged already):
There are two commits per keystroke, and both take 60ms - this causes heavy frame dropping
Now:
One commit per keystroke, roughly 16ms - very little frame dropping
While I'm not super happy with this solution, I think it's good enough for the moment until we can switch away from formik which will re-render much less with idiomatic usage.