You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@devlake.apache.org by GitBox <gi...@apache.org> on 2022/08/31 03:57:23 UTC
[GitHub] [incubator-devlake] likyh opened a new pull request, #2887: fix: connection saving must return after 2 seconds even though the ne…
likyh opened a new pull request, #2887:
URL: https://github.com/apache/incubator-devlake/pull/2887
# Summary
<!--
Thanks for submitting a pull request!
We appreciate you spending the time to work on these changes.
Please fill out as many sections below as possible.
-->
connection saving must return after 2 seconds even though the network is in localhost.
So I fix it by using `promise.then`.
Actually, the root reason is `Notification` is only displayed when save after several seconds. So I fix the root reason here too.
### Does this close any open issues?
close #2111
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@devlake.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-devlake] e2corporation commented on pull request #2887: fix: connection saving must return after 2 seconds even though the ne…
Posted by GitBox <gi...@apache.org>.
e2corporation commented on PR #2887:
URL: https://github.com/apache/incubator-devlake/pull/2887#issuecomment-1232876262
> @e2corporation need your review.
Have you had a chance to evaluate the last comment by chaining a `.catch(e => Toast......)` to the savePromise and see if that removes the need for using the timeout?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@devlake.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-devlake] e2corporation commented on a diff in pull request #2887: fix: connection saving must return after 2 seconds even though the ne…
Posted by GitBox <gi...@apache.org>.
e2corporation commented on code in PR #2887:
URL: https://github.com/apache/incubator-devlake/pull/2887#discussion_r959537878
##########
config-ui/src/hooks/useConnectionManager.jsx:
##########
@@ -282,7 +285,11 @@ function useConnectionManager (
if (!updateMode) {
history.push(`/integrations/${provider.id}`)
}
- } else {
+ }
+ })
+
+ setTimeout(() => {
Review Comment:
We can't block the UI for 30 seconds, I'll revise the timeout later.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@devlake.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-devlake] likyh commented on a diff in pull request #2887: fix: connection saving must return after 2 seconds even though the ne…
Posted by GitBox <gi...@apache.org>.
likyh commented on code in PR #2887:
URL: https://github.com/apache/incubator-devlake/pull/2887#discussion_r959530269
##########
config-ui/src/hooks/useConnectionManager.jsx:
##########
@@ -282,7 +285,11 @@ function useConnectionManager (
if (!updateMode) {
history.push(`/integrations/${provider.id}`)
}
- } else {
+ }
+ })
+
+ setTimeout(() => {
Review Comment:
> Have you had a chance to evaluate the last comment by chaining a `.catch(e => Toast......)` to the savePromise and see if that removes the need for using the timeout?
This change will lead to waiting most of 30 seconds or more. So I haven't changed it. If we can accept this, we can submit another PR.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@devlake.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-devlake] e2corporation commented on pull request #2887: fix: connection saving must return after 2 seconds even though the ne…
Posted by GitBox <gi...@apache.org>.
e2corporation commented on PR #2887:
URL: https://github.com/apache/incubator-devlake/pull/2887#issuecomment-1234379676
@likyh There is another related issue with one of your changes where the ToastNotification only flashes and the user never gets the chance to read the toast message. Try modifying and existing connection and you will see a toast success flash.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@devlake.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-devlake] likyh commented on pull request #2887: fix: connection saving must return after 2 seconds even though the ne…
Posted by GitBox <gi...@apache.org>.
likyh commented on PR #2887:
URL: https://github.com/apache/incubator-devlake/pull/2887#issuecomment-1232781800
@e2corporation need your review.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@devlake.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-devlake] likyh merged pull request #2887: fix: connection saving must return after 2 seconds even though the ne…
Posted by GitBox <gi...@apache.org>.
likyh merged PR #2887:
URL: https://github.com/apache/incubator-devlake/pull/2887
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@devlake.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-devlake] e2corporation commented on a diff in pull request #2887: fix: connection saving must return after 2 seconds even though the ne…
Posted by GitBox <gi...@apache.org>.
e2corporation commented on code in PR #2887:
URL: https://github.com/apache/incubator-devlake/pull/2887#discussion_r960609437
##########
config-ui/src/hooks/useConnectionManager.jsx:
##########
@@ -256,13 +256,16 @@ function useConnectionManager (
}
}
+ let savePromise
if (updateMode && activeConnection?.id !== null) {
modifyConfiguration(connectionPayload)
+ savePromise = modifyConfiguration(connectionPayload)
Review Comment:
✅ `FIXED` with PR https://github.com/apache/incubator-devlake/pull/2916
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@devlake.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-devlake] e2corporation commented on a diff in pull request #2887: fix: connection saving must return after 2 seconds even though the ne…
Posted by GitBox <gi...@apache.org>.
e2corporation commented on code in PR #2887:
URL: https://github.com/apache/incubator-devlake/pull/2887#discussion_r960590211
##########
config-ui/src/hooks/useConnectionManager.jsx:
##########
@@ -256,13 +256,16 @@ function useConnectionManager (
}
}
+ let savePromise
if (updateMode && activeConnection?.id !== null) {
modifyConfiguration(connectionPayload)
+ savePromise = modifyConfiguration(connectionPayload)
Review Comment:
@likyh Just now noticing you called `modifyConfiguration twice`? On lines 261 and 262, and same for `saveConfiguration` on `264` and `265` this isn't right you are sending 2 post requests.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@devlake.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-devlake] e2corporation commented on a diff in pull request #2887: fix: connection saving must return after 2 seconds even though the ne…
Posted by GitBox <gi...@apache.org>.
e2corporation commented on code in PR #2887:
URL: https://github.com/apache/incubator-devlake/pull/2887#discussion_r959339865
##########
config-ui/src/hooks/useConnectionManager.jsx:
##########
@@ -282,7 +285,11 @@ function useConnectionManager (
if (!updateMode) {
history.push(`/integrations/${provider.id}`)
}
- } else {
+ }
+ })
+
+ setTimeout(() => {
Review Comment:
If you're going to assign returned save & update promises at this point in the code-flow then why bother keeping the existing timeout? It would make more sense to chain a catch call and add then toast a notification on the failure so the timeout wrap can be removed.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@devlake.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org