-
Notifications
You must be signed in to change notification settings - Fork 834
Forms: Improve dashboard initial loading performance #45294
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
base: trunk
Are you sure you want to change the base?
Conversation
Endpoints for feedback config and integrations now use the '_fields=sync' query to minimize heavy operations during initial load. Conditional logic was added to avoid expensive checks and data fetching for integrations and AI features unless required, improving dashboard and block performance.
Are you an Automattician? Please test your changes on all WordPress.com environments to help mitigate accidental explosions.
Interested in more tips and information?
|
Thank you for your PR! When contributing to Jetpack, we have a few suggestions that can help us test and review your patch:
This comment will be updated as you work on your PR and make changes. If you think that some of those checks are not needed for your PR, please explain why you think so. Thanks for cooperation 🤖 Follow this PR Review Process:
If you have questions about anything, reach out in #jetpack-developers for guidance! |
Code Coverage SummaryCannot generate coverage summary while tests are failing. 🤐 Please fix the tests, or re-run the Code coverage job if it was something being flaky. |
…efit of the preloading" This reverts commit ed0d56f.
dc5b5c0
to
7d3c417
Compare
// Determine if Google Drive setup is complete. | ||
$jetpack_connected = ( new Host() )->is_wpcom_simple() || ( new Connection_Manager( 'jetpack-forms' ) )->is_user_connected( $user_id ); | ||
$status['isConnected'] = $jetpack_connected && Google_Drive::has_valid_connection(); | ||
} |
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.
@enejb - I'm not quite understanding why we wouldn't want/need this info to be returned on initial load? Like I'd think we might make the endpoint return faster, but only at the cost of not returning useful data? Same question applies to everything we wrapped in this conditional in the PR.
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 want the initial load fast to be as fast as possible and not wait for a .com request to finish loading. Which caused the page to load slow. Since it block the page from loading.
Instead I think we should load things progressively and only do the work that we need as we need it. Not only would this reduce requests on .com but it would also speed things up.
Since most users are not coming to this page to export stuff to google but to see the form responses.
if ( $is_not_init_load ) { | ||
unset( $config['hasAI'] ); | ||
} | ||
|
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.
I think this is causing the test failures. Curious why unset this this, vs just let it default to false as it was doing at the top of the function?
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.
I oped to not return a value so that the frontend might realize this and then load this value via the request on request.
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.
This test well. Approving. But we will need to fix the tests, which are looking for the hasAI
key which may or may not exist now given the changes.
This PR improved the performance of the inital loading of the forms dashboard by making sure that any slow ( things that require .com request don't block the page from loading )
Proposed changes:
Other information:
Jetpack product discussion
None.
Does this pull request change what data or activity we track or use?
No
Testing instructions: