Skip to content

feat: Decouple Clear All Events from heartbeat msg deletion (#7293)#7295

Closed
himax12 wants to merge 2 commits intolouislam:masterfrom
himax12:feature/7293-clear-events-modal
Closed

feat: Decouple Clear All Events from heartbeat msg deletion (#7293)#7295
himax12 wants to merge 2 commits intolouislam:masterfrom
himax12:feature/7293-clear-events-modal

Conversation

@himax12
Copy link
Copy Markdown

@himax12 himax12 commented Apr 19, 2026

Summary

In this pull request, the following changes are made:

Please follow this checklist to avoid unnecessary back and forth (click to expand)
  • If there are Breaking change (a fix or feature that alters existing functionality in a way that could cause issues) I have called them out
  • I have disclosed any use of LLMs/AI in this contribution and reviewed all generated content.
    I understand that I am responsible for and able to explain every line of code I submit.
  • Any UI changes adhere to visual style of this project.
  • I have self-reviewed and self-tested my code to ensure it works as expected.
  • I have commented my code, especially in hard-to-understand areas (e.g., using JSDoc for methods).
  • 🤖 I added or updated automated tests where appropriate.
  • 📄 Documentation updates are included (if applicable).
  • 🧰 Dependency updates are listed and explained.
  • CI passes and is green.

Screenshots for Visual Changes

The new confirmation modal appears when clicking "Clear All Events":

State Modal
Default (safe) Checkbox unchecked, primary button
Destructive Checkbox checked, danger button

UI Modifications:

  • Added new ClearEventsConfirm.vue component
  • Replaced Confirm.vue usage in DashboardHome.vue with new component

Before: Single button click immediately deleted msg data silently
After: Modal with checkbox gives user explicit choice

Testing

  1. Click "Clear All Events" without checking the checkbox - timeline clears, msg data preserved
  2. Click "Clear All Events" with checkbox checked - timeline clears AND msg data deleted

Code Explanation

I reviewed and understand all changes:

  1. server/server.js: The clearEvents handler now takes a second boolean parameter clearMsg. When false/undefined, only the important flag is cleared (timeline cleanup). When true, both msg and important are cleared (original destructive behavior).

  2. src/components/ClearEventsConfirm.vue: New modal component following existing Bootstrap Modal patterns. Added beforeUnmount lifecycle hook to properly clean up event listeners and dispose the Bootstrap modal instance (addressing Copilot review feedback).

  3. src/pages/DashboardHome.vue: Changed to use the new ClearEventsConfirm component instead of generic Confirm. The clearAllEvents method now passes the checkbox state to the backend.

  4. src/lang/en.json: Added three new translation keys for the modal text.

…eartbeat msg data

Decouples the UI timeline clear from database msg deletion as requested in issue louislam#7293.

Changes:
- Added ClearEventsConfirm.vue component with checkbox option
- Modified clearEvents socket handler to accept optional clearMsg parameter
- Default behavior now preserves msg data (clear timeline only)
- Users can opt-in to also delete msg data for disk space savings
- Fixed event listener memory leak (cleanup in beforeUnmount)

New i18n keys added:
- clearAllEventsMsgTimelineOnly: Timeline cleanup description
- clearAllEventsMsgDataWarning: Data deletion warning
- Also clear historical event data: Checkbox label
Copilot AI review requested due to automatic review settings April 19, 2026 21:17
Copy link
Copy Markdown
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 updates the “Clear All Events” flow to default to clearing only timeline-visible events while optionally allowing users to also purge stored heartbeat msg payloads, addressing #7293.

Changes:

  • Adds a dedicated ClearEventsConfirm modal with a checkbox controlling whether heartbeat msg data is also cleared.
  • Extends the clearEvents Socket.IO handler to accept an optional clearMsg parameter.
  • Updates the dashboard to use the new modal and pass the checkbox state to the backend.

Reviewed changes

Copilot reviewed 4 out of 4 changed files in this pull request and generated 3 comments.

File Description
src/pages/DashboardHome.vue Swaps the generic confirm modal for ClearEventsConfirm and sends clearMsg to the backend when clearing events.
src/components/ClearEventsConfirm.vue New Bootstrap modal component that collects the “also clear msg data” choice and emits it to the parent.
server/server.js Changes clearEvents handler behavior based on clearMsg (timeline-only vs. also clear heartbeat msg).
src/lang/en.json Adds new i18n strings for the updated modal copy.
Comments suppressed due to low confidence (1)

src/pages/DashboardHome.vue:291

  • failed is incremented inside the Socket.IO acknowledgement callback, but success/error toasts and clearingAllEvents are set immediately after forEach. This means the UI will almost always report success (and stop showing the loading state) before any responses arrive. Consider awaiting all acknowledgements (e.g., wrap emits in Promises and await Promise.all) before computing failed and clearing the loading flag.
                        failed++;
                    }
                });
            });
            this.clearingAllEvents = false;
            this.page = 1;
            this.getImportantHeartbeatListLength();
            if (failed === 0) {
                this.$root.toastSuccess(this.$t("Events cleared successfully"));
            } else {
                this.$root.toastError(
                    this.$t("Could not clear events", {
                        failed,

Comment on lines +39 to +42
</div>
</div>
<div class="modal-footer">
<button type="button" class="btn btn-secondary" @click="close">
Copy link

Copilot AI Apr 19, 2026

Choose a reason for hiding this comment

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

The checkbox label/text implies "clear historical event data", but the actual behavior behind clearMsg is deleting the heartbeat msg payload (not clearing events/heartbeats themselves). This mismatch can mislead users about what will be removed; please align the wording (label + warning text) with the actual destructive action (e.g., explicitly mention clearing heartbeat message/payload data).

Copilot uses AI. Check for mistakes.
Comment thread src/lang/en.json
Comment on lines +702 to +703
"clearAllEventsMsgDataWarning": "Also delete all msg data from heartbeats. This cannot be undone.",
"Also clear historical event data": "Also clear historical event data",
Copy link

Copilot AI Apr 19, 2026

Choose a reason for hiding this comment

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

User-facing copy uses the internal column name "msg" ("Also delete all msg data...") and doesn’t clearly explain what data is being removed. Consider rewording to something like "heartbeat message/payload data" (and keep it consistent with the checkbox label) so non-technical users understand the impact.

Suggested change
"clearAllEventsMsgDataWarning": "Also delete all msg data from heartbeats. This cannot be undone.",
"Also clear historical event data": "Also clear historical event data",
"clearAllEventsMsgDataWarning": "Also delete heartbeat message/payload data. This cannot be undone.",
"Also clear historical event data": "Also clear heartbeat message/payload data",

Copilot uses AI. Check for mistakes.
Comment thread server/server.js
Comment on lines +1634 to +1646
socket.on("clearEvents", async (monitorID, clearMsg, callback) => {
try {
checkLogin(socket);

log.info("manage", `Clear Events Monitor: ${monitorID} User ID: ${socket.userID}`);

await R.exec("UPDATE heartbeat SET msg = ?, important = ? WHERE monitor_id = ? ", ["", "0", monitorID]);
if (clearMsg) {
// Clear both msg and important flag (original behavior)
await R.exec("UPDATE heartbeat SET msg = ?, important = ? WHERE monitor_id = ?", [
"",
"0",
monitorID,
]);
Copy link

Copilot AI Apr 19, 2026

Choose a reason for hiding this comment

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

clearEvents socket handler signature change is not backward-compatible: existing clients still emit clearEvents as (monitorID, callback). In that case clearMsg becomes the callback function (truthy), callback is undefined, and the handler will both wipe msg unexpectedly and throw when calling callback(...). Please support both call shapes (e.g., if typeof clearMsg === "function" then treat it as callback and default clearMsg to false) and guard against missing callbacks.

Copilot uses AI. Check for mistakes.
@hemanth5544
Copy link
Copy Markdown
Contributor

Hello @himax12 #7293 as this issue states there is no need to over enginner the flow (as your pr) we just need to inform the user
@CommanderStorm i think this is a pure ai-slop code
updated a pr at #7299

@CommanderStorm
Copy link
Copy Markdown
Collaborator

yea, as said: lets just add a better warning to the confirmation modal (i think there should be one?)

@himax12
Copy link
Copy Markdown
Author

himax12 commented Apr 20, 2026 via email

@CommanderStorm
Copy link
Copy Markdown
Collaborator

We have merged a competing PR that was simpler.

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.

Feature Request: Decouple "Clear All Events" from heartbeat msg payload deletion (or add UI warning)

4 participants