Skip to content
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.

Sign up for GitHub

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

Jump to bottom

Feat: Full server-side pagination for important events #3515

Merged
merged 8 commits into from
Sep 23, 2023
Merged

Feat: Full server-side pagination for important events #3515

merged 8 commits into from
Sep 23, 2023

Conversation

chakflying
Copy link
Collaborator

@chakflying chakflying commented Aug 2, 2023 •
edited

⚠️⚠️⚠️ Since we do not accept all types of pull requests and do not want to waste your time. Please be sure that you have read pull request rules:
https://github.com/louislam/uptime-kuma/blob/master/CONTRIBUTING.md#can-i-create-a-pull-request-for-uptime-kuma

Tick the checkbox if you understand [x]:

  • I have read and understand the pull request rules.

Description

  • Remove the use of sendImportantHeartbeatList
  • There is no longer a global state of importantHeartbeatList in vue
  • Implement server-side pagination, where on initial load, the view request the total number of items, then each page of items is only loaded as they are selected
  • Use a global event bus mitt to pass new important heartbeats to any list currently active

The code change is relatively simple, but the performance improvement should be significant.

Fixes #726
Fixes #758
Fixes #3497
Fixes #3494
Fixes #3127
Fixes #1716
Fixes #1397
Fixes #2346

Type of change

  • Performance optimization

Checklist

  • My code follows the style guidelines of this project
  • I ran ESLint and other linters for modified files
  • I have performed a self-review of my own code and tested it
  • I have commented my code, particularly in hard-to-understand areas
    (including JSDoc for methods)
  • My changes generate no new warnings
  • My code needed automated testing. I have added them (this is optional task)

Screenshots (if any)

@chakflying chakflying mentioned this pull request Aug 2, 2023
Closed
7 tasks
CommanderStorm
CommanderStorm reviewed Aug 2, 2023
View reviewed changes
Copy link
Collaborator

@CommanderStorm CommanderStorm left a comment •
edited

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Overall, very clean. I have left some minor inline comments.

One question:
Why did you add mitt exactly? (instead of function calls)

server/server.js Outdated Show resolved Hide resolved
server/server.js Outdated Show resolved Hide resolved
src/pages/DashboardHome.vue Show resolved Hide resolved
src/pages/Details.vue
@@ -354,9 +340,23 @@ export default {
return getResBaseURL() + this.monitor.screenshot + "?time=" + this.cacheTime;
}
},

watch: {
page(to) {
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is the reactivity in this case correct? (am I missing something?) 🤔

Let's assume that monitor=null ⇒ the code is skipped.
If I then monitor gets populated ⇒ getImportantHeartbeatListPaged is never called or am I missing something?

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Good point. This code normally works because AFAIK in the vue lifecycle computed runs before mounted, so this.monitor should be true. I added a watch for monitor in case the monitorList has not been loaded when we are mounted for some reason.

@chakflying
Copy link
Collaborator Author

we need to send an event from the root of the vue app to deep inside the component tree Details.vue. I'm not aware of any function calls that can do this.

I looked through the alternatives suggested in the Vue 3 Docs but none of them seemed helpful, so the global event bus option was chosen.

@CommanderStorm CommanderStorm mentioned this pull request Aug 2, 2023
Closed
@louislam louislam added this to the 2.0.0 milestone Aug 3, 2023
@chakflying chakflying force-pushed the feat/important-beats-serverside branch from 6270abb to 27fc2a5 Compare August 4, 2023 10:32
@chakflying chakflying force-pushed the feat/important-beats-serverside branch from 27fc2a5 to 285320d Compare August 16, 2023 08:01
@chakflying chakflying changed the base branch from master to 2.0.X August 16, 2023 08:01
@chakflying chakflying mentioned this pull request Aug 17, 2023
Open
2 tasks
@CommanderStorm
Copy link
Collaborator

I found this issue, which is likely fixed by this PR:
#758

@louislam louislam changed the base branch from 2.0.X to master August 31, 2023 21:34
@CommanderStorm CommanderStorm mentioned this pull request Sep 8, 2023
Closed
2 tasks
@louislam louislam merged commit 7c49f7e into louislam:master Sep 23, 2023
12 of 14 checks passed
@Saibamen
Copy link
Contributor

Could this be ported to 1.23.x branch?

@CommanderStorm
Copy link
Collaborator

Given how many performance improvements have gone into v2, I would argue that migrating to v2 would be a better choice for our userbase.

=> I think classifying speed as a feature instead of a bug is very reasonable

@chakflying chakflying deleted the feat/important-beats-serverside branch September 26, 2023 04:08
@CommanderStorm CommanderStorm mentioned this pull request Nov 5, 2023
Closed
2 tasks
@CommanderStorm CommanderStorm mentioned this pull request Nov 25, 2023
Open
1 task
@CommanderStorm CommanderStorm mentioned this pull request Dec 8, 2023
Open
2 tasks
@CommanderStorm CommanderStorm mentioned this pull request Jan 17, 2024
Open
1 task
@CommanderStorm CommanderStorm mentioned this pull request Feb 17, 2024
Open
16 tasks
@CommanderStorm CommanderStorm mentioned this pull request Feb 26, 2024
Closed
2 tasks
@CommanderStorm CommanderStorm mentioned this pull request Apr 17, 2024
Closed
1 task
@CommanderStorm CommanderStorm mentioned this pull request May 8, 2024
Closed
2 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@CommanderStorm CommanderStorm CommanderStorm left review comments

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
2.0.0
4 participants
@chakflying @CommanderStorm @Saibamen @louislam