Skip to content

Conversation

@kurama
Copy link
Contributor

@kurama kurama commented Nov 24, 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

Test on my laptpo Old Version (div) New Version (canva) Improvement
1 47.13s 14.01s -70.3%
2 45.57s 12.94s -71.6%
3 46.52s 13.18s -71.7%
4 46.24s 13.35s -71.1%
5 47.80s 13.17s -72.5%
Average 46.65s 13.33s -71.4%

What problem does this pull request address?

This PR addresses a critical performance bottleneck in the HeartbeatBar component when displaying large numbers of monitors (800+). The div-based rendering created 12,000+ DOM elements (800 monitors * ~15 divs each), causing render times averaging 46.65 seconds and significant browser lag.

What features or functionality does this pull request introduce or enhance?

This PR introduces Canvas-based rendering that replaces div elements with HTML5 Canvas. The implementation reduces average render time from 46.65s to 13.33s.

Why Canvas over SVG?

Canvas was chosen because it renders programmatically to a single bitmap, resulting in just one DOM element regardless of content complexity. SVG would still create individual DOM nodes for each rectangle, defeating the purpose of the optimization. Canvas excels at rendering many similar shapes with minimal memory overhead.

Potential future improvements:
Implement virtual scrolling/lazy rendering to only render visible monitors?

Feel free to tell me what you think and suggest improvements! :)

🛠️ 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:
image

After:
image

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

@kurama kurama marked this pull request as ready for review November 24, 2025 10:47
Copilot AI review requested due to automatic review settings November 24, 2025 10:47
Copilot finished reviewing on behalf of kurama November 24, 2025 10:50
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 addresses a critical performance bottleneck in the HeartbeatBar component by replacing div-based rendering with HTML5 Canvas rendering. With 800+ monitors creating 12,000+ DOM elements, the original implementation had render times averaging 46.65 seconds. The Canvas-based approach reduces this to 13.33 seconds (71.4% improvement) by rendering all beats to a single canvas element.

Key Changes:

  • Replaced v-for loop of div elements with a single canvas element that programmatically draws all heartbeat bars
  • Implemented custom mouse event handling to detect hover states and show tooltips based on canvas coordinates
  • Added watchers for shortBeatList and hoveredBeatIndex to trigger canvas redraws when data or hover state changes

Comment on lines +294 to +302
shortBeatList: {
handler() {
this.canvasNeedsRedraw = true;
this.$nextTick(() => {
this.drawCanvas();
});
},
deep: true,
},
Copy link

Copilot AI Nov 24, 2025

Choose a reason for hiding this comment

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

The shortBeatList watcher with deep: true will trigger for any nested property changes within the beat objects. Since shortBeatList is a computed property based on beatList, and beatList already has a watcher, this may cause redundant canvas redraws. Consider if deep: true is necessary here, or if this watcher can be removed entirely since changes to beatList will already trigger redraws.

Suggested change
shortBeatList: {
handler() {
this.canvasNeedsRedraw = true;
this.$nextTick(() => {
this.drawCanvas();
});
},
deep: true,
},
// Removed redundant watcher on shortBeatList

Copilot uses AI. Check for mistakes.
Comment on lines +605 to +623
getBeatColor(beat) {
if (beat === 0 || beat === null || beat?.status === null) {
// Empty beat color
return getComputedStyle(document.documentElement).getPropertyValue("--bs-body-bg") || "#f0f8ff";
}
/*
pointer-events needs to be changed because
tooltip momentarily disappears when crossing between .beat-hover-area and .beat
*/
pointer-events: none;
const status = Number(beat.status);
&.empty {
background-color: aliceblue;
if (status === DOWN) {
return getComputedStyle(document.documentElement).getPropertyValue("--bs-danger") || "#dc3545";
} else if (status === PENDING) {
return getComputedStyle(document.documentElement).getPropertyValue("--bs-warning") || "#ffc107";
} else if (status === MAINTENANCE) {
return getComputedStyle(document.documentElement).getPropertyValue("--maintenance") || "#1d4ed8";
} else {
// UP status - primary color
return getComputedStyle(document.documentElement).getPropertyValue("--bs-primary") || "#5cdd8b";
}
},
Copy link

Copilot AI Nov 24, 2025

Choose a reason for hiding this comment

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

The getBeatColor method calls getComputedStyle(document.documentElement) for each beat on every canvas redraw. For 800 monitors, this results in thousands of DOM queries. Consider caching these CSS custom property values once during component initialization and updating them only when the theme changes, rather than recomputing them for every beat on every redraw.

Copilot uses AI. Check for mistakes.
// Draw beat rectangle
ctx.fillStyle = color;
const borderRadius = 2.5;
Copy link

Copilot AI Nov 24, 2025

Choose a reason for hiding this comment

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

The magic number 2.5 for border radius lacks context. Consider extracting this to a named constant (e.g., const BEAT_BORDER_RADIUS = 2.5;) at the top of the method or as a component data property to improve code maintainability and make it easier to adjust the styling consistently.

Copilot uses AI. Check for mistakes.
Comment on lines +4 to +12
<canvas
ref="canvas"
class="heartbeat-canvas"
:width="canvasWidth"
:height="canvasHeight"
@mousemove="handleMouseMove"
@mouseleave="hideTooltip"
@focus="showTooltip(beat, $event)"
@blur="hideTooltip"
>
<div
class="beat"
:class="getBeatClasses(beat)"
:style="beatStyle"
/>
</div>
@click="handleClick"
/>
Copy link

Copilot AI Nov 24, 2025

Choose a reason for hiding this comment

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

The Canvas implementation removes keyboard navigation and ARIA support that was present in the original div-based implementation. The previous version had tabindex="0", @focus, @blur event handlers, and aria-label attributes for accessibility. Consider adding keyboard navigation support and making the canvas focusable with appropriate ARIA attributes to maintain accessibility for screen reader users and keyboard-only users.

Copilot uses AI. Check for mistakes.
tooltipTimeoutId: null,
// Canvas
hoveredBeatIndex: -1,
canvasNeedsRedraw: true,
Copy link

Copilot AI Nov 24, 2025

Choose a reason for hiding this comment

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

The canvasNeedsRedraw data property is set to true in multiple watchers (lines 281, 296) but is never actually used to conditionally skip or perform canvas redraws. This appears to be unused code. Consider either implementing the optimization to check this flag before redrawing, or removing it if not needed.

Copilot uses AI. Check for mistakes.
Comment on lines +304 to +306
hoveredBeatIndex() {
this.drawCanvas();
},
Copy link

Copilot AI Nov 24, 2025

Choose a reason for hiding this comment

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

The hoveredBeatIndex watcher triggers a full canvas redraw on every hover change. For 800+ monitors, this could still cause performance issues during mouse movement. Consider implementing a more efficient redraw that only updates the affected beats (the previously hovered and newly hovered beats) using partial canvas updates or requestAnimationFrame to throttle redraws.

Copilot uses AI. Check for mistakes.
/**
* Hide custom tooltip
* @param {boolean} resetHoverIndex Whether to reset the hovered beat index
Copy link

Copilot AI Nov 24, 2025

Choose a reason for hiding this comment

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

The JSDoc comment should document the parameter type. Update the @param to: @param {boolean} resetHoverIndex Whether to reset the hovered beat index

Copilot uses AI. Check for mistakes.
white-space: nowrap;
}
const ctx = canvas.getContext("2d");
const dpr = window.devicePixelRatio || 1;
Copy link

Copilot AI Nov 24, 2025

Choose a reason for hiding this comment

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

The devicePixelRatio is read on every canvas redraw, but it typically doesn't change during a session. Consider caching this value in the component's data or computing it once during mount to avoid repeated property lookups, especially since this method is called frequently during hover interactions.

Copilot uses AI. Check for mistakes.
Comment on lines +649 to +663
// Only hide tooltip if we had one showing
if (previousIndex !== -1) {
this.hideTooltip(false); // Don't reset hover index
// Show tooltip for new beat after a brief delay
setTimeout(() => {
// Verify we're still hovering the same beat
if (this.hoveredBeatIndex === beatIndex) {
this.showTooltip(beat, beatIndex, rect);
}
}, 50);
} else {
// First time hovering, show immediately
this.showTooltip(beat, beatIndex, rect);
}
}
Copy link

Copilot AI Nov 24, 2025

Choose a reason for hiding this comment

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

The nested conditional logic with multiple tooltip hide/show calls and setTimeout makes this code difficult to follow. The 50ms delay on line 653-658 seems arbitrary and could cause tooltip flickering when rapidly moving between beats. Consider extracting this tooltip management logic into a separate method with clearer intent, or using a debounce utility to handle rapid hover changes more elegantly.

Copilot uses AI. Check for mistakes.
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.

Pagination on Dashboard main page

1 participant