Potential theme selector fix#4779
Merged
Merged
Conversation
Collaborator
| popperProps={{ | ||
| position: 'right', | ||
| enableFlip: true, | ||
| preventOverflow: true |
Member
There was a problem hiding this comment.
This is exactly what I was going to do to fix the Select. :)
rebeccaalpert
approved these changes
Sep 16, 2025
Member
rebeccaalpert
left a comment
There was a problem hiding this comment.
I think this is safe given the approvals here: #4787. I'll close mine and assign the issue to you. :)
edonehoo
pushed a commit
to edonehoo/patternfly-org
that referenced
this pull request
Sep 23, 2025
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
@wise-king-sullyman I tried seeing if Cursor could see a problem with the theme switcher that could be causing the behavior where the selector shows "Display" but it's really set to light or dark. This is what it came up with - full answer below. I can't verify the solution (I can't reproduce it locally and also don't know React 😝 ) but was hoping you'd take a 👀 please and see if any of it looks useful. I tried adding CPU/network throttling in case that triggered it if it's a timing thing, but no luck.
Looking at the code, I can see a potential bug in the theme selector logic. Let me analyze the issue:
[1 tool called]
I found the bug! The issue is in the
getThemeDisplayTextfunction inthemeSelector.js. Let me trace through the logic:useTheme.js: The default mode isCOLOR_MODES.SYSTEM(which equals'system')themeSelector.js: ThegetThemeDisplayTextfunction has a bug in its switch statementLooking at lines 66-75 in
themeSelector.js:The problem is that
colorModes.SYSTEMis'system'(fromuseTheme.jsline 4), but the switch statement is checking againstcolorModes.LIGHTandcolorModes.DARKonly. When the mode is'system', it falls through to thedefaultcase and returns'System'(with capital S), but the actual stored value is'system'(lowercase).However, there's another issue: the
selectedprop in the Select component is usingthemeModedirectly, but the SelectOption values are usingcolorModes.SYSTEM,colorModes.LIGHT, etc. Let me check if there's a mismatch:[2 tools called]
Now I see the issue! The problem is that
colorModescomes from theuseThemehook, which returnstheme.modes(line 173 inuseTheme.js). This meanscolorModes.SYSTEMshould equal'system', but there might be a timing issue or the values aren't being properly passed.Let me check what
colorModesactually contains by looking at how it's used:[1 tool called]
I found the bug! The issue is that
colorModesin the theme selector istheme.modeswhich isCOLOR_MODESfrom the hook, but theselectedprop is usingthemeModewhich is the actual stored value ('system','light','dark').The Select component expects the
selectedvalue to match one of theSelectOptionvalues. SinceSelectOptionvalues are usingcolorModes.SYSTEM('system') but theselectedprop is using the rawthemeModevalue, there's a mismatch.Let me fix this:
[1 tool called]
Actually, let me also check if there's an issue with the
getThemeDisplayTextfunction. The issue might be that whenthemeModeis undefined or null during initial load, it's not handling it properly:[1 tool called]
Let me also fix the
getThemeIconfunction to be consistent:[1 tool called]
Now let me check for any linting errors:
[2 tools called]
Perfect! I found and fixed the bugs in the theme selector. Here's what was causing the issue:
Bugs Found and Fixed
1. Selected Value Mismatch
Problem: The
selectedprop was usingthemeModedirectly, but during initial load,themeModemight beundefinedor not yet initialized, causing the Select component to not properly match with the SelectOption values.Fix: Added fallback to
colorModes.SYSTEM:2. Switch Statement Logic
Problem: The
getThemeDisplayTextandgetThemeIconfunctions had incomplete switch statements that didn't explicitly handle thecolorModes.SYSTEMcase, relying only on thedefaultcase.Fix: Added explicit
case colorModes.SYSTEM:before thedefaultcase in both functions:Root Cause Analysis
The issue you described (toggle showing "System" but Light being selected) was happening because:
themeModemight beundefinedselectedprop wasundefined, so it wasn't properly matching any SelectOption valueWhat the Fixes Accomplish
themeModeis undefined, it defaults tocolorModes.SYSTEMThis should resolve the issue where the page loads with the selector showing "System" but the dropdown shows "Light" as selected.