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.

Sign up for GitHub

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

Jump to bottom

Add UI for dynamic code block to execute Assistant wp-cli commands #214

Merged

katinthehatsite merged 24 commits into trunk from feat/dynamic-assistant-code-block

Jun 11, 2024

Contributor

derekblank commented Jun 6, 2024 •

edited

Loading

Resolves Automattic/dotcom-forge#7526

Proposed Changes

Adds dynamic code block to display output of wp-cli commands, to be integrated with output of add/execute-cli-inline implemented in #203.

dynamic-code-block.mov

JSX Example

A visual example of the dynamic code block JSX tree, and some of the components introduced in this PR. Open to feedback/ideas (and I would expect this tree to change somewhat as we integrate the wp-cli commands).

The general flow:

The <Message role="assistant" /> Assistant API response is parsed using react-markdown.
When a fenced code block is encountered, render <CodeBlock /> as a subcomponent of react-markdown to display the command to the user using basic syntax highlighting.
A button row containing two <ActionButton /> components is appended to the bottom of each <CodeBlock />. containing the Copy and Run buttons.
When the Run <ActionButton /> button is clicked, the command starts executing. Display the <Spinner /> while the command executes.
When the command has finished executing, display the result using the <InlineCLI /> component.

code-block-jsx

Testing Instructions

Start app: STUDIO_AI=true npm start
Navigate to Assistant tab
Ask the Assistant to show a code block (if none available): "Show me some code"
Observe the components mentioned above are rendered

Pre-merge Checklist

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

Todo

Consider integration details with Studio: Execute wp-cli inside Studio and receive its output #203
Resolve Code block type errors

derekblank added 4 commits

June 5, 2024 17:00


          Update Assistant Markdown styles to use class instead of id

3b4b651


          Remove unneccessary container div

1c77bad


          Merge branch 'fix/markdown-updates' into feat/dynamic-assistant-code-…

a3eba8e

…block


          Add UI for Assistant dynamic code blocks

5f146df

derekblank added the [Type] Enhancement label

derekblank requested review from sejas and katinthehatsite

June 6, 2024 07:18

derekblank self-assigned this

derekblank mentioned this pull request

Studio: Execute wp-cli inside Studio and receive its output #203

Merged

1 task


          Update todo notes

derekblank commented

View reviewed changes

src/components/content-tab-assistant.tsx Outdated Show resolved Hide resolved

derekblank commented

View reviewed changes

src/components/content-tab-assistant.tsx Outdated

+              	<div className="mt-1 p-4 bg-[#2D3337]">
+              		<div className="flex justify-between mb-2 font-sans">
+              			<span className={ status === 'success' ? 'text-[#63CE68]' : 'text-[#E66D6C]' }>
+              				{ status === 'success' ? 'Success' : 'Error' }

Contributor Author

derekblank Jun 6, 2024

Likely no need to repeat the status logic here once integrated with wp-cli -- this logic is just for demonstration, and could be refactored into separate states.

derekblank commented

View reviewed changes

src/components/content-tab-assistant.tsx Outdated Show resolved Hide resolved

derekblank commented

View reviewed changes

src/components/content-tab-assistant.tsx

Comment on lines +27 to +31

+              interface InlineCLIProps {
+              	output: string;
+              	status: 'success' | 'error';
+              	time: string;
+              }

Contributor Author

derekblank Jun 6, 2024 •

edited

Loading

This interface is likely to change based on the final shape of the wp-cli implementation in #203 -- just chose some probable params for UI demonstration purposes.

derekblank added 10 commits

June 6, 2024 19:52


          Remove inline from from Markdown render

178842f

 was removed in favor of :
remarkjs/react-markdown@4346276


          Remove duplicate pre tag when parsing code blocks

d6baaeb


          Update styling for non-Markdown code blocks

ef34a80


          Update Copy Text button handler

981a20e


          Update ActionButton handlers to use a second label when actioned

2b67bda


          Add secondIcon to ActionButton

8c26996


          Update Run button handler

5ac1502


          Update ActionButton styles to use tertiary variant

c6dfe6a


          Add a Spinner component to InlineCLI block

80d195d


          Update isRunning state

12289a0

derekblank mentioned this pull request

Update Assistant Markdown styles #208

Merged

1 task

derekblank added 3 commits

June 7, 2024 08:47


          Merge trunk and resolve conflicts

d36263b


          Use natural margins for Assistant Markdown headings

7ab76fc


          Refactor markdown code components to CodeBlock

ce1b83e

derekblank commented

View reviewed changes

src/components/content-tab-assistant.tsx Show resolved Hide resolved

derekblank marked this pull request as ready for review

June 7, 2024 05:13

derekblank requested a review from dcalhoun

June 7, 2024 05:13

Contributor Author

derekblank commented Jun 7, 2024 •

edited

Loading

This approach is at ~85% polish, but marking as "Ready for Review" to get feedback.

~~A current blocker to merging are some type errors when parsing nested code blocks from react-markdown (further info at #214 (comment)). Any feedback here is quite welcome.~~

Edit: Markdown code block type issues should now be resolved by 19202ac.

derekblank commented

View reviewed changes

src/components/button.tsx

@@ @@ -107,6 +120,7 @@ export default function ButtonComponent( { @@
               				baseStyles,
               				variant === 'primary' && primaryStyles,
               				variant === 'secondary' && secondaryStyles,
+              				variant === 'tertiary' && tertiaryStyles,

Contributor Author

derekblank Jun 7, 2024

Leverage tertiary for InlineCLI ActionButtons. (tertiary is currently unused as a <Button /> variant in trunk.)

Member

dcalhoun Jun 11, 2024 •

edited

Loading

I noted these new tertiary styles modified the existing usage of tertiary in the add site modal. We might consider consolidating button styles (i.e., ask the design team if we can reduce the number of button styles used) or rename this new style. WDYT?

Before	After

studio/src/components/add-site.tsx

Line 140 in 216154a

<Button onClick={ closeModal } disabled={ isAddingSite } variant="tertiary">

Contributor

matt-west Jun 12, 2024

@dcalhoun Good catch! I think we should use a different name for the buttons in code blocks as they're a bit of a special use case. I don't anticipate on re-using those dark styles anywhere else in the app at the moment.

Contributor

matt-west Jun 12, 2024

On second thought, let's update these buttons to use the same styling as the Add site button in the sidebar (outlined, not filled). That helps bring more consistency to the button styles in the app. (Figma)

Member

dcalhoun Jun 12, 2024

For posterity, I drafted #243 addressing the button styling.


          Fix Markdown code block typing

19202ac

Contributor

katinthehatsite commented Jun 10, 2024

Hi @derekblank - I did not have time to take a look at this one today but I will review tomorrow and probably create a new PR against this one that connects to whatever we implemented in #203


          Merge remote-tracking branch 'origin' into feat/dynamic-assistant-cod…

b821649

…e-block

Contributor Author

derekblank commented Jun 11, 2024

Hi @derekblank - I did not have time to take a look at this one today but I will review tomorrow and probably create a new PR against this one that connects to whatever we implemented in #203

@katinthehatsite No worries -- I updated the PR description with a visual example of the JSX tree for the components introduced in this PR, if it's helpful.

I also merged the latest from trunk into this branch, so now executeWPCLiInline handlers are available alongside this code -- I think creating a new branch and PR based off this one is a good approach to handle the integration.

Under the current UI structure proposed in this PR, the Run button handler in this PR would be replaced by the executeWPCLiInline handler from #203, and the output of that handler would be rendered in the <InlineCLI /> component. Open to feedback/ideas on changing this as the output from #203 is integrated, and also happy to help with the integration too.

derekblank and others added 4 commits

June 11, 2024 16:17


          Fix unauthenticated view HTML syntax

bac4d2a

Prevents DOM validation warning:

  Warning: validateDOMNesting(...): <div> cannot appear as a descendant of <p>.


          Disable Run button for code block for now

a0bcac3


          Replace spinner with wordpress components

65313ac


          Translate error and success

6e48014

sejas approved these changes

View reviewed changes

Member

sejas left a comment

@derekblank , thanks for creating all the UI elements.

I replaced the spinner to use the default from WordPress components here 65313ac

We'll merge this PR and @katinthehatsite will create a new PR for the integration with wp-cli.
We commented the Run button for now.

Thank you!

katinthehatsite approved these changes

View reviewed changes

Contributor

katinthehatsite left a comment

Nice work! We made a couple of minor changes and disabled the "Run" button for now.

Contributor

katinthehatsite commented Jun 11, 2024

@derekblank I think it might be good to also add the tests for the code block. We can handle it as a part of a different PR after we implemented the execution part.

katinthehatsite merged commit af60614 into trunk

11 checks passed

katinthehatsite deleted the feat/dynamic-assistant-code-block branch

June 11, 2024 11:49

dcalhoun reviewed

View reviewed changes

Member

dcalhoun left a comment

When asking the AI assistant to "Show me some code," I encountered an oddity with what appears to be an "inline" code block, but was rendered with a div causing the text to wrap multiple lines. Is this expected? I figure this should be a single line of text.

Screenshot 2024-06-11 at 11 47 41

src/components/button.tsx

@@ @@ -107,6 +120,7 @@ export default function ButtonComponent( { @@
               				baseStyles,
               				variant === 'primary' && primaryStyles,
               				variant === 'secondary' && secondaryStyles,
+              				variant === 'tertiary' && tertiaryStyles,

Member

dcalhoun Jun 11, 2024 •

edited

Loading

I noted these new tertiary styles modified the existing usage of tertiary in the add site modal. We might consider consolidating button styles (i.e., ask the design team if we can reduce the number of button styles used) or rename this new style. WDYT?

Before	After

studio/src/components/add-site.tsx

Line 140 in 216154a

<Button onClick={ closeModal } disabled={ isAddingSite } variant="tertiary">

dcalhoun mentioned this pull request

fix: Improve button style consistency #243

Merged

1 task

Member

dcalhoun commented Jun 12, 2024

When asking the AI assistant to "Show me some code," I encountered an oddity with what appears to be an "inline" code block, but was rendered with a div causing the text to wrap multiple lines. Is this expected? I figure this should be a single line of text.

Noting the unexpected spacing for inline code was addressed in #235. 🎉

However, it appears an error regarding invalid HTML remains.

HTML error

Warning: validateDOMNesting(...): <div> cannot appear as a descendant of <p>.
    at div
    at CodeBlock (http://localhost:3456/main_window/index.js:261917:17)
    at p
    at Markdown (http://localhost:3456/main_window/index.js:315155:35)
    at div
    at div
    at div
    at Message (http://localhost:3456/main_window/index.js:261902:20)
    at http://localhost:3456/main_window/index.js:261925:48
    at div
    at div
    at ContentTabAssistant (http://localhost:3456/main_window/index.js:261929:32)
    at div
    at div
    at ContextProvider
    at ScopedContextProvider
    at ContextProvider
    at ContextProvider
    at ScopedContextProvider
    at ContextProvider
    at ContextProvider
    at ContextProvider
    at ScopedContextProvider
    at ContextProvider
    at ScopedContextProvider
    at ContextProvider
    at ContextProvider
    at ScopedContextProvider
    at Role (http://localhost:3456/main_window/index.js:269000:32)
    at div
    at UnforwardedTabPanel (http://localhost:3456/main_window/index.js:114936:3)
    at div
    at SiteContentTabs (http://localhost:3456/main_window/index.js:263294:68)
    at main
    at div
    at http://localhost:3456/main_window/index.js:1092:66
    at UnconnectedHStack (http://localhost:3456/main_window/index.js:100429:71)
    at div
    at http://localhost:3456/main_window/index.js:1092:66
    at UnconnectedVStack (http://localhost:3456/main_window/index.js:122761:71)
    at App (http://localhost:3456/main_window/index.js:261483:59)
    at PromptUsageProvider (http://localhost:3456/main_window/index.js:264870:32)
    at OnboardingProvider (http://localhost:3456/main_window/index.js:264753:31)
    at InstalledAppsProvider (http://localhost:3456/main_window/index.js:264189:34)
    at ThemeDetailsProvider (http://localhost:3456/main_window/index.js:265425:33)
    at DemoSiteUpdateProvider (http://localhost:3456/main_window/index.js:265585:35)
    at SiteDetailsProvider (http://localhost:3456/main_window/index.js:265128:32)
    at AuthProvider (http://localhost:3456/main_window/index.js:261572:25)
    at ErrorBoundary (http://localhost:3456/main_window/index.js:262806:9)
    at Root

katinthehatsite mentioned this pull request

Studio: Add UI for chat offline mode #271

Merged

1 task

sejas mentioned this pull request

Remove unnecessary setTimeout in useExecuteWPCLI hook code block #308

Merged

1 task

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