Fix excessive WP-CLI command invocations in Assistant tab - #2472
Conversation
| // eslint-disable-next-line react-hooks/exhaustive-deps | ||
| }, [ dispatch, selectedSite.id ] ); |
There was a problem hiding this comment.
This is the key change. The selectedSite object is being mutated frequently somewhere, so we change the dependency to selectedSite.id
There was a problem hiding this comment.
Good catch! I think we could create a follow-up to identify the root cause, but for the time being, this is a good fix that prevents the re-runs when properties other than id change.
There was a problem hiding this comment.
I think we could create a follow-up to identify the root cause
It could be that the themeDetails object is overwritten more frequently now. That's not necessarily an error, IMO. Depending on the entire selectedSite object here might just have been a flawed strategy…
Still, it would be good to understand why this happened in the first place.
| 'x-quota-reset': z.string().datetime( { offset: true } ), | ||
| 'x-quota-reset': z.iso.datetime( { offset: true } ), |
There was a problem hiding this comment.
| { | ||
| condition: ( { site }, { getState } ) => { | ||
| const { chat } = getState(); | ||
| const thunkIsCurrentlyLoading = chat.isLoadingUpdateFromSiteDict[ site.id ]; | ||
| return ! thunkIsCurrentlyLoading; | ||
| }, | ||
| } |
There was a problem hiding this comment.
Adds additional safety by preventing multiple parallel updateFromSite invocations
📊 Performance Test ResultsComparing 408de2c vs trunk site-editor
site-startup
Results are median values from multiple test runs. Legend: 🟢 Improvement (faster) | 🔴 Regression (slower) | ⚪ No change |
epeicher
left a comment
epeicher
left a comment
There was a problem hiding this comment.
Thanks for the fix @fredrikekelund! I have tested it, and it works as expected. I don't see any errors in the console after using the AI assistant.
| Before | After |
|---|---|
 |
 |
| // eslint-disable-next-line react-hooks/exhaustive-deps | ||
| }, [ dispatch, selectedSite.id ] ); |
There was a problem hiding this comment.
Good catch! I think we could create a follow-up to identify the root cause, but for the time being, this is a good fix that prevents the re-runs when properties other than id change.
gcsecsey
left a comment
gcsecsey
left a comment
There was a problem hiding this comment.
The approach and the changes make sense to me. I couldn't reproduce the issue on trunk on Mac, so I can't confirm yet if the changes eliminated the errors on my machine.
I tested sending messages to ask about the installed plugins/themes on the site, or to print some WP CLI commands for me that I ran, but didn't get any errors in the console:
| trunk | After |
|---|---|
 |
 |
I see in Sentry that the issue is platform agnostic, so I think I should be able to reproduce it on Mac:
@fredrikekelund do you have any tips or steps on how to produce the excessive WPCLI calls on trunk?
ivan-ottinger
left a comment
ivan-ottinger
left a comment
There was a problem hiding this comment.
Thanks for the fix, Fredrik. The change looks good and I did not observe any regressions. The Assistant still works as expected.
Regarding the root issue - have you been able to reproduce it? I haven't been able so far.
| import * as Sentry from '@sentry/electron/renderer'; | ||
| import { WPCOM } from 'wpcom/types'; | ||
| import { z } from 'zod'; | ||
| import { DEFAULT_PHP_VERSION } from 'common/constants'; |
I have. It looks like @epeicher could, too (see #2472 (review)). The issue comes from excessive @gcsecsey @ivan-ottinger, maybe you cannot trigger the errors on trunk because the server wasn't running… |
5 participants
Related issues
Proposed Changes
The 1.7.0 release introduced a bug that caused an excessive number of WP-CLI invocations in the Assistant tab (triggered by the
updateFromSitethunk). This led to the commands failing and showing up as JSON parsing errors in the browser console. This seems to be caused by mutations of theselectedSiteobject returned by theuseSiteDetailshook.This PR fixes the issue by changing the dependency array of the
useEffecthook that triggers theupdateFromSitethunk, and by adding aconditionconfig to the thunk itself that prevents parallel invocations.Testing Instructions
npm startUnexpected end of JSON inputmessages in the devtools consolePre-merge Checklist