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/09/29 13:28:16 UTC
[GitHub] [incubator-devlake] likyh opened a new pull request, #3260: fix: fix migration dialog only appear after refresh
likyh opened a new pull request, #3260:
URL: https://github.com/apache/incubator-devlake/pull/3260
# 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.
-->
1. fix migration dialog only appear after refresh
2. fix migration cannot success
### Does this close any open issues?
Closes #3255
Closes #3109
--
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 #3260: fix: fix migration dialog only appear after refresh
Posted by GitBox <gi...@apache.org>.
likyh commented on PR #3260:
URL: https://github.com/apache/incubator-devlake/pull/3260#issuecomment-1262315724
> I like your proposed refactored solution, however I think if we tried to add a history push and re-route the user to another location when we receive the migration warning my original design of using local storage key will work as expected.
You means add history push in https://github.com/apache/incubator-devlake/blob/main/config-ui/src/utils/request.js#L43 ?
https://github.com/apache/incubator-devlake/c732e7bddeb1f498873c6cc807e66772deb205e9/main/config-ui/src/utils/request.js#L43
--
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 #3260: fix: fix migration dialog only appear after refresh
Posted by GitBox <gi...@apache.org>.
likyh commented on code in PR #3260:
URL: https://github.com/apache/incubator-devlake/pull/3260#discussion_r983597610
##########
config-ui/src/hooks/useDatabaseMigrations.jsx:
##########
@@ -36,17 +34,14 @@ function useDatabaseMigrations(Configuration = MigrationOptions) {
const migrate = async () => {
const m = await request.get(Configuration.apiProceedEndpoint)
setWasMigrationSuccessful(m?.status === 200 && m?.data?.success === true)
- setTimeout(() => {
Review Comment:
> That screenshot is after you commented/disabled the timeout or with the timeout enabled?
It's fix the typo `wasSuccessful={wasMigrationSuccessful}` on old code.
--
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 #3260: fix: fix migration dialog only appear after refresh
Posted by GitBox <gi...@apache.org>.
likyh commented on code in PR #3260:
URL: https://github.com/apache/incubator-devlake/pull/3260#discussion_r983609823
##########
config-ui/src/hooks/useDatabaseMigrations.jsx:
##########
@@ -36,17 +34,14 @@ function useDatabaseMigrations(Configuration = MigrationOptions) {
const migrate = async () => {
const m = await request.get(Configuration.apiProceedEndpoint)
setWasMigrationSuccessful(m?.status === 200 && m?.data?.success === true)
- setTimeout(() => {
Review Comment:
you want show loading time = 3s + actuallyTime or time = Max(3s, actuallyTime) or time = Min(3s, actuallyTime)?
--
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 #3260: fix: fix migration dialog only appear after refresh
Posted by GitBox <gi...@apache.org>.
e2corporation commented on code in PR #3260:
URL: https://github.com/apache/incubator-devlake/pull/3260#discussion_r983638346
##########
config-ui/src/hooks/useDatabaseMigrations.jsx:
##########
@@ -36,17 +34,14 @@ function useDatabaseMigrations(Configuration = MigrationOptions) {
const migrate = async () => {
const m = await request.get(Configuration.apiProceedEndpoint)
setWasMigrationSuccessful(m?.status === 200 && m?.data?.success === true)
- setTimeout(() => {
Review Comment:
The intent here is the dialog should display for at least 3 seconds so user can see the migration happening. If migration completes in 0.05 seconds, we still want the 3 second window so user can read the message. If DB Migration request is still processing and takes 30 seconds to complete, the dialog should remain open in the migrating state for 30SECONDS + 3SECONDS so 33seconds total
--
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 #3260: fix: fix migration dialog only appear after refresh
Posted by GitBox <gi...@apache.org>.
e2corporation commented on code in PR #3260:
URL: https://github.com/apache/incubator-devlake/pull/3260#discussion_r983558072
##########
config-ui/src/hooks/useDatabaseMigrations.jsx:
##########
@@ -36,17 +34,14 @@ function useDatabaseMigrations(Configuration = MigrationOptions) {
const migrate = async () => {
const m = await request.get(Configuration.apiProceedEndpoint)
setWasMigrationSuccessful(m?.status === 200 && m?.data?.success === true)
- setTimeout(() => {
Review Comment:
Line 35 is synchronous as await is being used `const m = await request.get(Configuration.apiProceedEndpoint)`.
--
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 #3260: fix: fix migration dialog only appear after refresh
Posted by GitBox <gi...@apache.org>.
likyh commented on code in PR #3260:
URL: https://github.com/apache/incubator-devlake/pull/3260#discussion_r983639594
##########
config-ui/src/hooks/useDatabaseMigrations.jsx:
##########
@@ -36,17 +34,14 @@ function useDatabaseMigrations(Configuration = MigrationOptions) {
const migrate = async () => {
const m = await request.get(Configuration.apiProceedEndpoint)
setWasMigrationSuccessful(m?.status === 200 && m?.data?.success === true)
- setTimeout(() => {
Review Comment:
So loading time = 3s + actuallyTime?
--
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 #3260: fix: fix migration dialog only appear after refresh
Posted by GitBox <gi...@apache.org>.
e2corporation commented on code in PR #3260:
URL: https://github.com/apache/incubator-devlake/pull/3260#discussion_r983552730
##########
config-ui/src/hooks/useDatabaseMigrations.jsx:
##########
@@ -36,17 +34,14 @@ function useDatabaseMigrations(Configuration = MigrationOptions) {
const migrate = async () => {
const m = await request.get(Configuration.apiProceedEndpoint)
setWasMigrationSuccessful(m?.status === 200 && m?.data?.success === true)
- setTimeout(() => {
Review Comment:
This timeout is for latency and shouldn't be removed, by removing it the user will not see the migration message instead it will just "flash" and be a confusing state. This gives the user at least a 3-second window, it does not block migration from actually happening.
--
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 #3260: fix: fix migration dialog only appear after refresh
Posted by GitBox <gi...@apache.org>.
likyh commented on code in PR #3260:
URL: https://github.com/apache/incubator-devlake/pull/3260#discussion_r983568501
##########
config-ui/src/hooks/useDatabaseMigrations.jsx:
##########
@@ -36,17 +34,14 @@ function useDatabaseMigrations(Configuration = MigrationOptions) {
const migrate = async () => {
const m = await request.get(Configuration.apiProceedEndpoint)
setWasMigrationSuccessful(m?.status === 200 && m?.data?.success === true)
- setTimeout(() => {
Review Comment:
Let's write a common helper to sovle this problem. Because if add setTimeout out of async migrate, I must write some logic to deal which one finish first between setTimeout and `request.get`.
--
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 #3260: fix: fix migration dialog only appear after refresh
Posted by GitBox <gi...@apache.org>.
likyh commented on code in PR #3260:
URL: https://github.com/apache/incubator-devlake/pull/3260#discussion_r983557802
##########
config-ui/src/hooks/useDatabaseMigrations.jsx:
##########
@@ -36,17 +34,14 @@ function useDatabaseMigrations(Configuration = MigrationOptions) {
const migrate = async () => {
const m = await request.get(Configuration.apiProceedEndpoint)
setWasMigrationSuccessful(m?.status === 200 && m?.data?.success === true)
- setTimeout(() => {
Review Comment:
and `setWasMigrationSuccessful` should write in it.
--
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 #3260: fix: fix migration dialog only appear after refresh
Posted by GitBox <gi...@apache.org>.
likyh commented on PR #3260:
URL: https://github.com/apache/incubator-devlake/pull/3260#issuecomment-1262305051
> @likyh The reason for a refresh being needed is so the local storage key is picked up. If the user navigates away or reloads the page the warning will happen, for a more instant warning we could just try to use a `history.push` to send the user to /dashboard or some alt route so the migration warning local storage key is detected.
But now it can not appear when first open. So I use a new way to get if migration needed.
Now it show appear when first request sent. (Usually is `/versions`)
--
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 #3260: fix: fix migration dialog only appear after refresh
Posted by GitBox <gi...@apache.org>.
e2corporation commented on code in PR #3260:
URL: https://github.com/apache/incubator-devlake/pull/3260#discussion_r983560072
##########
config-ui/src/hooks/useDatabaseMigrations.jsx:
##########
@@ -36,17 +34,14 @@ function useDatabaseMigrations(Configuration = MigrationOptions) {
const migrate = async () => {
const m = await request.get(Configuration.apiProceedEndpoint)
setWasMigrationSuccessful(m?.status === 200 && m?.data?.success === true)
- setTimeout(() => {
Review Comment:
You can clearly see the effect on the UI if you remove this timeout, the user can't read the migration message.
--
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 #3260: fix: fix migration dialog only appear after refresh
Posted by GitBox <gi...@apache.org>.
e2corporation commented on PR #3260:
URL: https://github.com/apache/incubator-devlake/pull/3260#issuecomment-1262318848
> > > @likyh The reason for a refresh being needed is so the local storage key is picked up. If the user navigates away or reloads the page the warning will happen, for a more instant warning we could just try to use a `history.push` to send the user to /dashboard or some alt route so the migration warning local storage key is detected.
> >
> >
> > But now it can not appear when first open. So I use a new way to get if migration needed.
> > Now it show appear when first request sent. (Usually is `/versions`)
>
> I like your proposed refactored solution, however I think if we tried to add a history push and re-route the user to another location when we receive the migration warning my original design of using local storage key will work as expected.
> > I like your proposed refactored solution, however I think if we tried to add a history push and re-route the user to another location when we receive the migration warning my original design of using local storage key will work as expected.
>
> Do you mean that add history push in `https://github.com/apache/incubator-devlake/blob/main/config-ui/src/utils/request.js#L43` ?
>
> https://github.com/apache/incubator-devlake/blob/c732e7bddeb1f498873c6cc807e66772deb205e9/config-ui/src/utils/request.js#L40-L50
We can't use a Hook call at this layer as it breaks core hook rules regarding usage, (Additionally, I left this utility Pure and it is Not a React component) what we need to do is find out where we can check the storage key first and send the appropriate signal.
--
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 #3260: fix: fix migration dialog only appear after refresh
Posted by GitBox <gi...@apache.org>.
e2corporation commented on PR #3260:
URL: https://github.com/apache/incubator-devlake/pull/3260#issuecomment-1262311449
> > @likyh The reason for a refresh being needed is so the local storage key is picked up. If the user navigates away or reloads the page the warning will happen, for a more instant warning we could just try to use a `history.push` to send the user to /dashboard or some alt route so the migration warning local storage key is detected.
>
> But now it can not appear when first open. So I use a new way to get if migration needed.
>
> Now it show appear when first request sent. (Usually is `/versions`)
I like your proposed refactored solution, however I think if we tried to add a history push and re-route the user to another location when we receive the migration warning my original design of using local storage key will work as expected.
--
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 #3260: fix: fix migration dialog only appear after refresh
Posted by GitBox <gi...@apache.org>.
e2corporation commented on code in PR #3260:
URL: https://github.com/apache/incubator-devlake/pull/3260#discussion_r983572525
##########
config-ui/src/hooks/useDatabaseMigrations.jsx:
##########
@@ -36,17 +34,14 @@ function useDatabaseMigrations(Configuration = MigrationOptions) {
const migrate = async () => {
const m = await request.get(Configuration.apiProceedEndpoint)
setWasMigrationSuccessful(m?.status === 200 && m?.data?.success === true)
- setTimeout(() => {
Review Comment:
The timeout is not doing much... we wait 3 seconds then 1) Turn off the Processing flag, 2) Set the Migration Failed flag based on the response. `m` is awaited, so it should be blocking. If the request to migrate takes 3 or 30 seconds, the timeout should execute _after_ that time not before.
--
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 #3260: fix: fix migration dialog only appear after refresh
Posted by GitBox <gi...@apache.org>.
e2corporation commented on code in PR #3260:
URL: https://github.com/apache/incubator-devlake/pull/3260#discussion_r983640385
##########
config-ui/src/hooks/useDatabaseMigrations.jsx:
##########
@@ -36,17 +34,14 @@ function useDatabaseMigrations(Configuration = MigrationOptions) {
const migrate = async () => {
const m = await request.get(Configuration.apiProceedEndpoint)
setWasMigrationSuccessful(m?.status === 200 && m?.data?.success === true)
- setTimeout(() => {
Review Comment:
If the timeout is truly causing a break in the flow we can comment/disable it for now. But my understanding of the await call is that line should be blocking. So that initial request whether it takes 1 second or 30 seconds to receive a response from backend, it should still be blocking until time timeout fires
--
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 #3260: fix: fix migration dialog only appear after refresh
Posted by GitBox <gi...@apache.org>.
likyh commented on code in PR #3260:
URL: https://github.com/apache/incubator-devlake/pull/3260#discussion_r983589119
##########
config-ui/src/hooks/useDatabaseMigrations.jsx:
##########
@@ -36,17 +34,14 @@ function useDatabaseMigrations(Configuration = MigrationOptions) {
const migrate = async () => {
const m = await request.get(Configuration.apiProceedEndpoint)
setWasMigrationSuccessful(m?.status === 200 && m?.data?.success === true)
- setTimeout(() => {
Review Comment:
old logic can not run actually. setTimeout must write without async and await... Now setTimeout only start when migrations success and it will wait like this:
Do you means you want show loading time = 3s + actually time?
--
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 merged pull request #3260: fix: fix migration dialog only appear after refresh
Posted by GitBox <gi...@apache.org>.
e2corporation merged PR #3260:
URL: https://github.com/apache/incubator-devlake/pull/3260
--
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 #3260: fix: fix migration dialog only appear after refresh
Posted by GitBox <gi...@apache.org>.
likyh commented on code in PR #3260:
URL: https://github.com/apache/incubator-devlake/pull/3260#discussion_r983644249
##########
config-ui/src/hooks/useDatabaseMigrations.jsx:
##########
@@ -36,17 +34,14 @@ function useDatabaseMigrations(Configuration = MigrationOptions) {
const migrate = async () => {
const m = await request.get(Configuration.apiProceedEndpoint)
setWasMigrationSuccessful(m?.status === 200 && m?.data?.success === true)
- setTimeout(() => {
Review Comment:
Just tell me if loading time = 3s + actuallyTime ? Other information I have gotten.
--
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 #3260: fix: fix migration dialog only appear after refresh
Posted by GitBox <gi...@apache.org>.
e2corporation commented on PR #3260:
URL: https://github.com/apache/incubator-devlake/pull/3260#issuecomment-1262321684
@likyh If this new solution is working for now, I'll approve and we can try to fix the `localStorage` method later. It would be ideal to restore the timeout and fix the eslint dependency warning however.
--
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 #3260: fix: fix migration dialog only appear after refresh
Posted by GitBox <gi...@apache.org>.
e2corporation commented on code in PR #3260:
URL: https://github.com/apache/incubator-devlake/pull/3260#discussion_r983559116
##########
config-ui/src/hooks/useDatabaseMigrations.jsx:
##########
@@ -36,17 +34,14 @@ function useDatabaseMigrations(Configuration = MigrationOptions) {
const migrate = async () => {
const m = await request.get(Configuration.apiProceedEndpoint)
setWasMigrationSuccessful(m?.status === 200 && m?.data?.success === true)
- setTimeout(() => {
Review Comment:
The timeout gives the UI at least a 3second window for migration to complete. If migration takes longer than 3 seconds then the delta is still [MIGRATION_TIME] + [3SEC 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 #3260: fix: fix migration dialog only appear after refresh
Posted by GitBox <gi...@apache.org>.
e2corporation commented on code in PR #3260:
URL: https://github.com/apache/incubator-devlake/pull/3260#discussion_r983561158
##########
config-ui/src/hooks/useDatabaseMigrations.jsx:
##########
@@ -36,17 +34,14 @@ function useDatabaseMigrations(Configuration = MigrationOptions) {
const migrate = async () => {
const m = await request.get(Configuration.apiProceedEndpoint)
setWasMigrationSuccessful(m?.status === 200 && m?.data?.success === true)
- setTimeout(() => {
Review Comment:
This timeout has nothing to do with the core problem
--
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 #3260: fix: fix migration dialog only appear after refresh
Posted by GitBox <gi...@apache.org>.
e2corporation commented on code in PR #3260:
URL: https://github.com/apache/incubator-devlake/pull/3260#discussion_r983576542
##########
config-ui/src/hooks/useDatabaseMigrations.jsx:
##########
@@ -36,17 +34,14 @@ function useDatabaseMigrations(Configuration = MigrationOptions) {
const migrate = async () => {
const m = await request.get(Configuration.apiProceedEndpoint)
setWasMigrationSuccessful(m?.status === 200 && m?.data?.success === true)
- setTimeout(() => {
Review Comment:
If you restore this timeout does your new logic break?
--
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 #3260: fix: fix migration dialog only appear after refresh
Posted by GitBox <gi...@apache.org>.
e2corporation commented on PR #3260:
URL: https://github.com/apache/incubator-devlake/pull/3260#issuecomment-1262299806
This ESLint warning should be resolved as well (missing dependency)
```
/home/runner/work/incubator-devlake/incubator-devlake/config-ui/src/hooks/useDatabaseMigrations.jsx
[16](https://github.com/apache/incubator-devlake/actions/runs/3151698589/jobs/5125983010#step:5:17)
86:5 warning React Hook useCallback has a missing dependency: 'migrationWarning'. Either include it or remove the dependency array react-hooks/exhaustive-deps
```
--
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 #3260: fix: fix migration dialog only appear after refresh
Posted by GitBox <gi...@apache.org>.
likyh commented on code in PR #3260:
URL: https://github.com/apache/incubator-devlake/pull/3260#discussion_r983556496
##########
config-ui/src/hooks/useDatabaseMigrations.jsx:
##########
@@ -36,17 +34,14 @@ function useDatabaseMigrations(Configuration = MigrationOptions) {
const migrate = async () => {
const m = await request.get(Configuration.apiProceedEndpoint)
setWasMigrationSuccessful(m?.status === 200 && m?.data?.success === true)
- setTimeout(() => {
Review Comment:
setTimeout must write without async and await...
--
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 #3260: fix: fix migration dialog only appear after refresh
Posted by GitBox <gi...@apache.org>.
e2corporation commented on PR #3260:
URL: https://github.com/apache/incubator-devlake/pull/3260#issuecomment-1262297548
@likyh The reason for a refresh being needed is so the local storage key is picked up. If the user navigates away or reloads the page the warning will happen, for a more instant warning we could just try to use a `history.push` to send the user to /dashboard or some alt route so the migration warning local storage key is detected.
--
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 #3260: fix: fix migration dialog only appear after refresh
Posted by GitBox <gi...@apache.org>.
e2corporation commented on code in PR #3260:
URL: https://github.com/apache/incubator-devlake/pull/3260#discussion_r983595004
##########
config-ui/src/hooks/useDatabaseMigrations.jsx:
##########
@@ -36,17 +34,14 @@ function useDatabaseMigrations(Configuration = MigrationOptions) {
const migrate = async () => {
const m = await request.get(Configuration.apiProceedEndpoint)
setWasMigrationSuccessful(m?.status === 200 && m?.data?.success === true)
- setTimeout(() => {
Review Comment:
That screenshot is after you commented/disabled the timeout or with the timeout enabled?
--
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 #3260: fix: fix migration dialog only appear after refresh
Posted by GitBox <gi...@apache.org>.
e2corporation commented on code in PR #3260:
URL: https://github.com/apache/incubator-devlake/pull/3260#discussion_r983645895
##########
config-ui/src/hooks/useDatabaseMigrations.jsx:
##########
@@ -36,17 +34,14 @@ function useDatabaseMigrations(Configuration = MigrationOptions) {
const migrate = async () => {
const m = await request.get(Configuration.apiProceedEndpoint)
setWasMigrationSuccessful(m?.status === 200 && m?.data?.success === true)
- setTimeout(() => {
Review Comment:
yes 3s + actual time
--
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 #3260: fix: fix migration dialog only appear after refresh
Posted by GitBox <gi...@apache.org>.
likyh commented on code in PR #3260:
URL: https://github.com/apache/incubator-devlake/pull/3260#discussion_r983652104
##########
config-ui/src/hooks/useDatabaseMigrations.jsx:
##########
@@ -36,17 +34,14 @@ function useDatabaseMigrations(Configuration = MigrationOptions) {
const migrate = async () => {
const m = await request.get(Configuration.apiProceedEndpoint)
setWasMigrationSuccessful(m?.status === 200 && m?.data?.success === true)
- setTimeout(() => {
Review Comment:
ok
--
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 #3260: fix: fix migration dialog only appear after refresh
Posted by GitBox <gi...@apache.org>.
e2corporation commented on PR #3260:
URL: https://github.com/apache/incubator-devlake/pull/3260#issuecomment-1262396163
@likyh Also remember to "npm run lint" locally to fix formatting and indentation etc..
--
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