fix(ui): keep popups opaque in transparent mode#411
Conversation
Greptile SummaryThis PR fixes transparent-background mode leaking through menu dropdowns and the help dialog by passing
Confidence Score: 4/5The production code change is a safe, targeted two-line swap; the main concern is that the new test's menu assertion doesn't fully cover the fix it's meant to guard. The App.tsx change is straightforward and correct — baseTheme is the right value to pass to popup overlays. The interaction test covers the help-dialog assertion well (checking theme.panel, which withTransparentBackground does clear), but the menu check uses theme.accentMuted, a color that is never made transparent, so that assertion would pass even without the fix. The hasLineWithBackground helper also doesn't require the background color to be on the same span as the matching text, which could yield a false positive in edge cases. src/ui/AppHost.interactions.test.tsx — the menu-opacity assertion and span-matching logic in the new helpers warrant a second look. Important Files Changed
Flowchart%%{init: {'theme': 'neutral'}}%%
flowchart TD
A[transparentBackground option] --> B{activeTheme}
B --> |withTransparentBackground| C[Transparent surfaces\nbackground, panel, panelAlt…]
B --> |baseTheme| D[Opaque surfaces]
C --> E[MenuBar]
C --> F[Sidebar]
C --> G[DiffPane]
C --> H[StatusBar]
D --> I[LazyMenuDropdown\n✅ baseTheme after fix]
D --> J[LazyHelpDialog\n✅ baseTheme after fix]
Prompt To Fix All With AIFix the following 3 code review issues. Work through them one at a time, proposing concise fixes.
---
### Issue 1 of 3
src/ui/AppHost.interactions.test.tsx:476-481
**`hasLineWithBackground` checks text and background on unrelated spans**
The predicate asserts that the line's joined text contains the target string *and* that some span on the line has the expected background, but the two spans don't have to be the same span. A line where the text lives in a transparent span and a totally different span carries the `accentMuted`/`panel` color would still pass. For stronger coverage, filter `line.spans` to those whose `text` contributes to the match before checking the background.
### Issue 2 of 3
src/ui/AppHost.interactions.test.tsx:2421-2423
**Menu-opacity assertion doesn't target what the fix changes**
`theme.accentMuted` is not touched by `withTransparentBackground`, so `baseTheme.accentMuted === activeTheme.accentMuted`. This `hasLineWithBackground` check would pass identically before and after the fix, meaning it doesn't actually verify the menu container stays opaque. To prove the fix, check a color that `withTransparentBackground` clears — e.g. `theme.background` or `theme.panel` — on the menu container spans instead.
### Issue 3 of 3
src/ui/AppHost.interactions.test.tsx:455-468
**`capturedColorToHex` is duplicated from `ui-components.test.tsx`**
The identical helper already exists at line 376 of `src/ui/components/ui-components.test.tsx`. Extracting it to a shared test-helper module (e.g. `test/helpers/color-helpers.ts`) would remove the duplication and keep the two implementations in sync if the buffer format ever changes.
Reviews (1): Last reviewed commit: "fix(ui): keep popups opaque in transpare..." | Re-trigger Greptile |
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
| return frame.lines.some((line) => | ||
| line.spans.map((span) => span.text).join("").includes(text) && | ||
| line.spans.some( | ||
| (span) => capturedColorToHex(span.bg)?.toLowerCase() === backgroundColor.toLowerCase(), | ||
| ), | ||
| ); |
There was a problem hiding this comment.
hasLineWithBackground checks text and background on unrelated spans
The predicate asserts that the line's joined text contains the target string and that some span on the line has the expected background, but the two spans don't have to be the same span. A line where the text lives in a transparent span and a totally different span carries the accentMuted/panel color would still pass. For stronger coverage, filter line.spans to those whose text contributes to the match before checking the background.
Prompt To Fix With AI
This is a comment left during a code review.
Path: src/ui/AppHost.interactions.test.tsx
Line: 476-481
Comment:
**`hasLineWithBackground` checks text and background on unrelated spans**
The predicate asserts that the line's joined text contains the target string *and* that some span on the line has the expected background, but the two spans don't have to be the same span. A line where the text lives in a transparent span and a totally different span carries the `accentMuted`/`panel` color would still pass. For stronger coverage, filter `line.spans` to those whose `text` contributes to the match before checking the background.
How can I resolve this? If you propose a fix, please make it concise.| expect( | ||
| hasLineWithBackground(setup.captureSpans(), "Toggle files/filter focus", theme.accentMuted), | ||
| ).toBe(true); |
There was a problem hiding this comment.
theme.accentMuted is not touched by withTransparentBackground, so baseTheme.accentMuted === activeTheme.accentMuted. This hasLineWithBackground check would pass identically before and after the fix, meaning it doesn't actually verify the menu container stays opaque. To prove the fix, check a color that withTransparentBackground clears — e.g. theme.background or theme.panel — on the menu container spans instead.
Prompt To Fix With AI
This is a comment left during a code review.
Path: src/ui/AppHost.interactions.test.tsx
Line: 2421-2423
Comment:
**Menu-opacity assertion doesn't target what the fix changes**
`theme.accentMuted` is not touched by `withTransparentBackground`, so `baseTheme.accentMuted === activeTheme.accentMuted`. This `hasLineWithBackground` check would pass identically before and after the fix, meaning it doesn't actually verify the menu container stays opaque. To prove the fix, check a color that `withTransparentBackground` clears — e.g. `theme.background` or `theme.panel` — on the menu container spans instead.
How can I resolve this? If you propose a fix, please make it concise.| /** Convert captured RGBA output back into a #rrggbb color string for background assertions. */ | ||
| function capturedColorToHex(color: { buffer?: ArrayLike<number> } | undefined) { | ||
| const buffer = color?.buffer; | ||
| if (!buffer || buffer[0] == null || buffer[1] == null || buffer[2] == null) { | ||
| return null; | ||
| } | ||
|
|
||
| const componentToHex = (value: number) => | ||
| Math.max(0, Math.min(255, Math.round(value * 255))) | ||
| .toString(16) | ||
| .padStart(2, "0"); | ||
|
|
||
| return `#${componentToHex(buffer[0])}${componentToHex(buffer[1])}${componentToHex(buffer[2])}`; | ||
| } |
There was a problem hiding this comment.
capturedColorToHex is duplicated from ui-components.test.tsx
The identical helper already exists at line 376 of src/ui/components/ui-components.test.tsx. Extracting it to a shared test-helper module (e.g. test/helpers/color-helpers.ts) would remove the duplication and keep the two implementations in sync if the buffer format ever changes.
Prompt To Fix With AI
This is a comment left during a code review.
Path: src/ui/AppHost.interactions.test.tsx
Line: 455-468
Comment:
**`capturedColorToHex` is duplicated from `ui-components.test.tsx`**
The identical helper already exists at line 376 of `src/ui/components/ui-components.test.tsx`. Extracting it to a shared test-helper module (e.g. `test/helpers/color-helpers.ts`) would remove the duplication and keep the two implementations in sync if the buffer format ever changes.
How can I resolve this? If you propose a fix, please make it concise.Note: If this suggestion doesn't match your team's coding style, reply to this and let me know. I'll remember it for next time!
1 participant
👋 (hope these are welcome)