refactor(ui): auto refresh button #1105

shhdgit · 2021-12-22T07:45:41Z

Related PR: #1099

ti-chi-bot · 2021-12-22T07:45:42Z

[REVIEW NOTIFICATION]

This pull request has been approved by:

baurine

To complete the pull request process, please ask the reviewers in the list to review by filling /cc @reviewer in the comment.
After your PR has acquired the required number of LGTMs, you can assign this pull request to the committer in the list by filling /assign @committer in the comment to help you merge this pull request.

The full list of commands accepted by this bot can be found here.

Reviewer can indicate their review by submitting an approval review.
Reviewer can cancel approval by submitting a request changes review.

shhdgit · 2021-12-22T07:48:21Z

/cc @baurine @breeswish
PTAL, thanks!

ui/lib/components/AutoRefreshButton/AutoRefreshButton.tsx

breezewish · 2021-12-22T16:32:43Z

ui/lib/components/AutoRefreshButton/AutoRefreshButton.tsx

+      }, 1000) as unknown as number
+    )
+    return () => clearTimeout(timer)
+  }, [autoRefreshSeconds, isLoading, getRemainingRefreshSeconds()])


The CI is passing without complaining about missing deps.. It doesn't look right. Could you take a look? @baurine

OK, let me check it.

ui/lib/components/AutoRefreshButton/AutoRefreshButton.tsx

breezewish

I think a correct behavior of your component should be:

When auto refresh is on, a tick starts.
At any time it is set to disabled, the tick paused.
At any time it is set to enabled, the tick continues.
When auto refresh settings is changed, the tick skips if the new refresh interval is set to a value smaller than remaining ticks.
When auto refresh is off, the tick stops.

The loading behavior can be naturally expressed with the disabled property, as you should not allow user to click refresh button again when there is a request on-going. In this case, you will want to set the disabled when using this component. This also applies to the auto refresh: there is no need for the tick to continue if there is a request on-going, since the last request is not finished yet. This means, disabled should be applied to both manual refresh and auto refresh. One property for all, which is great.

ui/lib/apps/KeyViz/components/KeyViz.tsx

breezewish · 2021-12-27T05:48:03Z

ui/lib/components/AutoRefreshButton/AutoRefreshButton.tsx

+    ) {
+      setRemainingRefreshSeconds(autoRefreshSeconds)
+    }
+  }, [autoRefreshSeconds])


the deps check is not working @baurine PTAL~

codecov-commenter · 2021-12-27T09:27:18Z

Codecov Report

❗ No coverage uploaded for pull request base (master@84fb1f0). Click here to learn what that means.
The diff coverage is n/a.

@@            Coverage Diff            @@
##             master    #1105   +/-   ##
=========================================
  Coverage          ?   23.12%           
=========================================
  Files             ?      102           
  Lines             ?     9981           
  Branches          ?        0           
=========================================
  Hits              ?     2308           
  Misses            ?     7523           
  Partials          ?      150

Flag	Coverage Δ
be_integration_test_latest	`7.05% <0.00%> (?)`
be_integration_test_v4.0.1	`7.05% <0.00%> (?)`
be_unit_test	`24.13% <0.00%> (?)`

Flags with carried forward coverage won't be shown. Click here to find out more.

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 84fb1f0...874a57b. Read the comment docs.

breezewish · 2021-12-27T13:48:25Z

ui/lib/apps/KeyViz/components/KeyViz.tsx

+  const onChangeMetricType = useCallback(
+    (v: DataTag) => {
+      setMetricType(v)
+      resetAutoRefresh()


I believe it is intentional that the there is no need to reset the refresh when metric type is changed, since there will be no data fetching operations during this.

It looks better to recover the remaining seconds. And there's data fetching logic caused by metric type at L195-L200. Is this as expected? PTAL.

ui/lib/apps/KeyViz/components/KeyViz.tsx

ui/lib/apps/KeyViz/components/KeyVizToolbar.tsx

ui/lib/components/AutoRefreshButton/AutoRefreshButton.tsx

baurine · 2021-12-28T02:26:45Z

ui/lib/components/AutoRefreshButton/AutoRefreshButton.tsx

+      }
+    }, 1000)
+    return () => clearTimeout(timer.current!)
+  }, [autoRefreshSeconds, disabled, getRemainingRefreshSeconds()])


I feel this kind of way to implement the timer is a little hacky, and not accurate. There is a gap between the effects, maybe use interval is better.

Yes, it's not accurate and it's on purpose. This may be necessary if we want to pause or resume timer instantly. Any good ideas?

… auto-refresh

shhdgit · 2021-12-28T15:31:37Z

/merge

ti-chi-bot · 2021-12-28T15:31:40Z

This pull request has been accepted and is ready to merge.

Commit hash: 874a57b

* refactor(ui): auto refresh button * chore: update translation * fix: remain seconds * refine: refresh button * fix: onRefresh * fix: auto refresh * fix: continue tick * chore: add some comments * tweak: remaining refresh seconds * chore: clean code Co-authored-by: Wenxuan <breezewish@pingcap.com>

* Revert "Release v2021.12.06.1 (#1084)" This reverts commit bcc43a0. * Compitable with different TiDB versions for conprof and non-root-login features (#1047) * make conprof independent * check feature enable * add check feature enable middleware * hide menu if feature is not enabled * refactor non root login switch by new design * i18n * yarn fmt * renaming * adjust fe code * refine * remove unused log * build(deps): bump ws from 5.2.2 to 5.2.3 in /ui (#1055) * CICD: Update the release pipeline for recent PD format policies (#1054) * fix i18n wording (#1056) * Refactor: Change util module to util package (#1052) * Refactor: Fix godot incorrectly add dot suffix to annotations (#1059) * lint: Add goheader for copyright lints (#1062) * Refactor: Migrate to use the `rest` package in util/ (#1060) * fix(*): globally delete/update data by GORM (#1065) * ui: bump dependencies (#1066) * refactor: Switch to use ziputil, netutil, reflectutil and fileswap (#1067) * Fix request header being pinned after pd profiling (#1069) * Integrate speedscope (#1064) * fix potential panic when GetPDInstances (#1075) Signed-off-by: crazycs <chen.two.cs@gmail.com> * Refactor: a new httpclient (#1073) * Refactor: Switch to use util/distro in all places (#1078) * chore: support import relative file URL (#1082) * Refactor: Move tools into a standalone module (#1079) * Fix script to embed the ui (#1088) * Fix script to embed the ui * Hack write_strings * Refactor feature flag to support more modules (#1057) * Drop sysutil dependency (#1093) * chore: add graph generation (#1085) * Refactor: Add TopologyProvider (#1098) * esbuild: i18n + dep (#1101) * script: Add a script to generate version matrix (#1104) * distro: support dynamic config (#1094) * chore: support multiple profiling types (#1095) * fix(distro): check distro_strings.json fmt by prettier (#1106) * script: fix generate assets (#1107) * Add integration test (#1083) * debug_api: Switch to use the new util (#1103) * refactor(ui): auto refresh button (#1105) * refactor(ui): auto refresh button * chore: update translation * fix: remain seconds * refine: refresh button * fix: onRefresh * fix: auto refresh * fix: continue tick * chore: add some comments * tweak: remaining refresh seconds * chore: clean code Co-authored-by: Wenxuan <breezewish@pingcap.com> * ui: refine conprof (#1102) * update wording * not check prom any more * replace time range component * i18n * support view profile by diffrent ways * extract ActionsButton * change download data format * refine * comments * Revert "comments" This reverts commit 3b03fdb. * fix view cpu profile fail * update state * hide action button if disable * address feedback * update release-version * sync with master Co-authored-by: dependabot[bot] <49699333+dependabot[bot]@users.noreply.github.com> Co-authored-by: Wenxuan <breezewish@pingcap.com> Co-authored-by: Suhaha <jklopsdfw@gmail.com> Co-authored-by: Yini Xu <34967660+YiniXu9506@users.noreply.github.com> Co-authored-by: crazycs <crazycs520@gmail.com>

refactor(ui): auto refresh button

9938b30

shhdgit force-pushed the auto-refresh branch from 3c48d16 to 9938b30 Compare December 22, 2021 07:47

ti-chi-bot requested review from baurine and breezewish December 22, 2021 07:48

shhdgit mentioned this pull request Dec 22, 2021

Feature Top SQL #1099

Merged

3 tasks

shhdgit added 2 commits December 22, 2021 15:51

chore: update translation

1879323

fix: remain seconds

fbfc336

breezewish reviewed Dec 22, 2021

View reviewed changes

ui/lib/components/AutoRefreshButton/AutoRefreshButton.tsx Outdated Show resolved Hide resolved

breezewish reviewed Dec 22, 2021

View reviewed changes

ui/lib/components/AutoRefreshButton/AutoRefreshButton.tsx Outdated Show resolved Hide resolved

breezewish reviewed Dec 22, 2021

View reviewed changes

ui/lib/components/AutoRefreshButton/AutoRefreshButton.tsx Outdated Show resolved Hide resolved

breezewish reviewed Dec 22, 2021

View reviewed changes

ui/lib/components/AutoRefreshButton/AutoRefreshButton.tsx Outdated Show resolved Hide resolved

breezewish reviewed Dec 22, 2021

View reviewed changes

ui/lib/components/AutoRefreshButton/AutoRefreshButton.tsx Outdated Show resolved Hide resolved

breezewish reviewed Dec 22, 2021

View reviewed changes

ui/lib/components/AutoRefreshButton/AutoRefreshButton.tsx Outdated Show resolved Hide resolved

breezewish reviewed Dec 22, 2021

View reviewed changes

ui/lib/components/AutoRefreshButton/AutoRefreshButton.tsx Outdated Show resolved Hide resolved

breezewish reviewed Dec 22, 2021

View reviewed changes

ui/lib/components/AutoRefreshButton/AutoRefreshButton.tsx Outdated Show resolved Hide resolved

breezewish reviewed Dec 22, 2021

View reviewed changes

ui/lib/components/AutoRefreshButton/AutoRefreshButton.tsx Outdated Show resolved Hide resolved

breezewish reviewed Dec 22, 2021

View reviewed changes

ui/lib/components/AutoRefreshButton/AutoRefreshButton.tsx Outdated Show resolved Hide resolved

breezewish reviewed Dec 22, 2021

View reviewed changes

ui/lib/components/AutoRefreshButton/AutoRefreshButton.tsx Outdated Show resolved Hide resolved

breezewish reviewed Dec 22, 2021

View reviewed changes

ui/lib/components/AutoRefreshButton/AutoRefreshButton.tsx Outdated Show resolved Hide resolved

refine: refresh button

996deeb

breezewish reviewed Dec 23, 2021

View reviewed changes

ui/lib/components/AutoRefreshButton/AutoRefreshButton.tsx Outdated Show resolved Hide resolved

breezewish reviewed Dec 23, 2021

View reviewed changes

ui/lib/components/AutoRefreshButton/AutoRefreshButton.tsx Outdated Show resolved Hide resolved

breezewish reviewed Dec 23, 2021

View reviewed changes

ui/lib/components/AutoRefreshButton/AutoRefreshButton.tsx Outdated Show resolved Hide resolved

breezewish reviewed Dec 23, 2021

View reviewed changes

shhdgit added 2 commits December 23, 2021 16:35

fix: onRefresh

aad7811

fix: auto refresh

2ad6cf4

shhdgit requested a review from breezewish December 23, 2021 09:16

fix: continue tick

a16eced

shhdgit force-pushed the auto-refresh branch from bfc997b to a16eced Compare December 23, 2021 09:17

chore: add some comments

bec2230

shhdgit requested a review from YiniXu9506 December 27, 2021 03:53

baurine approved these changes Dec 27, 2021

View reviewed changes

ti-chi-bot added the status/LGT1 label Dec 27, 2021

breezewish reviewed Dec 27, 2021

View reviewed changes

Merge branch 'master' into auto-refresh

4098c4d

breezewish reviewed Dec 27, 2021

View reviewed changes

ui/lib/apps/KeyViz/components/KeyViz.tsx Outdated Show resolved Hide resolved

breezewish reviewed Dec 27, 2021

View reviewed changes

ui/lib/apps/KeyViz/components/KeyVizToolbar.tsx Outdated Show resolved Hide resolved

breezewish reviewed Dec 27, 2021

View reviewed changes

ui/lib/components/AutoRefreshButton/AutoRefreshButton.tsx Outdated Show resolved Hide resolved

baurine reviewed Dec 28, 2021

View reviewed changes

shhdgit added 3 commits December 28, 2021 12:06

tweak: remaining refresh seconds

9530d81

Merge branch 'auto-refresh' of github.com:shhdgit/tidb-dashboard into…

0cbdd0b

… auto-refresh

chore: clean code

803f117

breezewish approved these changes Dec 28, 2021

View reviewed changes

Merge branch 'master' into auto-refresh

874a57b

ti-chi-bot added the status/can-merge label Dec 28, 2021

shhdgit merged commit df625e0 into pingcap:master Dec 28, 2021

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

refactor(ui): auto refresh button #1105

refactor(ui): auto refresh button #1105

shhdgit commented Dec 22, 2021 •

edited

Loading

ti-chi-bot commented Dec 22, 2021 •

edited

Loading

shhdgit commented Dec 22, 2021

breezewish Dec 22, 2021

baurine Dec 23, 2021

breezewish left a comment •

edited

Loading

breezewish Dec 27, 2021

codecov-commenter commented Dec 27, 2021 •

edited

Loading

breezewish Dec 27, 2021

shhdgit Dec 28, 2021

baurine Dec 28, 2021

shhdgit Dec 28, 2021 •

edited

Loading

shhdgit commented Dec 28, 2021

ti-chi-bot commented Dec 28, 2021

refactor(ui): auto refresh button #1105

refactor(ui): auto refresh button #1105

Conversation

shhdgit commented Dec 22, 2021 • edited Loading

ti-chi-bot commented Dec 22, 2021 • edited Loading

shhdgit commented Dec 22, 2021

breezewish Dec 22, 2021

Choose a reason for hiding this comment

baurine Dec 23, 2021

Choose a reason for hiding this comment

breezewish left a comment • edited Loading

Choose a reason for hiding this comment

breezewish Dec 27, 2021

Choose a reason for hiding this comment

codecov-commenter commented Dec 27, 2021 • edited Loading

Codecov Report

breezewish Dec 27, 2021

Choose a reason for hiding this comment

shhdgit Dec 28, 2021

Choose a reason for hiding this comment

baurine Dec 28, 2021

Choose a reason for hiding this comment

shhdgit Dec 28, 2021 • edited Loading

Choose a reason for hiding this comment

shhdgit commented Dec 28, 2021

ti-chi-bot commented Dec 28, 2021

shhdgit commented Dec 22, 2021 •

edited

Loading

ti-chi-bot commented Dec 22, 2021 •

edited

Loading

breezewish left a comment •

edited

Loading

codecov-commenter commented Dec 27, 2021 •

edited

Loading

shhdgit Dec 28, 2021 •

edited

Loading