Skip to content

Conversation

hunzlahmalik
Copy link

@hunzlahmalik hunzlahmalik commented Aug 10, 2025

Description

This PR introduces a unified health/alert state for Django Debug Toolbar panels and updates the UI to reflect panel.
The name health is debatable, but the alert was already taken by the panel.

Key Changes

  • Adds a ToolbarHealthLevel enum (NONE, WARNING, CRITICAL) for numeric health states.

  • Adds a health_level property to the base Panel class, allowing panels to report their health status.

  • Implements health_level in AlertsPanel and SQLPanel for alert and slow query detection.

  • The toolbar now computes the maximum health level across all panels and uses it to color the hidden toolbar handle.

  • UI Updates:

    • Panel buttons and the hidden toolbar handle use new CSS classes and colors based on health level.
    • Adds CSS variables and styles for warning and critical states.
Warning Warning on Hover Critical Critical on Hover
image image image image
Critical on Panel Warning on Panel
image image

Motivation

  • Improves user experience by visually highlighting issues in the toolbar.

Checklist:

  • I have added the relevant tests for this change.
  • I have added an item to the Pending section of docs/changes.rst.
  • Add to Documentation

@hunzlahmalik
Copy link
Author

hunzlahmalik commented Aug 10, 2025

@matthiask @tim-schilling

I'll be writing the testcases for this. It would be great if you could take a look at it before testcases.

Thanks!

Edit: You can also suggest how to compute health(debatable name) across other panel profiling, time etc...

Copy link
Member

@matthiask matthiask left a comment

Choose a reason for hiding this comment

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

I like the idea! I added some nitpicks and comments/general questions.


--djdt-health-bg-1: #4b3f1b;
--djdt-health-color-1: #ffe761;
--djdt-health-bc-1: #ffcc00;
Copy link
Member

Choose a reason for hiding this comment

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

What is bc? Background color? I think we prefer using variable names without abbreviations.


#djDebug .djdt-toolbarhandle-health-2 {
border-color: var(--djdt-health-bc-2) !important;
}
Copy link
Member

Choose a reason for hiding this comment

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

Can you add a comment why all these !important are necessary? And/or remove them when they are not?

</div>
<div class="djdt-hidden" id="djDebugToolbarHandle">
<div title="{% translate 'Show toolbar' %}" id="djShowToolBarButton">
<div title="{% translate 'Show toolbar' %}" id="djShowToolBarButton" class="{% if toolbar.health_level %} djdt-toolbarhandle-health-{{ toolbar.health_level }}{% endif %}">
Copy link
Member

Choose a reason for hiding this comment

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

I generally do not like uppercase letters much in ID and class attributes, but I wonder if we should keep capitalization for internal consistency here? Any thoughts?

Copy link
Author

Choose a reason for hiding this comment

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

They are inconsistent right now. Some places use camelCase when starting class names with dj, while others use lowercase with dashes (mostly when starting with djdt).

Rightnow, I can change them to camelcase if you want, but for consistency the changes in whole project will be required in future.

Copy link
Member

Choose a reason for hiding this comment

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

Ah, don't worry about it then. Being as consistent as possible is good enough.

Return the maximum health level across all panels.
This is used to color the toolbar hidden button.
"""
return max(panel.health_level for panel in self.panels)
Copy link
Member

Choose a reason for hiding this comment

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

What happens when self.panels is empty? Is it possible? I think you might want to provide a default value as a fallback (max((...), NONE))

)


class ToolbarHealthLevel(IntEnum):
Copy link
Member

Choose a reason for hiding this comment

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

I think this could be called HealthLevel only since it actually doesn't only apply to the toolbar itself but also to individual panels.

@hunzlahmalik hunzlahmalik force-pushed the main branch 2 times, most recently from 2c46d26 to b38335d Compare August 17, 2025 11:28
@hunzlahmalik hunzlahmalik requested a review from matthiask August 17, 2025 11:34
Comment on lines +38 to +43
--djdt-health-background-color-1: #4b3f1b;
--djdt-health-color-1: #ffe761;
--djdt-health-border-color-1: #ffcc00;
--djdt-health-background-color-2: #5a2327;
--djdt-health-color-2: #ffb3b3;
--djdt-health-border-color-2: #ff0000;
Copy link
Member

Choose a reason for hiding this comment

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

Could we use a descriptive word rather than 1 and 2 here please?

)


class HealthLevel(IntEnum):
Copy link
Member

Choose a reason for hiding this comment

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

@matthiask @hunzlahmalik do we want to bikeshed a bit on the name here? If we build out an API, this will likely be included in that and no longer be a private-ish thing. In my mind, "severity" may be more appropriate, but then it may lack the context of what is the severity in relation to.

Not sure, but want to bring it up since it's likely to become a public interface in the near future (< 1 years).

Copy link
Author

Choose a reason for hiding this comment

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

Yes we can change the name. I thought about it and was inclined to using the alert, but it was already taking in the app for the panel. We can use severity, we can see this as independent from the colorization. Later this can be use to get the panel severitylevel to do something else.

@tim-schilling
Copy link
Member

@hunzlahmalik this is really neat! How does it work with the history panel? Does the color only show for the request that renders the button? If you switch to a request that has problems, does the color update? Last question, I promise, did you consider adding the health level to the history panel so a person can view the high level issues of all the requests in a summary-esque view? I personally think that would be a fantastic way to leverage this feature.

@hunzlahmalik
Copy link
Author

hunzlahmalik commented Sep 23, 2025

For the panels that do define the health_level logic, after switching the request they should update the color too. Right now there are only two panels with health_level login, sqlpanel and alerts.

See this:
image
image

Regarding summary-esque view, what do you have in mind. We can colorize the rows in the history panel based on health, or do you want a separate some kind of filter/tab for it. I think coloring the rows looks good to me. Like if there is a request with health_level CRITICAL, then that row will be red, with warning, that row will be yellowish.

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.

3 participants