feat(ui): show selected theme in footer#396
Conversation
Greptile SummaryThis PR routes all theme-switching (both keyboard cycling with
Confidence Score: 4/5Safe to merge; the logic change is small and correctly unified through a single callback. The implementation is clean and the dependency arrays are correctly updated. The only rough edge is the new test hardcoding "Paper" and "Ember" against an implicit dependency on THEMES ordering and the test-helper default theme, which could cause silent failures if either changes. src/ui/AppHost.interactions.test.tsx — the hardcoded theme name expectations are fragile. Important Files Changed
Sequence DiagramsequenceDiagram
participant User
participant cycleTheme
participant ThemeMenu
participant selectTheme
participant setThemeId
participant showTransientNotice
User->>cycleTheme: press "t"
cycleTheme->>selectTheme: selectTheme(nextThemeId)
selectTheme->>setThemeId: setThemeId(nextThemeId)
selectTheme->>showTransientNotice: showTransientNotice("Theme: Paper")
User->>ThemeMenu: select theme
ThemeMenu->>selectTheme: selectTheme(theme.id)
selectTheme->>setThemeId: setThemeId(theme.id)
selectTheme->>showTransientNotice: showTransientNotice("Theme: Ember")
Prompt To Fix All With AIFix the following 1 code review issue. Work through them one at a time, proposing concise fixes.
---
### Issue 1 of 1
src/ui/AppHost.interactions.test.tsx:700-707
**Hardcoded theme names couple test to THEMES ordering**
`"Paper"` and `"Ember"` are correct today because `createTestVcsAppBootstrap` defaults to `initialTheme: "midnight"` (index 1 in `THEMES`), so one `t` press lands on index 2 ("Paper") and a second on index 3 ("Ember"). If the default initial theme or the order of entries in `THEMES` ever changes, these expectations will silently fail or erroneously pass. Consider deriving the expected labels from `themeOptions` at test-setup time rather than embedding literal strings.
Reviews (1): Last reviewed commit: "feat(ui): show selected theme in footer" | Re-trigger Greptile |
| let frame = await waitForFrame(setup, (nextFrame) => nextFrame.includes("Theme: Paper")); | ||
| expect(frame).toContain("Theme: Paper"); | ||
|
|
||
| await act(async () => { | ||
| await setup.mockInput.typeText("t"); | ||
| }); | ||
| frame = await waitForFrame(setup, (nextFrame) => nextFrame.includes("Theme: Ember")); | ||
| expect(frame).toContain("Theme: Ember"); |
There was a problem hiding this comment.
"Paper" and "Ember" are correct today because createTestVcsAppBootstrap defaults to initialTheme: "midnight" (index 1 in THEMES), so one t press lands on index 2 ("Paper") and a second on index 3 ("Ember"). If the default initial theme or the order of entries in THEMES ever changes, these expectations will silently fail or erroneously pass. Consider deriving the expected labels from themeOptions at test-setup time rather than embedding literal strings.
Prompt To Fix With AI
This is a comment left during a code review.
Path: src/ui/AppHost.interactions.test.tsx
Line: 700-707
Comment:
**Hardcoded theme names couple test to THEMES ordering**
`"Paper"` and `"Ember"` are correct today because `createTestVcsAppBootstrap` defaults to `initialTheme: "midnight"` (index 1 in `THEMES`), so one `t` press lands on index 2 ("Paper") and a second on index 3 ("Ember"). If the default initial theme or the order of entries in `THEMES` ever changes, these expectations will silently fail or erroneously pass. Consider deriving the expected labels from `themeOptions` at test-setup time rather than embedding literal strings.
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!
Summary
tTests
bun test src/ui/AppHost.interactions.test.tsx --timeout 20000bun run typecheckThis PR description was generated by Pi using OpenAI GPT-5