Skip to content
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

Studio: fix site not found error when add a site #442

Merged
merged 6 commits into from
Aug 13, 2024

Conversation

kozer
Copy link
Contributor

@kozer kozer commented Aug 8, 2024

Fixes https://github.com/Automattic/dotcom-forge/issues/8624

Proposed Changes

When we add a new site, the following error is thrown 'executeWPCLiInline': Error: Site not found.
This PR fixes that.

Testing Instructions

  1. Create a new site
  2. Ensure that there is no error in the console

Pre-merge Checklist

  • Have you checked for TypeScript, React or other console errors?

@kozer kozer self-assigned this Aug 8, 2024
@kozer kozer requested a review from a team August 8, 2024 12:10
src/hooks/use-chat-context.tsx Outdated Show resolved Hide resolved
@@ -120,7 +120,9 @@ export const ChatProvider: React.FC< ChatProviderProps > = ( { children } ) => {
! initialLoad[ selectedSite.id ] &&
isCurrent &&
! pluginsList[ selectedSite.id ] &&
! themesList[ selectedSite.id ]
! themesList[ selectedSite.id ] &&
selectedSite.running === true &&
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should we limit this to running sites? If not, the AI chat context won't retrieve the themes and plugins for stopped sites when selecting them. It's true that for the case of the focus listener, we do limit it to sites running. But that's because we only expect running sites to modify the themes/plugins list (discussion).

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I tried removing the condition selectedSite.running === true and, although it works to fetch themes/plugins when selecting already-created sites, it doesn't retrieve properly newly created sites as it's returning empty arrays.

We might need to tweak this logic to ensure we fetch the themes/plugins for the following cases:

  1. After creating a new site.
  2. Selecting a site for the first time in a session (i.e. we didn't perform an initial load).

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@fluiddot , I updated the logic. It should be ok now. What do you think? Do you find any cases that this doesn't produce the expected behavior?

Copy link
Contributor

@wojtekn wojtekn left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@kozer this PR fixes the issue for me.

@kozer kozer requested a review from fluiddot August 12, 2024 13:55
Copy link
Contributor

@fluiddot fluiddot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I've directly tested the chat context by asking the assistant Give me the site context and noticed that we are not saving properly the initial state. The themes and plugins seem to be only saved to the state when the app gains focus on a site running.

It's likely that the changes introduced in #272 have disrupted this logic 😞. I'll take a look and provide a fix. In the meantime, we can merge this PR.

@kozer kozer merged commit 307f951 into trunk Aug 13, 2024
10 checks passed
@kozer kozer deleted the fix/not_found_add_site_error branch August 13, 2024 06:54
@fluiddot
Copy link
Contributor

It's likely that the changes introduced in #272 have disrupted this logic 😞. I'll take a look and provide a fix. In the meantime, we can merge this PR.

I created the following fix for this: #459

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants