-
-
Notifications
You must be signed in to change notification settings - Fork 2.1k
[18.0][IMP] web_refresher: add auto refresh on interval #3256
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: 18.0
Are you sure you want to change the base?
Conversation
f179836 to
620e3d7
Compare
1c710d5 to
9dde2b4
Compare
| title="Refresh" | ||
| tabindex="-1" | ||
| > | ||
| <i id="manual-refresh-icon" class="fa fa-refresh" /> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Maintain existing flow with the a reset of the new timeout when manually clicked
| <button | ||
| class="dropdown-item" | ||
| t-on-click="onChangeAutoRefreshInterval" | ||
| value="1000" | ||
| >1s</button> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm not sure 1s is really appropriate. I could easily be persuaded to remove 1s
| <button | ||
| class="dropdown-item" | ||
| t-on-click="onChangeAutoRefreshInterval" | ||
| value="-1" | ||
| >Off</button> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Default text could be moved out to allow easier i18n
| title="Refresh" | ||
| tabindex="-1" | ||
| > | ||
| <i id="manual-refresh-icon" class="fa fa-refresh" /> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
added id for easy selections
| } else { | ||
| intervalValue = `${intervalInSeconds}s`; | ||
| } | ||
| document.getElementById("manual-refresh-icon").classList.add("fa-spin"); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
add fa-spin to show active auto refresh
| } else { | ||
| const intervalInSeconds = this.refreshInterval / 1000; | ||
| if (intervalInSeconds >= 60) { | ||
| intervalValue = `${Math.floor(intervalInSeconds / 60)}min`; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
60s or more gets "min" added
| 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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Reviewing this logic i don't love how it played out. I'm going to refactor
| // 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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Consolidated logic for starting the refresh interval in the current refresh to allow recursive calls
| const newIntervalText = | ||
| clickedOption.textContent ?? clickedOption.target.textContent; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Use the xml button text as the interval on the label
803e87d to
e746c50
Compare
f922248 to
c1f6881
Compare
c1f6881 to
935ca4d
Compare
74e0f06 to
33face0
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Pull request overview
This PR adds an auto-refresh feature to the web_refresher module, allowing users to set automatic page refresh intervals. The implementation includes a new dropdown UI for selecting refresh intervals (1s, 5s, 10s, 30s, 1min, or Off), the ability to set a default refresh interval stored in localStorage, and visual indicators showing the refresh status.
Changes:
- Added auto-refresh interval dropdown with preset options in the UI template
- Implemented localStorage-based persistence for refresh settings per page and default settings
- Added visual feedback with spinning refresh icon and star icon for default setting indication
Reviewed changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated 10 comments.
| File | Description |
|---|---|
| web_refresher/static/src/xml/refresher.xml | Replaced simple refresh button with button group containing auto-refresh dropdown, default setting button, and manual refresh button |
| web_refresher/static/src/js/refresher.esm.js | Added auto-refresh logic with localStorage persistence, timer management, and UI update methods |
| web_refresher/manifest.py | Updated version number from 18.0.1.0.0 to 18.0.2.0.0 |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| returnInterval = parseInt(refreshInterval ?? -1); | ||
| returnText = intervalText; | ||
| } else if (Object.hasOwn(refreshSettings, this.refreshDefaultSettingsKey)) { | ||
| const {refreshInterval = -1, intervalText = "Off"} = | ||
| refreshSettings[this.refreshDefaultSettingsKey]; | ||
| returnInterval = parseInt(refreshInterval ?? -1); |
Copilot
AI
Jan 16, 2026
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The nullish coalescing operator is redundant here since refreshInterval already has a default value of -1 from destructuring on line 80. Additionally, parseInt on an already-numeric value is unnecessary if refreshInterval is stored as a number. If it's stored as a string in localStorage, ensure consistent parsing is applied.
| returnInterval = parseInt(refreshInterval ?? -1); | |
| returnText = intervalText; | |
| } else if (Object.hasOwn(refreshSettings, this.refreshDefaultSettingsKey)) { | |
| const {refreshInterval = -1, intervalText = "Off"} = | |
| refreshSettings[this.refreshDefaultSettingsKey]; | |
| returnInterval = parseInt(refreshInterval ?? -1); | |
| returnInterval = | |
| typeof refreshInterval === "string" | |
| ? parseInt(refreshInterval, 10) | |
| : refreshInterval; | |
| returnText = intervalText; | |
| } else if (Object.hasOwn(refreshSettings, this.refreshDefaultSettingsKey)) { | |
| const {refreshInterval = -1, intervalText = "Off"} = | |
| refreshSettings[this.refreshDefaultSettingsKey]; | |
| returnInterval = | |
| typeof refreshInterval === "string" | |
| ? parseInt(refreshInterval, 10) | |
| : refreshInterval; |
| returnInterval = parseInt(refreshInterval ?? -1); | ||
| returnText = intervalText; | ||
| } else if (Object.hasOwn(refreshSettings, this.refreshDefaultSettingsKey)) { | ||
| const {refreshInterval = -1, intervalText = "Off"} = | ||
| refreshSettings[this.refreshDefaultSettingsKey]; | ||
| returnInterval = parseInt(refreshInterval ?? -1); |
Copilot
AI
Jan 16, 2026
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The nullish coalescing operator is redundant here since refreshInterval already has a default value of -1 from destructuring on line 85. Additionally, parseInt on an already-numeric value is unnecessary if refreshInterval is stored as a number. If it's stored as a string in localStorage, ensure consistent parsing is applied.
| returnInterval = parseInt(refreshInterval ?? -1); | |
| returnText = intervalText; | |
| } else if (Object.hasOwn(refreshSettings, this.refreshDefaultSettingsKey)) { | |
| const {refreshInterval = -1, intervalText = "Off"} = | |
| refreshSettings[this.refreshDefaultSettingsKey]; | |
| returnInterval = parseInt(refreshInterval ?? -1); | |
| returnInterval = | |
| typeof refreshInterval === "string" | |
| ? parseInt(refreshInterval, 10) | |
| : refreshInterval; | |
| returnText = intervalText; | |
| } else if (Object.hasOwn(refreshSettings, this.refreshDefaultSettingsKey)) { | |
| const {refreshInterval = -1, intervalText = "Off"} = | |
| refreshSettings[this.refreshDefaultSettingsKey]; | |
| returnInterval = | |
| typeof refreshInterval === "string" | |
| ? parseInt(refreshInterval, 10) | |
| : refreshInterval; |
| value: intervalValue.refreshInterval, | ||
| textContent: intervalValue.intervalText, |
Copilot
AI
Jan 16, 2026
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Calling onChangeAutoRefreshInterval in onMounted with an object that doesn't match the event structure could cause issues. The method expects either clickedOption.value/clickedOption.target.value and clickedOption.textContent/clickedOption.target.textContent. This works because of the nullish coalescing fallbacks, but creates inconsistent API usage. Consider creating a separate initialization method or documenting this dual usage pattern.
| value: intervalValue.refreshInterval, | |
| textContent: intervalValue.intervalText, | |
| target: { | |
| value: intervalValue.refreshInterval, | |
| textContent: intervalValue.intervalText, | |
| }, |
|
|
||
| onChangeAutoRefreshInterval(clickedOption) { | ||
| const newInterval = | ||
| parseInt(clickedOption.value ?? clickedOption.target.value) ?? -1; |
Copilot
AI
Jan 16, 2026
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If both clickedOption.value and clickedOption.target.value are undefined, parseInt(undefined) returns NaN, and the second nullish coalescing won't trigger because NaN is not null/undefined. This should use logical OR (||) or check for NaN explicitly: parseInt(clickedOption.value ?? clickedOption.target.value, 10) || -1.
| parseInt(clickedOption.value ?? clickedOption.target.value) ?? -1; | |
| parseInt(clickedOption.value ?? clickedOption.target.value, 10) || -1; |
| this._setLocalStorageValue(this.refreshDefaultSettingsKey, { | ||
| refreshInterval: this.refreshInterval, | ||
| intervalText: document.getElementById("auto-refresh-interval-text") | ||
| .textContent, | ||
| }); | ||
| this._setIntervalUi( | ||
| document.getElementById("auto-refresh-interval-text").textContent | ||
| ); |
Copilot
AI
Jan 16, 2026
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This DOM query is performed twice in setRefreshAsDefault (lines 197-198 and 201). Consider storing the element or its textContent in a variable to avoid redundant DOM queries.
| this._setLocalStorageValue(this.refreshDefaultSettingsKey, { | |
| refreshInterval: this.refreshInterval, | |
| intervalText: document.getElementById("auto-refresh-interval-text") | |
| .textContent, | |
| }); | |
| this._setIntervalUi( | |
| document.getElementById("auto-refresh-interval-text").textContent | |
| ); | |
| const intervalElement = document.getElementById("auto-refresh-interval-text"); | |
| const intervalText = intervalElement ? intervalElement.textContent : ""; | |
| this._setLocalStorageValue(this.refreshDefaultSettingsKey, { | |
| refreshInterval: this.refreshInterval, | |
| intervalText: intervalText, | |
| }); | |
| this._setIntervalUi(intervalText); |
| const manualRefreshIcon = document.getElementById("manual-refresh-icon"); | ||
| if (manualRefreshIcon) { | ||
| if (!this.refreshInterval || this.refreshInterval <= 0) { | ||
| manualRefreshIcon.classList.remove("fa-spin"); | ||
| } else { | ||
| manualRefreshIcon.classList.add("fa-spin"); | ||
| } | ||
| } |
Copilot
AI
Jan 16, 2026
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Direct DOM manipulation using getElementById is not idiomatic in OWL components. Consider using t-ref in the template and useRef hook to access DOM elements, or manage the spinning state through reactive state that updates the template.
| // 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") { |
Copilot
AI
Jan 16, 2026
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The type check typeof this.runningRefresherId === 'number' is unnecessarily strict. Since clearTimeout safely handles null, undefined, and 0, a simple truthy check would suffice: if (this.runningRefresherId). This pattern is also used on line 212.
| if (typeof this.runningRefresherId === "number") { | |
| if (this.runningRefresherId) { |
| this.refreshAnimation = useRefreshAnimation(1000); | ||
| this.onClickRefresh = useDebounced(this.onClickRefresh, 200); | ||
| this.onChangeAutoRefreshInterval = this.onChangeAutoRefreshInterval.bind(this); | ||
| this.runningRefresherId = null; |
Copilot
AI
Jan 16, 2026
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The property runningRefresherId is initialized but not defined as a class field. Consider declaring it at the class level for clarity: runningRefresherId = null; before the setup() method, similar to autoRefreshIntervalKey and refreshDefaultSettingsKey.
| parseInt(clickedOption.value ?? clickedOption.target.value) ?? -1; | ||
| const newIntervalText = | ||
| clickedOption.textContent ?? clickedOption.target.textContent ?? "Off"; | ||
| this.refreshInterval = newInterval; |
Copilot
AI
Jan 16, 2026
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The property refreshInterval is assigned but never declared as a class field. Consider declaring it at the class level for clarity, similar to other class properties.
| this.refreshInterval = newInterval; | ||
| this._setIntervalUi(newIntervalText); | ||
| if (this.runningRefresherId) { | ||
| clearTimeout(this.runningRefresherId); | ||
| } |
Copilot
AI
Jan 16, 2026
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Calling refresh() immediately when changing the auto-refresh interval triggers an immediate refresh, which is correct. However, if auto-refresh is turned off (value === -1), calling refresh() will not schedule another refresh (line 137 checks > 0), but the running timer isn't cleared until line 212-214, creating a potential race condition where the previous timer might still fire. The clearTimeout should happen before checking conditions in the refresh() method or the order should be reconsidered.
| this.refreshInterval = newInterval; | |
| this._setIntervalUi(newIntervalText); | |
| if (this.runningRefresherId) { | |
| clearTimeout(this.runningRefresherId); | |
| } | |
| if (this.runningRefresherId) { | |
| clearTimeout(this.runningRefresherId); | |
| this.runningRefresherId = null; | |
| } | |
| this.refreshInterval = newInterval; | |
| this._setIntervalUi(newIntervalText); |
|
Hi @O-Mutt , thank you for your work on this PR. I wanted to suggest taking a look at this other PR #3396. Although it targets version 17.0, it proposes an interesting approach using the Odoo Bus to update the view reactively (listening to create/write events) instead of using an automatic refresh interval (polling). |
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