Retain button labels and use readable text colors - #406
Merged
Conversation
Issue: Fix dark Cancel labels and button text flickering during caret-driven redraws, and address the same texture-lifetime problem in related UI buttons. Solution: Use readable theme foreground colors for outlined labels and retain cached SDL text textures across frames. Invalidate caches when fonts, labels, or colors change so dynamic modal text remains correct without same-frame texture destruction.
There was a problem hiding this comment.
🟡 Changes recommended
The remove-modal path label is still created and destroyed within the frame, leaving the stated rendering issue partially unresolved.
Once you've addressed the issues Copilot identified, you can request another Copilot review.
Pull request overview
Retains UI label textures to prevent Metal rendering flicker and improves button-label readability.
Changes:
- Adds reusable cached button-label textures.
- Applies caching across worktree, reader, quit, and diff actions.
- Registers color-policy tests and documents the rendering invariant.
File summaries
| File | Description |
|---|---|
src/ui/components/button.zig |
Adds texture caching and label-color policy. |
src/ui/components/worktree_overlay.zig |
Retains modal and button labels. |
src/ui/components/reader_overlay.zig |
Caches the jump-button label. |
src/ui/components/quit_confirm.zig |
Caches confirmation button labels. |
src/ui/components/diff_overlay.zig |
Caches comment and send-button labels. |
src/main.zig |
Registers button tests. |
docs/ARCHITECTURE.md |
Documents retained texture requirements. |
Review details
- Files reviewed: 7/7 changed files
- Comments generated: 1
- Review effort level: Balanced
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
Address review comment 3989851456: retain the remove-modal path textures across frames. Cache wrapped path lines by their source text, font identity and generation, color, and available width, and release them when the removal target changes or the modal closes.
Issue: The worktree removal-modal cache refactor left a stale method-style call that made the macOS build fail during compilation.\nSolution: Invoke the static cached-path renderer with the cache pointer it expects, preserving the existing texture cache while restoring type-correct compilation.
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.
Issue
Outlined Cancel labels can render nearly black against dark button fills. Worktree Confirm and related button labels can also flicker during caret-driven redraws because their SDL textures are created and destroyed in the same frame.
Solution
Use the theme foreground for outlined button labels so they remain readable against selection fills. Retain button and modal-label SDL textures across frames, invalidating them when their font, text, or color changes. Apply the same lifetime fix to related action buttons and register a unit test for the shared color policy.
Context
This follows the existing UI rendering invariant that textures queued by SDL's Metal backend must not be destroyed within the frame that renders them.
Test plan