-
-
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?
Changes from all commits
21659b9
e0da397
db951f6
888ab62
7566222
abdc1c2
7956c86
f9ef3c7
2ad1ace
0290062
99b3316
2f71126
c4d694b
509f99a
7cdfeaa
33face0
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
|
|
@@ -3,34 +3,40 @@ | |||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| * Copyright 2023 Taras Shabaranskyi | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| * License AGPL-3.0 or later (http://www.gnu.org/licenses/agpl). */ | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| import {Component} from "@odoo/owl"; | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| import {Component, onMounted, onWillUnmount} from "@odoo/owl"; | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| import {useDebounced} from "@web/core/utils/timing"; | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| import {useService} from "@web/core/utils/hooks"; | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| export function useRefreshAnimation(timeout) { | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| const refreshClass = "o_content__refresh"; | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| let timeoutId = null; | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| /** | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| * @returns {DOMTokenList|null} | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| */ | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| function contentClassList() { | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| const content = document.querySelector(".o_content"); | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| return content ? content.classList : null; | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| } | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| let cachedElement = null; | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| function clearAnimationTimeout() { | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| if (timeoutId) { | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| clearTimeout(timeoutId); | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| } | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| timeoutId = null; | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| cachedElement = null; | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| } | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| function animate() { | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| clearAnimationTimeout(); | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| contentClassList().add(refreshClass); | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| // Cache the element reference before modifying it | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| const content = document.querySelector(".o_content"); | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| if (!content) { | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| return; | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| } | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| cachedElement = content; | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| cachedElement.classList.add(refreshClass); | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| timeoutId = setTimeout(() => { | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| contentClassList().remove(refreshClass); | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| // Only remove class from the same cached element | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| if (cachedElement && cachedElement.classList.contains(refreshClass)) { | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| cachedElement.classList.remove(refreshClass); | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| } | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| clearAnimationTimeout(); | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| }, timeout); | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| } | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
|
@@ -39,13 +45,59 @@ export function useRefreshAnimation(timeout) { | |||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| } | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| export class Refresher extends Component { | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| autoRefreshIntervalKey = "oca.web_refresher.auto_refresh"; | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| refreshDefaultSettingsKey = "default"; | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| setup() { | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| super.setup(); | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| this.action = useService("action"); | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| this.refreshAnimation = useRefreshAnimation(1000); | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| this.onClickRefresh = useDebounced(this.onClickRefresh, 200); | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| this.onChangeAutoRefreshInterval = this.onChangeAutoRefreshInterval.bind(this); | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| this.runningRefresherId = null; | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| onMounted(() => { | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| const intervalValue = this._getLocalStorageValue(window.location.pathname); | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| this.onChangeAutoRefreshInterval({ | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| value: intervalValue.refreshInterval, | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| textContent: intervalValue.intervalText, | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
Comment on lines
+60
to
+61
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| value: intervalValue.refreshInterval, | |
| textContent: intervalValue.intervalText, | |
| target: { | |
| 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.
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; |
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; |
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) { |
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
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); |
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; |
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.
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); |
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.
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -5,13 +5,71 @@ | |
| <template> | ||
| <t t-name="web_refresher.Button"> | ||
| <nav class="oe_refresher" aria-label="Refresher" aria-atomic="true"> | ||
| <button | ||
| class="fa fa-refresh btn btn-icon oe_pager_refresh" | ||
| aria-label="Refresh" | ||
| t-on-click="onClickRefresh" | ||
| title="Refresh" | ||
| tabindex="-1" | ||
| /> | ||
| <div class="btn-group" role="group"> | ||
| <div class="btn-group d-none d-md-block" role="group"> | ||
| <button | ||
| id="auto-refresh-dd" | ||
|
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. added |
||
| class="btn btn-secondary dropdown-toggle" | ||
| type="button" | ||
| data-bs-toggle="dropdown" | ||
| aria-expanded="false" | ||
| > | ||
| Auto Refresh: <span id="auto-refresh-interval-text">Off</span> | ||
| </button> | ||
| <div class="dropdown-menu" aria-labelledby="auto-refresh-dd"> | ||
| <button | ||
| class="dropdown-item" | ||
| t-on-click="onChangeAutoRefreshInterval" | ||
| value="-1" | ||
| >Off</button> | ||
|
Comment on lines
+20
to
+24
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Used |
||
| <button | ||
| class="dropdown-item" | ||
| t-on-click="onChangeAutoRefreshInterval" | ||
| value="1000" | ||
| >1s</button> | ||
|
Comment on lines
+25
to
+29
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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 |
||
| <button | ||
| class="dropdown-item" | ||
| t-on-click="onChangeAutoRefreshInterval" | ||
| value="5000" | ||
| >5s</button> | ||
| <button | ||
| class="dropdown-item" | ||
| t-on-click="onChangeAutoRefreshInterval" | ||
| value="10000" | ||
| >10s</button> | ||
| <button | ||
| class="dropdown-item" | ||
| t-on-click="onChangeAutoRefreshInterval" | ||
| value="30000" | ||
| >30s</button> | ||
| <button | ||
| class="dropdown-item" | ||
| t-on-click="onChangeAutoRefreshInterval" | ||
| value="60000" | ||
| >1min</button> | ||
| </div> | ||
| </div> | ||
| <button | ||
| id="set-as-default-btn" | ||
| class="btn btn-secondary m-0 d-none d-md-block" | ||
| aria-label="Set as Default" | ||
| t-on-click="setRefreshAsDefault" | ||
| title="Set as Default" | ||
| tabindex="-1" | ||
| > | ||
| <i id="set-as-default-icon" class="fa" /> | ||
| </button> | ||
| <button | ||
| id="manual-refresh-btn" | ||
| class="btn btn-secondary m-0" | ||
| aria-label="Refresh" | ||
| t-on-click="onClickRefresh" | ||
|
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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 |
||
| title="Refresh" | ||
| tabindex="-1" | ||
| > | ||
| <i id="manual-refresh-icon" class="fa fa-refresh" /> | ||
|
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Moved the icons to an
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. added |
||
| </button> | ||
| </div> | ||
| </nav> | ||
| </t> | ||
| </template> | ||
Uh oh!
There was an error while loading. Please reload this page.