Skip to content

Conversation

@kurama
Copy link
Contributor

@kurama kurama commented Oct 29, 2025

❗ Important Announcements

Click here for more details:

⚠️ Please Note: We do not accept all types of pull requests, and we want to ensure we don’t waste your time. Before submitting, make sure you have read our pull request guidelines: Pull Request Rules

🚫 Please Avoid Unnecessary Pinging of Maintainers

We kindly ask you to refrain from pinging maintainers unless absolutely necessary. Pings are for critical/urgent pull requests that require immediate attention.

📋 Overview

What problem does this pull request address?
When deleting a monitor group that contains child monitors, the child monitors visually disappear from the monitor list until the page is refreshed. This creates a confusing user experience as it appears the monitors have been deleted along with the group, when in reality they still exist in the database.

What features or functionality does this pull request introduce or enhance?
This PR ensures that when a monitor group is deleted, all child monitors are properly unlinked and immediately repositioned at the root level in the UI without requiring a page refresh.

Questions

  1. Why are the package.json and package-lock.json versions not synchronized? Should I push the package-lock.json update?

  2. In the issue linked I noticed @Justinzobel gave two good ideas:

  • Add a checkbox if you want to remove the monitors in the group you want to remove
  • Update the message "Are you sure you want to delete this monitor?" based on the status of the checkbox, "Are you sure you want to delete this group?" and "Are you sure you want to delete this group and its monitors?"

I think this would be a good improvement to the user experience. I wanted to know what you thought before working on it.

🛠️ Type of change

  • 🐛 Bugfix (a non-breaking change that resolves an issue)
  • ✨ New feature (a non-breaking change that adds new functionality)
  • ⚠️ Breaking change (a fix or feature that alters existing functionality in a way that could cause issues)
  • 🎨 User Interface (UI) updates
  • 📄 New Documentation (addition of new documentation)
  • 📄 Documentation Update (modification of existing documentation)
  • 📄 Documentation Update Required (the change requires updates to related documentation)
  • 🔧 Other (please specify):
    • Provide additional details here.

📄 Checklist

  • 🔍 My code adheres to the style guidelines of this project.
  • 🦿 I have indicated where (if any) I used an LLM for the contributions
  • ✅ I ran ESLint and other code linters for modified files.
  • 🛠️ I have reviewed and tested my code.
  • 📝 I have commented my code, especially in hard-to-understand areas (e.g., using JSDoc for methods).
  • ⚠️ My changes generate no new warnings.
  • 🤖 My code needed automated testing. I have added them (this is an optional task).
  • 📄 Documentation updates are included (if applicable).
  • 🔒 I have considered potential security impacts and mitigated risks.
  • 🧰 Dependency updates are listed and explained.
  • 📚 I have read and understood the Pull Request guidelines.

📷 Screenshots or Visual Changes

  • UI Modifications: Highlight any changes made to the user interface.
  • Before & After: Include screenshots or comparisons (if applicable).

Before Group deletion without refreshing
image

After Group deletion without refreshing
image

Event Before After
UP Before After
DOWN Before After
Certificate-expiry Before After
Testing Before After

@kurama kurama force-pushed the fix/monitors-disappear-after-group-deletion branch from d532c01 to bc30559 Compare October 29, 2025 16:39
@kurama
Copy link
Contributor Author

kurama commented Oct 29, 2025

Could you help me with the GitHub Actions? I can't retry them and I don't understand the error...

@kurama kurama marked this pull request as ready for review October 29, 2025 19:03
Copilot AI review requested due to automatic review settings October 29, 2025 19:03
Copy link
Contributor

Copilot AI left a 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 functionality to properly handle children of group monitors when the parent group is deleted. Before deletion, the code now unlinks all child monitors from the group and notifies the frontend to update the UI for each affected child monitor.

Key changes:

  • Fetches the group monitor before deletion to check its type
  • Retrieves and unlinks all children if the monitor is a group type
  • Sends frontend updates for each child monitor to reflect the parent removal

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

Comment on lines +1078 to +1080
for (const child of children) {
await server.sendUpdateMonitorIntoList(socket, child.id);
}
Copy link

Copilot AI Oct 29, 2025

Choose a reason for hiding this comment

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

Sequential await calls inside a loop can cause performance issues. Consider using Promise.all() to send updates concurrently: await Promise.all(children.map(child => server.sendUpdateMonitorIntoList(socket, child.id)));

Suggested change
for (const child of children) {
await server.sendUpdateMonitorIntoList(socket, child.id);
}
await Promise.all(children.map(child => server.sendUpdateMonitorIntoList(socket, child.id)));

Copilot uses AI. Check for mistakes.
Copy link
Contributor Author

Choose a reason for hiding this comment

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

I don’t think Promise.all() fits here. It could mess up event ordering in Socket.IO, overload the DB with parallel queries (especially on SQLite), and wouldn’t bring much performance gain since the main cost is network latency. A better approach would be a batched update method like sendMultipleUpdatesMonitorIntoList(socket, childIds), doing one DB query and one socket emit for all children at once. WDYT ?

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.

Works as expected, Code LGTM

Thanks for tackling this 🎉

Regarding your questions:

I think this would be a good improvement to the user experience. I wanted to know what you thought before working on it.

Yes, making this configurable like that is likely a good idea.

Why are the package.json and package-lock.json versions not synchronized? Should I push the package-lock.json update?

We had a bug in our release script, thus the version in the package-lock was not updated. This should be fixed automatically in the next release.

@CommanderStorm CommanderStorm merged commit 5207ba6 into louislam:master Nov 3, 2025
19 checks passed
@CommanderStorm CommanderStorm added this to the 2.1.0 milestone Nov 3, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Clarify in the delete monitor if it will delete child monitors

3 participants