Stop the dashboard UI from stutter by checking the build sha #2953
Conversation
|
|
| @@ -3,5 +3,5 @@ import './main.js'; | |||
|
|
|||
| window.addEventListener('load', () => { | |||
| checkForBuild(); | |||
| setInterval(checkForBuild, 5000); | |||
| setInterval(checkForBuild, 30000); | |||
There was a problem hiding this comment.
This can probably be every 10s, since you'll ignore it if the build hasn't changed.
| @@ -3,11 +3,19 @@ import terminal from './terminal.js'; | |||
| import buildHeader from './build-header.js'; | |||
| import showToast from '../utils/toast.js'; | |||
|
|
|||
| let build; | |||
There was a problem hiding this comment.
Should we put this variable inside the function checkBuildStatus()?
There was a problem hiding this comment.
Putting it here is ok, we want to have a variable here containing the build being shown in the build logs. This is compared against upcoming builds, if the same, skip writing a new log to the terminal.
There was a problem hiding this comment.
I mean the best practice is to avoid global variable @humphd can I have your thought on this ?
There was a problem hiding this comment.
It's a module-level variable vs. global, so it's not terrible. If it really bothered you, you could wrap this data + your function below in a class, so they go together. But this is no worse than showToast, which is also being defined at the module level.
In general I agree with you: fewer variables, reduce their scope. But this is hard to avoid completely.
| @@ -3,11 +3,19 @@ import terminal from './terminal.js'; | |||
| import buildHeader from './build-header.js'; | |||
| import showToast from '../utils/toast.js'; | |||
|
|
|||
| let build; | |||
There was a problem hiding this comment.
Putting it here is ok, we want to have a variable here containing the build being shown in the build logs. This is compared against upcoming builds, if the same, skip writing a new log to the terminal.
TueeNguyen
dismissed stale reviews from DukeManh, humphd, and themself
via
e938990
Issue This PR Addresses
Fix #2773
Type of Change
Description
Checking the
shaof the build and stop re-rendering if we have the sameshadisplayed in thebuild logand thebuild headeralreadySteps to test the PR
Checklist