-
Notifications
You must be signed in to change notification settings - Fork 26
refactor DeletedMessageGroups view data handling into a store #2692
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: monitoring_store
Are you sure you want to change the base?
Conversation
# Conflicts: # src/Frontend/src/stores/MessageStore.ts
| } | ||
| watch(autoRefreshValue, (newValue) => { | ||
| updateInterval(newValue || 0); |
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.
Given that you modified updateInterval to do the pause and resume as part of the internals, I don't think we need the if/else statement at all here.
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.
without the if statement, there would be nothing to call start when going from 0 to >0. This has made me realise, however, that the updateInterval will potentially resume before start is ever called
src/Frontend/src/components/failedmessages/DeletedMessageGroups.vue
Outdated
Show resolved
Hide resolved
| constructor(store?: ServiceControlStore) { | ||
| //this module is only called from within view setup or other pinia stores, so this call is lifecycle safe | ||
| this.serviceControlStore = useServiceControlStore(); | ||
| this.serviceControlStore = store ?? useServiceControlStore(); |
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.
Given useServiceControlStore() is a singleton, do we need to pass it in?
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.
it's not a singleton, but pinia ensures that the same store is injected for each call. This seems safer, given we don't control where in the lifecycle this class is constructed
| watch(isRefreshing, () => { | ||
| // If we're currently polling at 5 seconds and there is a restore in progress, then change the polling interval to poll every 1 second | ||
| if (!pollingFaster && isRestoreInProgress()) { | ||
| pollingFaster = true; | ||
| updateInterval(1000); | ||
| } else if (pollingFaster && !isRestoreInProgress()) { | ||
| // if we're currently polling every 1 second and all restores are done, change polling frequency back to every 5 seconds | ||
| pollingFaster = false; | ||
| updateInterval(5000); | ||
| } | ||
| }); |
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.
Something feels off here. Why are we updating the interval on the isRefreshing watch?
The interval is already updated when we call restoreGroup.
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.
So I decided to rewrite this, see #2717
part of #1905, extending #2675 to remove direct SC calls from views