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

Remove unnecessary setTimeout in useExecuteWPCLI hook code block #308

Merged
merged 3 commits into from
Jul 23, 2024

Conversation

sejas
Copy link
Member

@sejas sejas commented Jun 26, 2024

Related to #214

Proposed Changes

  • Remove unnecessary timeout that was added to fake the execution time

Sometimes I was receiving an error even the code block was executed correctly. It's hard to replicate and I think it could be related to this overlooked timeout.

Testing Instructions

  • Run Studio with the assistant
  • Open the assistant tab
  • Send a message to return a wp-cli command, like "generate 10 fake posts"
  • Click run
  • Observe the code block is still working as expected

Pre-merge Checklist

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

@sejas sejas self-assigned this Jun 26, 2024
@sejas sejas requested a review from a team June 26, 2024 11:00
@@ -30,32 +30,27 @@ export function useExecuteWPCLI(
args: args.join( ' ' ),
} );

setTimeout( () => {
Copy link
Member

Choose a reason for hiding this comment

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

I recently encountered this timeout and wondered why it existed. 😅

When performing the test plan, I encountered an odd success output where the incomplete progress indicator remained in place. Do you think the timeout was in place to ensure the correct output was captured and displayed? Is there an alternative to a timeout that might address this?

image

If we do remove the timeout, would you be willing to update the recently merged Assistant code block tests to remove the now unnecessary fake timers? We may need to still await the promise resolution via await new Promise( process.nextTick ) or some other mechanism.

@sejas
Copy link
Member Author

sejas commented Jul 22, 2024

I've updated the tests removing the unnecessary useFakeTimers. src/hooks/tests/use-assistant.test.ts is still using it, but for a different timeout.

Copy link
Contributor

@katinthehatsite katinthehatsite left a comment

Choose a reason for hiding this comment

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

The changes look good and the code block still works expected for me 👍

I don't recollect if the horizontal scrolling is supposed to be present in the generated code block once the command runs but that issue is not related to the changes in this PR:

Screenshot 2024-07-23 at 10 41 32 AM

@sejas sejas merged commit b4f1558 into trunk Jul 23, 2024
10 checks passed
@sejas sejas deleted the remove/code-block-timeout branch July 23, 2024 12:25
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants