Fix LLMStreamView usages not being responsive to device theme changes #372
Conversation
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
There was a problem hiding this comment.
Greptile Summary
This PR fixes a theming issue where LLMStreamView components weren't responding to system theme changes between light and dark modes. The changes affect two key files: GeneratedContentView.swift and QuickEditResponseView.swift.
The solution introduces dynamic color selection by:
- Adding
@Environment(\.colorScheme)to track the current system theme - Replacing hardcoded
Color.S_0with conditional logic that sets text color to white for dark mode and black for light mode - Updating the view's
.idmodifier to includecolorScheme.hashValue, which forces SwiftUI to recreate the LLMStreamView when the theme changes
This approach is necessary because the LLMStream library (an external dependency) doesn't automatically respond to SwiftUI environment changes, so the view recreation via the .id modifier ensures the new color configuration is applied immediately when users switch themes. The fix addresses a significant UX issue where text could become unreadable when the theme changed but the text color remained static.
The implementation follows a consistent pattern across both affected files, suggesting a systematic approach to resolving theme responsiveness issues throughout the codebase.
Confidence score: 3/5
• This change fixes a legitimate theming bug but introduces potential inconsistency with the app's color system
• The solution bypasses the established semantic color system (S_0, S_1, etc.) which should already handle theme adaptation, suggesting either those colors aren't working properly with the LLMStream library or there's a deeper architectural issue
• The files GeneratedContentView.swift and QuickEditResponseView.swift need attention to verify the semantic colors aren't working as expected
2 files reviewed, 4 comments
| let font = FontConfiguration(size: fontSize, lineHeight: lineHeight) | ||
| let color = ColorConfiguration(textColor: .S_0, | ||
|
|
||
| let textColor = colorScheme == .dark ? Color.white : Color.black |
There was a problem hiding this comment.
style: Using Color.white/Color.black conflicts with the custom rule that text should use .FG or low gray colors (.gray100, .gray200, .gray300) since the app only has dark mode
| let textColor = colorScheme == .dark ? Color.white : Color.black | |
| let textColor = colorScheme == .dark ? Color.FG : Color.gray100 |
| onUrlClicked: onUrlClicked, | ||
| onCodeAction: codeAction) | ||
| .id("\(fontSize)-\(lineHeight)") // Force recreation when font settings change | ||
| .id("\(fontSize)-\(lineHeight)-\(colorScheme.hashValue)") // Force recreation when font settings change |
There was a problem hiding this comment.
style: Consider using a more stable identifier than colorScheme.hashValue - perhaps colorScheme == .dark for better predictability
| let font = FontConfiguration(size: fontSize, lineHeight: lineHeight) | ||
| let color = ColorConfiguration(textColor: .S_0, | ||
|
|
||
| let textColor = colorScheme == .dark ? Color.white : Color.black |
There was a problem hiding this comment.
style: Consider using the existing semantic colors (Color.S_0) instead of hardcoded black/white. The S_0 color is already configured to be black in light mode and white in dark mode in the Assets catalog.
| onUrlClicked: onUrlClicked, | ||
| onCodeAction: codeAction) | ||
| .id("\(fontSize)-\(lineHeight)") // Force recreation when font settings change | ||
| .id("\(fontSize)-\(lineHeight)-\(colorScheme.hashValue)") // Force recreation when font settings change |
There was a problem hiding this comment.
style: Using colorScheme.hashValue could cause unnecessary view recreations if hashValue implementation changes. Consider using a more stable identifier like colorScheme.rawValue or a string representation.
| .id("\(fontSize)-\(lineHeight)-\(colorScheme.hashValue)") // Force recreation when font settings change | |
| .id("\(fontSize)-\(lineHeight)-\(colorScheme == .dark ? "dark" : "light")") // Force recreation when font settings change |
Niduank
left a comment
Niduank
left a comment
There was a problem hiding this comment.
The PR looks good 👍
Don't forget to save the patch and re-apply it in onit-beacon.
Thanks for the review! I'll merge this into the light/dark mode PR once it gets approved, then merge the light/dark mode PR into |
* WIP * Finish adding theming updates. * Update colors post-rebase with main * Add fix for theme-based text colors on LLMStreamViews. (#372) * Fix `TetheredButton` hover colors. --- * Fix post-rebase with `main`: * Shadow on `SimpleButton`. * Spacing and colors on `WebSearchTab`. * Disabled state in `RemoteModelSection` when verifying provider API token. * `Color` prefixes.
@Niduank I've ultimately decided to make the PR, after all! Just wanted to get your eyes/thoughts on my solution here.