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: Add chat context #252

Merged
merged 11 commits into from
Jun 19, 2024
Merged

Studio: Add chat context #252

merged 11 commits into from
Jun 19, 2024

Conversation

kozer
Copy link
Contributor

@kozer kozer commented Jun 14, 2024

Proposed Changes

This PR handles adding extra chat context to the API request.
Fixes: https://github.com/Automattic/dotcom-forge/issues/7601

Testing Instructions

  1. Apply D152354-code
  2. Run the above Studio pr locally.
  3. Make sure you have at least 1 site.
  4. Make questions related to your site:

eg: what is my wordpress version? What is my php version? What are my themes?

  1. Ensure that you get the correct response like below:

2024-06-17T12:24:12,515900609+03:00
2024-06-17T12:24:21,565267570+03:00
2024-06-17T12:24:30,996942316+03:00

Pre-merge Checklist

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

@kozer kozer marked this pull request as draft June 14, 2024 14:51
@kozer kozer self-assigned this Jun 17, 2024
@kozer kozer requested a review from a team June 17, 2024 09:25
@kozer kozer marked this pull request as ready for review June 17, 2024 09:38
@kozer
Copy link
Contributor Author

kozer commented Jun 17, 2024

NOTE: We ll need to change things if #213 is merged first.

@kozer kozer requested a review from fluiddot June 19, 2024 07:46
@fluiddot fluiddot requested a review from a team June 19, 2024 08:02
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 tested different prompts and worked great, awesome work @kozer 🏅 !

Additionally, I shared some comments but I think none should block the PR.

For reference, with the prompt Share the context of the site we can easily see the values passed for the current site:

Screenshot 2024-06-19 at 11 40 08

src/hooks/use-chat-context.tsx Outdated Show resolved Hide resolved
src/hooks/use-chat-context.tsx Outdated Show resolved Hide resolved
src/lib/is-installed.ts Outdated Show resolved Hide resolved
vendor/wp-now/src/execute-wp-cli.ts Outdated Show resolved Hide resolved
vendor/wp-now/src/execute-wp-cli.ts Show resolved Hide resolved
src/hooks/use-chat-context.tsx Show resolved Hide resolved
src/hooks/use-chat-context.tsx Outdated Show resolved Hide resolved
@fluiddot fluiddot requested a review from a team June 19, 2024 09:45
@kozer
Copy link
Contributor Author

kozer commented Jun 19, 2024

@fluiddot , I did all the changes you requested. Can you please check again? Thanks!

@kozer kozer merged commit b52020a into trunk Jun 19, 2024
10 checks passed
@kozer kozer deleted the add/api_context branch June 19, 2024 11:26
Copy link
Member

@sejas sejas left a comment

Choose a reason for hiding this comment

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

@kozer , the happy path works great! I've been updating the php version and installing plugins. It works great. Here is an screenshot.

Screenshot 2024-06-19 at 12 18 31

I have a concern about running wp-cli commands multiple times when the site or focus changes. I found that changing the site in the left sidebar has a performance issue and freezes the app, even in the overview tab. Is possible to fix it?

Hhodxe.mp4
17McGF.mp4

@fluiddot
Copy link
Contributor

@fluiddot , I did all the changes you requested. Can you please check again? Thanks!

They look great, thanks 🙇 !

@fluiddot
Copy link
Contributor

I have a concern about running wp-cli commands multiple times when the site or focus changes. I found that changing the site in the left sidebar has a performance issue and freezes the app, even in the overview tab. Is possible to fix it?

Oh, I've just noticed this issue when selecting different sites, thanks for spotting it @sejas. Sorry to overlook it when testing it, as it severely impacts the performance of basic actions like selecting sites 😞 .

I think the main problem comes from the fact that the initial load is only marked as complete when all promises are finished (reference). A potential workaround could be to mark it as complete right before invoking the promises to avoid executing the wp-cli commands multiple times. In case they fail, we could mark the initial load as empty to let the app try again later on.

Another enhancement would be to debounce the run to avoid invoking multiple times the wp-cli commands when selecting different sites quickly.

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.

None yet

3 participants