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

web-console: Added Statement Timeout Configuration and retry button for timeout #143

Draft
wants to merge 4 commits into
base: main
Choose a base branch
from

Conversation

aikchun
Copy link

@aikchun aikchun commented May 16, 2023

Added an extra param isFromError for toggleRunning to inform the query run in Monaco to run the query that is set from the `ErrorNotification

Added an extra param to queryRaw to set headers for the query.

Added a RetryButton for ErrorNotifcation that are timeouts.

Clicking on the RetryButton will run the query, and also based on the running.value set the button to disabled.
As a general heuristic the query running this way will be doubling the timeout value.

Initial Request with timeout at 6000
timeout-6000

Button disabled while a query is running
Screenshot 2023-05-16 at 9 03 07 PM

Query made with retry button with timeout at 12000
Screenshot 2023-05-16 at 9 03 29 PM

Edit:
UX improvements:
Made the retry button green with greyed out background to increase prominence
Screenshot 2023-05-19 at 7 41 51 PM

When running, the button turns to a stop button that can be used to stop the query if the user wishes.
Screenshot 2023-05-19 at 7 43 34 PM

Some further UX improvements that can be made/considered:

  • Automatic retrying of query with increased timeout.
  • If retrying query still causes a timeout, instead of adding another timeout notification, we can check if the last notifications, is a timeout notification with same query, if so, we can either replace or skip adding the notification.

Some of the best practices for react that I am familiar with are:

  • Use functional components as much as possible (applied in PR)
  • If a state change causes a dispatch, use the dispatch in a useEffect. (applied in PR)
  • Use multiple useEffect to break up the side effects based on their dependencies rather than putting them all in one (applied in PR)
  • Even though useState(heavyComputation()) is initialized once, heavyComputation() will still be invoked everytime the component renders, use useMemo to improve performance.

@CLAassistant
Copy link

CLAassistant commented May 16, 2023

CLA assistant check
All committers have signed the CLA.

@aikchun aikchun marked this pull request as draft May 16, 2023 15:44
@bluestreak01
Copy link
Member

Hi Aik,

There are couple of issue with the PR:

  • I would like to see you apply apply best practices you're familiar with to the PR description
  • It would be good if you attempt to improve the UX of the feature you're adding in this PR

Thanks,
Vlad

@aikchun
Copy link
Author

aikchun commented May 19, 2023

Hi Aik,

There are couple of issue with the PR:

* I would like to see you apply apply best practices you're familiar with to the PR description

* It would be good if you attempt to improve the UX of the feature you're adding in this PR

Thanks, Vlad

@bluestreak01 Hi there. Thanks for the feedback.

I have made some changes to the PR and updated the PR description.
Do let me know if further improvements can be made for this PR or in another.

Thank you
Aik Chun

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.

3 participants