[18.0][IMP] web_refresher: add auto refresh on interval #3256
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
O-Mutt
force-pushed
the
feat/autoRefresh
branch
3 times, most recently
from
f179836 to
620e3d7
Compare
O-Mutt
changed the title
O-Mutt
changed the title
O-Mutt
force-pushed
the
feat/autoRefresh
branch
2 times, most recently
from
1c710d5 to
9dde2b4
Compare
O-Mutt
commented
Aug 21, 2025
| title="Refresh" | ||
| tabindex="-1" | ||
| > | ||
| <i id="manual-refresh-icon" class="fa fa-refresh" /> |
There was a problem hiding this comment.
Moved the icons to an <i> to allow the fa-spin to be added and removed without spinning the whole button 🤣
| id="manual-refresh-btn" | ||
| class="btn btn-secondary m-0" | ||
| aria-label="Refresh" | ||
| t-on-click="onClickRefresh" |
There was a problem hiding this comment.
Maintain existing flow with the a reset of the new timeout when manually clicked
Comment on lines
+25
to
+29
| <button | ||
| class="dropdown-item" | ||
| t-on-click="onChangeAutoRefreshInterval" | ||
| value="1000" | ||
| >1s</button> |
There was a problem hiding this comment.
I'm not sure 1s is really appropriate. I could easily be persuaded to remove 1s
Comment on lines
+20
to
+24
| <button | ||
| class="dropdown-item" | ||
| t-on-click="onChangeAutoRefreshInterval" | ||
| value="-1" | ||
| >Off</button> |
There was a problem hiding this comment.
Used -1 as the "off" value to avoid null/undefined confusions. #javascriptthings
| data-bs-toggle="dropdown" | ||
| aria-expanded="false" | ||
| > | ||
| Auto Refresh: Off |
There was a problem hiding this comment.
Default text could be moved out to allow easier i18n
| title="Refresh" | ||
| tabindex="-1" | ||
| > | ||
| <i id="manual-refresh-icon" class="fa fa-refresh" /> |
| } else { | ||
| intervalValue = `${intervalInSeconds}s`; | ||
| } | ||
| document.getElementById("manual-refresh-icon").classList.add("fa-spin"); |
There was a problem hiding this comment.
add fa-spin to show active auto refresh
| } else { | ||
| const intervalInSeconds = this.refreshInterval / 1000; | ||
| if (intervalInSeconds >= 60) { | ||
| intervalValue = `${Math.floor(intervalInSeconds / 60)}min`; |
Comment on lines
140
to
243
| let intervalValue = "Off"; | ||
| if (!this.refreshInterval || this.refreshInterval <= 0) { | ||
| intervalValue = "Off"; | ||
| document.getElementById("manual-refresh-icon").classList.remove("fa-spin"); | ||
| } else { | ||
| const intervalInSeconds = this.refreshInterval / 1000; | ||
| if (intervalInSeconds >= 60) { | ||
| intervalValue = `${Math.floor(intervalInSeconds / 60)}min`; | ||
| } else { | ||
| intervalValue = `${intervalInSeconds}s`; | ||
| } | ||
| document.getElementById("manual-refresh-icon").classList.add("fa-spin"); | ||
| } | ||
| document.getElementById("auto-refresh-dd").textContent = | ||
| `Auto Refresh: ${intervalValue}`; | ||
| } |
There was a problem hiding this comment.
Reviewing this logic i don't love how it played out. I'm going to refactor
Comment on lines
+87
to
+139
| // Check the refreshInterval is greater than 0 and start a timer for the next refresh | ||
| if (this.refreshInterval > 0) { | ||
| // Always attempt to clear a running timeout in case the refresh was done manually | ||
| if (typeof this.runningRefresherId === "number") { | ||
| clearTimeout(this.runningRefresherId); | ||
| } | ||
| this.runningRefresherId = setTimeout(() => { | ||
| this.refresh(); | ||
| }, this.refreshInterval); | ||
| } |
There was a problem hiding this comment.
Consolidated logic for starting the refresh interval in the current refresh to allow recursive calls
O-Mutt
commented
Aug 21, 2025
Comment on lines
131
to
157
| const newIntervalText = | ||
| clickedOption.textContent ?? clickedOption.target.textContent; |
There was a problem hiding this comment.
Use the xml button text as the interval on the label
O-Mutt
commented
Aug 22, 2025
O-Mutt
commented
Aug 22, 2025
O-Mutt
force-pushed
the
feat/autoRefresh
branch
from
803e87d to
e746c50
Compare
StefanRijnhart
changed the title
O-Mutt
force-pushed
the
feat/autoRefresh
branch
from
f922248 to
c1f6881
Compare
O-Mutt
force-pushed
the
feat/autoRefresh
branch
from
c1f6881 to
935ca4d
Compare
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
This adds a new "auto" refresh option to the UI for allowing an interval refresh. I have also implemented a setting that is stored in localStorage for the auto refresh interval. When the auto refresh is set it puts the value into localStorage to be re-used on each screen by default.
Here is the auto refresh in action with network traffic included:
Screen.Recording.2025-08-21.at.1.36.27.PM.mov
UI of baseline
UI w/hover
UI Clicked dropdown
UI showing animation of manual refresh button to indicate ongoing action