-
-
Notifications
You must be signed in to change notification settings - Fork 4.5k
fix(ui): global body background color #102396
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
Conversation
static/app/styles/global.tsx
Outdated
| body.theme-dark { | ||
| background: ${theme.tokens.background.primary}; | ||
| } | ||
| body.theme-system { | ||
| @media (prefers-color-scheme: dark) { | ||
| background: ${theme.tokens.background.primary}; | ||
| } | ||
| } |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Bug: The body.theme-dark CSS rule uses lightTheme colors when isDark is false, if theme-dark class persists on body.
Severity: MEDIUM | Confidence: 0.95
🔍 Detailed Analysis
The body.theme-dark CSS rule evaluates with lightTheme colors when isDark is false, but the theme-dark class remains on the body element. This occurs when the theme changes via paths other than the hotkey (e.g., settings page, backend updates), which prevents removeBodyTheme() from being called. Consequently, if the theme-dark class persists on the body while the application is in light mode, the background will incorrectly display light mode colors, leading to visual inconsistencies.
💡 Suggested Fix
Use theme.inverted to style the body.theme-dark class, or conditionally apply different background colors based on the isDark prop, similar to existing patterns in the file.
🤖 Prompt for AI Agent
Review the code at the location below. A potential bug has been identified by an AI
agent.
Verify if this is a real issue. If it is, propose a fix; if not, explain why it's not
valid.
Location: static/app/styles/global.tsx#L146-L153
Potential issue: The `body.theme-dark` CSS rule evaluates with `lightTheme` colors when
`isDark` is `false`, but the `theme-dark` class remains on the `body` element. This
occurs when the theme changes via paths other than the hotkey (e.g., settings page,
backend updates), which prevents `removeBodyTheme()` from being called. Consequently, if
the `theme-dark` class persists on the `body` while the application is in light mode,
the background will incorrectly display light mode colors, leading to visual
inconsistencies.
Did we get this right? 👍 / 👎 to inform future reviews.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We need to remove isDark here, that shouldn't exist as we pass the theme in anyways.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
you’re right, I think to keep the code the same as in base.less I’ll add an if around this. we only need it for dark theme, and once UI1 is deleted and we updated base.less, we can actually remove this again.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Love the high quality investigation and explanation @TkDodo, you are setting a great example here 🙏🏼
| .theme-dark .loading.triangle .loading-indicator { | ||
| background: #fff; | ||
| } |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I know this looks weird, but hear me out: With @natemoo-re’s fix we are now taking the html from the splash loader and add it to the OrganizationLoadingIndicator in react’s context. But, if we have theme-dark, we still have global styles that have an invert filter applied:
sentry/static/less/shared-components.less
Lines 528 to 531 in a82d1ed
| .theme-dark .loading.triangle .loading-indicator { | |
| filter: invert(100%); | |
| opacity: 0.8; | |
| } |
so by re-setting the .theme-dark .loading .loading-indicator to background.primary, the triangle becomes white again because of the filter.
All of this is a mess and will hopefully become better with the new splash loader. Those global styles and invert filter are a pain and I hope we can remove them once that lands.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
however, this now reliably fixes that the loading spinner sometimes had the wrong background color. It’s really a pity that we mix and re-use styles for our loading spinner and the loading triangle with .loading and .loading.triangle...
but imo with the new splash loader, all those .trinagle things hopefully go away
This PR has two fixes: 1) we were using the _wrong_ background color in `base.less` for our `theme-dark` override, it was pointing to `theme.surface200`, but our “main” background color is `theme.surface300` (actually `theme.tokens.background.primary`, but in UI1, this points to `surface300`. I’ve updated that value and changed the comments to reflect the real location. This body background color isn’t visible a lot, because if we remove the `theme-dark` classname, our global body css will take effect, and that already points to the real value: https://github.com/getsentry/sentry/blob/84e8645edfe70f0c537f10c5fe73246ff411c550/static/app/styles/global.tsx#L142 However, this had the effect that e.g. the settings page could look different (in both UI1 and UI2) depending on if that class was still attached to the body or not, because `body.theme-dark` has a higher css specificity than just `body`. You can see that if you go to settings and switch between light and dark mode, you get different results: | with theme-dark class | after switching themes | |--------|--------| | <img width="1721" height="620" alt="Screenshot 2025-10-30 at 15 48 18" src="https://github.com/user-attachments/assets/71d66611-bb05-4e15-b85b-0b4f0f751fe2" /> | <img width="1721" height="633" alt="Screenshot 2025-10-30 at 15 48 30" src="https://github.com/user-attachments/assets/4f0cceca-41ed-4679-8a47-941ffd65541e" /> | This is where fix 2) comes in, where I’ve also added `body.theme-dark` (and the system one) to our global react styles so that they will also override those classes with the correct color. This is especially important for UI1 because the colors in `base.less` are UI1 colors and we _definitely_ don’t want to see them in UI2. After UI1 is removed, we could actually revert the second fix because the colors would “match”. Another situation where this helps if when loaders from lazy-loading are displayed because they were _also_ showing that (wrong) background color, resulting in flashes. before: https://github.com/user-attachments/assets/3c2c1501-f2f3-4d8f-bf4c-9d36e41eb6c1 after: https://github.com/user-attachments/assets/5af0ec12-4dac-45ba-976f-42481d2d5faf
This PR has two fixes: 1) we were using the _wrong_ background color in `base.less` for our `theme-dark` override, it was pointing to `theme.surface200`, but our “main” background color is `theme.surface300` (actually `theme.tokens.background.primary`, but in UI1, this points to `surface300`. I’ve updated that value and changed the comments to reflect the real location. This body background color isn’t visible a lot, because if we remove the `theme-dark` classname, our global body css will take effect, and that already points to the real value: https://github.com/getsentry/sentry/blob/84e8645edfe70f0c537f10c5fe73246ff411c550/static/app/styles/global.tsx#L142 However, this had the effect that e.g. the settings page could look different (in both UI1 and UI2) depending on if that class was still attached to the body or not, because `body.theme-dark` has a higher css specificity than just `body`. You can see that if you go to settings and switch between light and dark mode, you get different results: | with theme-dark class | after switching themes | |--------|--------| | <img width="1721" height="620" alt="Screenshot 2025-10-30 at 15 48 18" src="https://github.com/user-attachments/assets/71d66611-bb05-4e15-b85b-0b4f0f751fe2" /> | <img width="1721" height="633" alt="Screenshot 2025-10-30 at 15 48 30" src="https://github.com/user-attachments/assets/4f0cceca-41ed-4679-8a47-941ffd65541e" /> | This is where fix 2) comes in, where I’ve also added `body.theme-dark` (and the system one) to our global react styles so that they will also override those classes with the correct color. This is especially important for UI1 because the colors in `base.less` are UI1 colors and we _definitely_ don’t want to see them in UI2. After UI1 is removed, we could actually revert the second fix because the colors would “match”. Another situation where this helps if when loaders from lazy-loading are displayed because they were _also_ showing that (wrong) background color, resulting in flashes. before: https://github.com/user-attachments/assets/3c2c1501-f2f3-4d8f-bf4c-9d36e41eb6c1 after: https://github.com/user-attachments/assets/5af0ec12-4dac-45ba-976f-42481d2d5faf
This PR has two fixes:
base.lessfor ourtheme-darkoverride, it was pointing totheme.surface200, but our “main” background color istheme.surface300(actuallytheme.tokens.background.primary, but in UI1, this points tosurface300.I’ve updated that value and changed the comments to reflect the real location.
This body background color isn’t visible a lot, because if we remove the
theme-darkclassname, our global body css will take effect, and that already points to the real value:sentry/static/app/styles/global.tsx
Line 142 in 84e8645
However, this had the effect that e.g. the settings page could look different (in both UI1 and UI2) depending on if that class was still attached to the body or not, because
body.theme-darkhas a higher css specificity than justbody. You can see that if you go to settings and switch between light and dark mode, you get different results:This is where fix 2) comes in, where I’ve also added
body.theme-dark(and the system one) to our global react styles so that they will also override those classes with the correct color.This is especially important for UI1 because the colors in
base.lessare UI1 colors and we definitely don’t want to see them in UI2. After UI1 is removed, we could actually revert the second fix because the colors would “match”.Another situation where this helps if when loaders from lazy-loading are displayed because they were also showing that (wrong) background color, resulting in flashes.
before:
Screen.Recording.2025-10-30.at.15.21.23.mov
after:
Screen.Recording.2025-10-30.at.15.51.48.mov