-
Notifications
You must be signed in to change notification settings - Fork 87
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
perf: conditionally use hooks based on activeness of field row #6303
Conversation
which separates the hooks relating to handling duplicate fields and deleting fields. Preventing unnecessary re-renders to non-active fields.
Changes are current on staging-alt can use this form use-template to test (300 fields) |
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.
Thanks a lot for that @foochifa !! 💪
.../src/features/admin-form/create/builder-and-design/BuilderAndDesignContent/BuilderFields.tsx
Outdated
Show resolved
Hide resolved
.../admin-form/create/builder-and-design/BuilderAndDesignContent/FieldRow/FieldRowContainer.tsx
Show resolved
Hide resolved
.../src/features/admin-form/create/builder-and-design/BuilderAndDesignContent/BuilderFields.tsx
Outdated
Show resolved
Hide resolved
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.
LGTM! The readability stuff I was mentioning are minor, we can push this first and get the perf improvements sooner rather than later.
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.
LGTM!
(assuming that having isDraggingOver:undefined
and fieldBuilderState:undefined
for the non-active fields does not cause problem down the nested elements)
* test: add stories for admin and public forms with payments (#6292) * test: add stories for admin and public forms * test: add stories for FormPaymentPage * fix: misc frontend updates * fix: UI fixes for payment preview block in form builder and public form * chore(deps-dev): bump @babel/plugin-transform-runtime from 7.19.6 to 7.21.4 (#6297) chore(deps-dev): bump @babel/plugin-transform-runtime Bumps [@babel/plugin-transform-runtime](https://github.com/babel/babel/tree/HEAD/packages/babel-plugin-transform-runtime) from 7.19.6 to 7.21.4. - [Release notes](https://github.com/babel/babel/releases) - [Changelog](https://github.com/babel/babel/blob/main/CHANGELOG.md) - [Commits](https://github.com/babel/babel/commits/v7.21.4/packages/babel-plugin-transform-runtime) --- updated-dependencies: - dependency-name: "@babel/plugin-transform-runtime" dependency-type: direct:development update-type: version-update:semver-minor ... Signed-off-by: dependabot[bot] <[email protected]> Co-authored-by: dependabot[bot] <49699333+dependabot[bot]@users.noreply.github.com> * chore(deps-dev): bump @typescript-eslint/eslint-plugin from 5.59.2 to 5.59.5 in /shared (#6288) chore(deps-dev): bump @typescript-eslint/eslint-plugin in /shared Bumps [@typescript-eslint/eslint-plugin](https://github.com/typescript-eslint/typescript-eslint/tree/HEAD/packages/eslint-plugin) from 5.59.2 to 5.59.5. - [Release notes](https://github.com/typescript-eslint/typescript-eslint/releases) - [Changelog](https://github.com/typescript-eslint/typescript-eslint/blob/main/packages/eslint-plugin/CHANGELOG.md) - [Commits](https://github.com/typescript-eslint/typescript-eslint/commits/v5.59.5/packages/eslint-plugin) --- updated-dependencies: - dependency-name: "@typescript-eslint/eslint-plugin" dependency-type: direct:development update-type: version-update:semver-patch ... Signed-off-by: dependabot[bot] <[email protected]> Co-authored-by: dependabot[bot] <49699333+dependabot[bot]@users.noreply.github.com> * build(deps): bump json5 from 1.0.1 to 1.0.2 in /frontend (#5688) Bumps [json5](https://github.com/json5/json5) from 1.0.1 to 1.0.2. - [Release notes](https://github.com/json5/json5/releases) - [Changelog](https://github.com/json5/json5/blob/main/CHANGELOG.md) - [Commits](json5/json5@v1.0.1...v1.0.2) --- updated-dependencies: - dependency-name: json5 dependency-type: indirect ... Signed-off-by: dependabot[bot] <[email protected]> Co-authored-by: dependabot[bot] <49699333+dependabot[bot]@users.noreply.github.com> * build(deps): bump http-cache-semantics from 4.1.0 to 4.1.1 in /frontend (#5729) Bumps [http-cache-semantics](https://github.com/kornelski/http-cache-semantics) from 4.1.0 to 4.1.1. - [Release notes](https://github.com/kornelski/http-cache-semantics/releases) - [Commits](kornelski/http-cache-semantics@v4.1.0...v4.1.1) --- updated-dependencies: - dependency-name: http-cache-semantics dependency-type: indirect ... Signed-off-by: dependabot[bot] <[email protected]> Co-authored-by: dependabot[bot] <49699333+dependabot[bot]@users.noreply.github.com> * chore(deps-dev): bump type-fest from 2.19.0 to 3.10.0 (#6270) Bumps [type-fest](https://github.com/sindresorhus/type-fest) from 2.19.0 to 3.10.0. - [Release notes](https://github.com/sindresorhus/type-fest/releases) - [Commits](sindresorhus/type-fest@v2.19.0...v3.10.0) --- updated-dependencies: - dependency-name: type-fest dependency-type: direct:development update-type: version-update:semver-major ... Signed-off-by: dependabot[bot] <[email protected]> Co-authored-by: dependabot[bot] <49699333+dependabot[bot]@users.noreply.github.com> * chore(deps-dev): bump jest-extended from 1.2.1 to 3.2.4 (#5793) Bumps [jest-extended](https://github.com/jest-community/jest-extended) from 1.2.1 to 3.2.4. - [Release notes](https://github.com/jest-community/jest-extended/releases) - [Changelog](https://github.com/jest-community/jest-extended/blob/main/CHANGELOG.md) - [Commits](jest-community/jest-extended@v1.2.1...v3.2.4) --- updated-dependencies: - dependency-name: jest-extended dependency-type: direct:development update-type: version-update:semver-major ... Signed-off-by: dependabot[bot] <[email protected]> Co-authored-by: dependabot[bot] <49699333+dependabot[bot]@users.noreply.github.com> * fix: squished logic block either option (#6305) * Add minimum width 0 for flex child items that do not respect flex basis settings * refactor: logicValueWrapperWidth to be more concise --------- Co-authored-by: Dexter Tan <[email protected]> * feat: add local dev support for twilio (#5881) * feat: add prism for twilio * feat: add localdev env config * fix: mocktwilio randomly assigning errorCode in response * feat: route sms body to maildev * fix: incorrect refernce to determine localdev * refactor: rename localDev to useMockTwilio * feat: generalize sendLocalDevMail * fix: test cases not mocking new mocktwilio * chore(deps-dev): bump @types/express from 4.17.14 to 4.17.17 (#6311) Bumps [@types/express](https://github.com/DefinitelyTyped/DefinitelyTyped/tree/HEAD/types/express) from 4.17.14 to 4.17.17. - [Release notes](https://github.com/DefinitelyTyped/DefinitelyTyped/releases) - [Commits](https://github.com/DefinitelyTyped/DefinitelyTyped/commits/HEAD/types/express) --- updated-dependencies: - dependency-name: "@types/express" dependency-type: direct:development update-type: version-update:semver-patch ... Signed-off-by: dependabot[bot] <[email protected]> Co-authored-by: dependabot[bot] <49699333+dependabot[bot]@users.noreply.github.com> * fix(deps): bump @sentry/browser from 7.28.1 to 7.51.2 (#6314) Bumps [@sentry/browser](https://github.com/getsentry/sentry-javascript) from 7.28.1 to 7.51.2. - [Release notes](https://github.com/getsentry/sentry-javascript/releases) - [Changelog](https://github.com/getsentry/sentry-javascript/blob/develop/CHANGELOG.md) - [Commits](getsentry/sentry-javascript@7.28.1...7.51.2) --- updated-dependencies: - dependency-name: "@sentry/browser" dependency-type: direct:production update-type: version-update:semver-minor ... Signed-off-by: dependabot[bot] <[email protected]> Co-authored-by: dependabot[bot] <49699333+dependabot[bot]@users.noreply.github.com> * chore(deps-dev): bump @types/opossum from 6.2.2 to 6.2.3 (#6316) Bumps [@types/opossum](https://github.com/DefinitelyTyped/DefinitelyTyped/tree/HEAD/types/opossum) from 6.2.2 to 6.2.3. - [Release notes](https://github.com/DefinitelyTyped/DefinitelyTyped/releases) - [Commits](https://github.com/DefinitelyTyped/DefinitelyTyped/commits/HEAD/types/opossum) --- updated-dependencies: - dependency-name: "@types/opossum" dependency-type: direct:development update-type: version-update:semver-patch ... Signed-off-by: dependabot[bot] <[email protected]> Co-authored-by: dependabot[bot] <49699333+dependabot[bot]@users.noreply.github.com> * chore(deps-dev): bump husky from 8.0.2 to 8.0.3 (#6312) Bumps [husky](https://github.com/typicode/husky) from 8.0.2 to 8.0.3. - [Release notes](https://github.com/typicode/husky/releases) - [Commits](typicode/husky@v8.0.2...v8.0.3) --- updated-dependencies: - dependency-name: husky dependency-type: direct:development update-type: version-update:semver-patch ... Signed-off-by: dependabot[bot] <[email protected]> Co-authored-by: dependabot[bot] <49699333+dependabot[bot]@users.noreply.github.com> * chore(deps-dev): bump axios-mock-adapter from 1.21.2 to 1.21.4 (#6313) Bumps [axios-mock-adapter](https://github.com/ctimmerm/axios-mock-adapter) from 1.21.2 to 1.21.4. - [Release notes](https://github.com/ctimmerm/axios-mock-adapter/releases) - [Changelog](https://github.com/ctimmerm/axios-mock-adapter/blob/master/CHANGELOG.md) - [Commits](ctimmerm/axios-mock-adapter@v1.21.2...v1.21.4) --- updated-dependencies: - dependency-name: axios-mock-adapter dependency-type: direct:development update-type: version-update:semver-patch ... Signed-off-by: dependabot[bot] <[email protected]> Co-authored-by: dependabot[bot] <49699333+dependabot[bot]@users.noreply.github.com> * feat: prevent prefill editing (#6097) * chore: add prefill property to model * feat: admin can lock prefill * chore: render locked prefill on public form * chore: update copy * feat: styling for locked prefill field * fix: model validation * chore: add tests * chore: add stories * fix: prefill can be locked only when there is a prefill value * chore: update copy * chore: update copy * fix: styling for dropdown input * chore: copy update * chore: unwanted import * feat: show different message based on combination of prefills * feat: update stories * chore: update copy for locked fields * perf: conditionally use hooks based on activeness of field row (#6303) * perf: refactor active field button group which separates the hooks relating to handling duplicate fields and deleting fields. Preventing unnecessary re-renders to non-active fields. * perf: update parent builder field * perf: update dragging over flag only for active fields * refactor: available size limit should only be computed on attachment * chore: remove unnecessary check * refactor: improve name of activeField * refactor: improve readability for dragging over * refactor: only pass extra props when active * fix: playwright tests cancelled after timeout (and other issues) (#6308) * fix: add ssm env site name for playwright * fix: email sort by time in ascending order * ci: separate playwright test suites into diff steps * fix: change textbox to combobox for field settings dropdown * fix: change in fillMultiDropdowns behaviour - Don't close dropdown between item selections. - Scroll options into view if they're not in view. * ci: temporarily comment out responses download * ci: temp rm of webkit from playwright - due to secret key download issues only on webkit * fix: type change due to upgrade in types/express to 4.17.17 (#6319) fix: type change due to upgrade in types/express * feat: retain image metadata after compression (#6320) * chore: bump browser-image-compression to 2.0.2 * feat: preserve exif when image is compressed * chore(deps-dev): bump @types/jsonfile from 6.1.0 to 6.1.1 (#6329) Bumps [@types/jsonfile](https://github.com/DefinitelyTyped/DefinitelyTyped/tree/HEAD/types/jsonfile) from 6.1.0 to 6.1.1. - [Release notes](https://github.com/DefinitelyTyped/DefinitelyTyped/releases) - [Commits](https://github.com/DefinitelyTyped/DefinitelyTyped/commits/HEAD/types/jsonfile) --- updated-dependencies: - dependency-name: "@types/jsonfile" dependency-type: direct:development update-type: version-update:semver-patch ... Signed-off-by: dependabot[bot] <[email protected]> Co-authored-by: dependabot[bot] <49699333+dependabot[bot]@users.noreply.github.com> * chore(deps-dev): bump @typescript-eslint/parser from 5.59.5 to 5.59.6 in /shared (#6331) chore(deps-dev): bump @typescript-eslint/parser in /shared Bumps [@typescript-eslint/parser](https://github.com/typescript-eslint/typescript-eslint/tree/HEAD/packages/parser) from 5.59.5 to 5.59.6. - [Release notes](https://github.com/typescript-eslint/typescript-eslint/releases) - [Changelog](https://github.com/typescript-eslint/typescript-eslint/blob/main/packages/parser/CHANGELOG.md) - [Commits](https://github.com/typescript-eslint/typescript-eslint/commits/v5.59.6/packages/parser) --- updated-dependencies: - dependency-name: "@typescript-eslint/parser" dependency-type: direct:development update-type: version-update:semver-patch ... Signed-off-by: dependabot[bot] <[email protected]> Co-authored-by: dependabot[bot] <49699333+dependabot[bot]@users.noreply.github.com> * chore(deps-dev): bump @typescript-eslint/eslint-plugin from 5.59.5 to 5.59.6 in /shared (#6332) chore(deps-dev): bump @typescript-eslint/eslint-plugin in /shared Bumps [@typescript-eslint/eslint-plugin](https://github.com/typescript-eslint/typescript-eslint/tree/HEAD/packages/eslint-plugin) from 5.59.5 to 5.59.6. - [Release notes](https://github.com/typescript-eslint/typescript-eslint/releases) - [Changelog](https://github.com/typescript-eslint/typescript-eslint/blob/main/packages/eslint-plugin/CHANGELOG.md) - [Commits](https://github.com/typescript-eslint/typescript-eslint/commits/v5.59.6/packages/eslint-plugin) --- updated-dependencies: - dependency-name: "@typescript-eslint/eslint-plugin" dependency-type: direct:development update-type: version-update:semver-patch ... Signed-off-by: dependabot[bot] <[email protected]> Co-authored-by: dependabot[bot] <49699333+dependabot[bot]@users.noreply.github.com> * chore: add code of conduct from contributor covenant (#6326) * fix: pin MockPass version (#6324) * fix(deps): bump twilio from 4.7.2 to 4.11.0 (#6328) Bumps [twilio](https://github.com/twilio/twilio-node) from 4.7.2 to 4.11.0. - [Release notes](https://github.com/twilio/twilio-node/releases) - [Changelog](https://github.com/twilio/twilio-node/blob/main/CHANGES.md) - [Commits](twilio/twilio-node@4.7.2...4.11.0) --- updated-dependencies: - dependency-name: twilio dependency-type: direct:production update-type: version-update:semver-minor ... Signed-off-by: dependabot[bot] <[email protected]> Co-authored-by: dependabot[bot] <49699333+dependabot[bot]@users.noreply.github.com> * fix(deps): bump vm2 from 3.9.17 to 3.9.18 (#6333) Bumps [vm2](https://github.com/patriksimek/vm2) from 3.9.17 to 3.9.18. - [Release notes](https://github.com/patriksimek/vm2/releases) - [Changelog](https://github.com/patriksimek/vm2/blob/master/CHANGELOG.md) - [Commits](patriksimek/vm2@3.9.17...3.9.18) --- updated-dependencies: - dependency-name: vm2 dependency-type: indirect ... Signed-off-by: dependabot[bot] <[email protected]> Co-authored-by: dependabot[bot] <49699333+dependabot[bot]@users.noreply.github.com> * feat: required copy updates for payment launch on prod (#6327) * feat: update banner copy on payment forms preview * feat: add test card banner for non-production envs * feat: update payments settings page with guide * chore: remove unnecessary Box import * fix: remove external link icon for payment guide * refactor: extract textProps in preview banner * fix: put px back into payments input panel infobox * refactor: extract uat link to constant file * fix: preview banner env condition * fix: disable payment resume and duplicate modals in preview (#6334) * feat: add isPreview context to PublicFormContext * refactor: change isPreview props to useContext * fix: disable payment resume modal in preview * fix: disable payment duplicate modal in preview * refactor: made isPreview essential * fix: do not show any payment modal in preview * fix(deps): bump stripe from 11.1.0 to 12.5.0 (#6330) Bumps [stripe](https://github.com/stripe/stripe-node) from 11.1.0 to 12.5.0. - [Release notes](https://github.com/stripe/stripe-node/releases) - [Changelog](https://github.com/stripe/stripe-node/blob/master/CHANGELOG.md) - [Commits](stripe/stripe-node@v11.1.0...v12.5.0) --- updated-dependencies: - dependency-name: stripe dependency-type: direct:production update-type: version-update:semver-major ... Signed-off-by: dependabot[bot] <[email protected]> Co-authored-by: dependabot[bot] <49699333+dependabot[bot]@users.noreply.github.com> * chore: remove support for legacy stripe metadata (#6264) chore: remove support for legacy metadata * fix: rm white box for payment preview when disabled (#6342) * fix: rm white box for payment preview when disabled * fix: paymentPreview must define paymentDetails * chore: remove conditional frontend routing (#6190) * chore: remove react migration vars * chore: remove more conditional code in angular, backend types * chore: remove angular from express routing rules! * chore: add back public route * chore: remove angular public route * chore: add react router * chore: delete home folder, rename frontend to frontend-old and frontend-new to frontend * chore: remove src_e2e ci test, rename tests to better reflect backend-frontend split * test: add --openssl-legacy-provider to the ci build step * chore: update node options for build step * chore: revert ci to node 14 * fix(deps): bump @aws-sdk/client-cloudwatch-logs from 3.276.0 to 3.332.0 (#6345) Bumps [@aws-sdk/client-cloudwatch-logs](https://github.com/aws/aws-sdk-js-v3/tree/HEAD/clients/client-cloudwatch-logs) from 3.276.0 to 3.332.0. - [Release notes](https://github.com/aws/aws-sdk-js-v3/releases) - [Changelog](https://github.com/aws/aws-sdk-js-v3/blob/main/clients/client-cloudwatch-logs/CHANGELOG.md) - [Commits](https://github.com/aws/aws-sdk-js-v3/commits/v3.332.0/clients/client-cloudwatch-logs) --- updated-dependencies: - dependency-name: "@aws-sdk/client-cloudwatch-logs" dependency-type: direct:production update-type: version-update:semver-minor ... Signed-off-by: dependabot[bot] <[email protected]> Co-authored-by: dependabot[bot] <49699333+dependabot[bot]@users.noreply.github.com> * feat: add sample-submission endpoint to retrieve sample submission data of a public form (#6325) * feat: add createSampleSubmissionData function * feat: add endpoint to get sample submission * docs: add comments for GET sample submission endpoint * feat: randomise response, add attachment type * fix: remove form deactivation check * tests: add unit tests for sample submission endpoint * ref: split sample submission creation into smaller functions * fix: return static fields in createSingleSampleSubmissionAnswer * chore: bump version to v6.50.0 --------- Signed-off-by: dependabot[bot] <[email protected]> Co-authored-by: Justyn Oh <[email protected]> Co-authored-by: Lin Huiqing <[email protected]> Co-authored-by: dependabot[bot] <49699333+dependabot[bot]@users.noreply.github.com> Co-authored-by: Dexter Tan <[email protected]> Co-authored-by: Ken Lee Shu Ming <[email protected]> Co-authored-by: tshuli <[email protected]> Co-authored-by: Foo Chi Fa <[email protected]> Co-authored-by: Timothee Groleau <[email protected]> Co-authored-by: Ken Jin <[email protected]>
Problem
Despite #6189 improving performance of dragging and updating field rows slightly, users can still face significant lags when attempting to:
This is because of existing hooks still causing re-renders of all field rows from various user actions.
Closes #6045
Solution
Currently, all of the field rows are too 'smart' and knows a lot of unnecessary global states/contexts, such as field builder states, entire form data (just to compute attachment size limits), and mutation methods to duplicate/delete fields.
Firstly, we identify properties that all field rows must know. These properties are mostly used in the logic for
handleFieldClick
inFieldRowContainer
.There are specifically two properties that all fields need to keep track of:
handleBuilderClick
: As the builder must be displayed upon click, thus, non-active fields must also have the correct/updated method to display the BuilderisDirty
: This is to ensure if user click to another field, upon any existing unsaved changes, the dirty modal will still be registered and displayed.Except for the two above, all the other properties can be conditionally called based on
isActive
Steps done in this PR to abstract unnecessary properties out of
FieldRowContainer
Collapse
-isble button group containing Edit (in mobile), Duplicate, and Delete, always exists for ALL field rows even if they are not displayed. This button group will only be displayed upon a field being selected (active). Hence, will conditionally render this button group based on whether the field row is activeuseDuplicateFieldMutation
) are being used forhandleDuplicateClick
andhandleDeleteClick
, we can abstract out the entire button group as its own component. Then move these hooks into that specific component. As this component will ONLY be rendered for one field (the active field), all re-renders from these hooks will be isolated to thisFieldRowButtonGroup
component for only oneFieldRowContainer
.fieldBuilderState
of theFieldRowContainer
is primarily used when the field is active, have made it into an optional prop. This will prevent unnecessary re-renders from the updating offieldBuilderState
, hence making most of the fields more 'stupid'.isDraggingOver
to all field rows, however, from testing it seems that it only impacts the active field. Hence, have made it tied to activeness of the field. Allowing for faster start and drop of dragging action, as only the field being activated will be re-renderedOther optimizations:
isDirty
hook toBuilderFields
, for better debugging of prop changes. Now all major re-renders ofFieldRowContainers
should be seen from the change in props at the React profiler. Allowing for easier debugging of performance issues in the futurecolorTheme
hook toBuilderFields
, as the underlying logic usesuseCreateTabForm
that causes re-renders upon any form definition changes (such as saving a field), despite the value remaining static without any design tab changesRemaining re-renders that cannot be trivially solved:
On why I did not conditionally render the drag icon:
Fade
keeps the margins of the hidden component.There is likely no more trivial improvements we can make to field rows performance after this PR
Breaking Changes
Before & After Screenshots
Video capture performing a simple process of:
BEFORE:
takes 1min
Screen.Recording.2023-05-15.at.11.49.22.AM.mov
AFTER:
takes 35seconds
Screen.Recording.2023-05-15.at.11.58.25.AM.mov
Tests
To ensure no regression issues
All the behaviours below should work as intended
On mobile