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.

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

feat: display when the next check will happen as a heartbeat timer #5189

Open
wants to merge 26 commits into
base: master
Choose a base branch
from

Conversation

mohit-nagaraj
Copy link
Contributor

⚠️⚠️⚠️ 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

  1. navigating to other pages and coming back to this one will still have the timer to continue
  2. update the timer when monitor is paused and played back
  3. reset the timer when a ping is made based on whenever it makes (keeps it consistnent cuz heartbeat & heart retry can be at different times hence reset it whenever request is made

Fixes #5120

Type of change

Please delete any options that are not relevant.

  • New feature (non-breaking change which adds functionality)

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 generates no new warnings
  • My code needed automated testing. I have added them (this is optional task)

Screenshots (if any)

image
added all test cases to work

Please do not use any external image service. Instead, just paste in or drag and drop the image here, and it will be uploaded automatically.

Copy link
Collaborator

@CommanderStorm CommanderStorm left a comment

Choose a reason for hiding this comment

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

I have left some comments below

src/pages/Details.vue Outdated Show resolved Hide resolved
src/pages/Details.vue Outdated Show resolved Hide resolved
src/pages/Details.vue Outdated Show resolved Hide resolved
src/pages/Details.vue Show resolved Hide resolved
src/pages/Details.vue Outdated Show resolved Hide resolved
@CommanderStorm CommanderStorm marked this pull request as draft October 11, 2024 22:21
@CommanderStorm CommanderStorm changed the title feat(app): #5120 Upcoming heartbeat timer feat: display when the next check will happen as a heartbeat timer Oct 11, 2024
@CommanderStorm CommanderStorm added area:dashboard The main dashboard page where monitors' status are shown pr:please address review comments this PR needs a bit more work to be mergable labels Oct 11, 2024
@louislam
Copy link
Owner

louislam commented Oct 12, 2024

Based on your description, this pr might be going into an incorrect direction, and this pr could be much difficult to be implemented that you think. We should have a discussion before we go.

Some points come in my mind:

  • It can't be frontend only, because frontend can never know the next check time.
  • Use this.lastHeartBeat.end_time to "guess" the next check time won't work.
  • Uptime Kuma is a socket.io application, a monitor could be changed by another browser at the same time.
  • Restart Uptime Kuma also affect it.

And actually, I have a plan to change the core to Croner (#3447)

Croner provides the next run time, which fits for this pr:

job.nextRun( /*optional*/ startFromDate );	// Get a Date object representing the next run.

I would suggest that this pr should be based on #3447

@mohit-nagaraj
Copy link
Contributor Author

Hey @louislam i do get your point. The thing is since the lastheart gets updated whenever we change the interval (even from other browser, all the related data about it also gets updated. Below is a video attached for ur reference.

Video link - https://www.loom.com/share/65820c1c0e624839ac0f7cce6fa78982

@mohit-nagaraj mohit-nagaraj marked this pull request as ready for review October 14, 2024 07:18
@mohit-nagaraj

This comment has been minimized.

@mohit-nagaraj

This comment has been minimized.

@CommanderStorm
Copy link
Collaborator

FYI: pinging me daily will not get your PRs reviewed and merged quicker.
On the contrary: being annoying usually has the opposite effect of pushing your PR to the back of peoples queues.

If it has been a month and you think your PR has been forgotten, fine... but daily?...

@louislam
Copy link
Owner

louislam commented Oct 17, 2024

Please read our pull request rules, rushing is not allowed.

And I think you didn't get my point of my previous comment, frontend only timer must be wrong in some cases, because only backend knows the actual next check time.

You video didn't cover all possible cases unfortunately.

@mohit-nagaraj
Copy link
Contributor Author

Hey first of all i am sorry if it disturbed you a lot. I didnt meant to to cause any trouble to you guys. Secondly, All i was expecting was some communication from you guys stating "sure" "will check it". Since days were passing without any information, I thought this pr is becoming stale. I didnt want to be a trouble in your already busy schdule. Third, if you want more video content covering all test cases do let me know. I wanted to just show for that case. Alternatively if required, you can post the ones which fail here after checking out the code. I do get your point of having this on backend, but its simply uncessary. Once again thank you for replying yesterday. If you guys require something else, do let me kno. Plus just a small quick reminder tht hacktober fest ends in like 11 days. Thank you

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area:dashboard The main dashboard page where monitors' status are shown pr:please address review comments this PR needs a bit more work to be mergable
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Heartbeat Interval (Check every XX seconds)
3 participants