Support responsive height in Lyrics and Themes views#215
Conversation
📝 WalkthroughWalkthroughThis PR refactors and expands an "expanded view" toggle feature across the UI. The ChangesUI Expanded View Toggle Refactor
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
ui/model/view_overlays.go (1)
498-575: 🧹 Nitpick | 🔵 Trivial | ⚡ Quick winDynamic lyrics window looks correct; the
len(lines) - 2arithmetic is the fragile bit.The synced-mode half-window math (
half := visible / 2, clampingendIdx/startIdx) and the scroll-modemin(scroll+visible, len(lines))are both sound, and switching topadLines(lines, visible, rendered)matches the dynamic height nicely.The one fragile spot is line 568:
rendered := len(lines) - 2 // -2 for header and spacingThis silently assumes exactly two leading header lines. Today's renderer does open with
titleStyle.Render("L Y R I C S")+"", so it's correct, but if anyone adds a subtitle/dim line at the top later, padding will drift and the overlay height will jump. Computingrenderedas a counter inside each branch (or capturingheaderLen := len(lines)before the body block and usinglen(lines) - headerLen) would make the coupling explicit.♻️ Less fragile rendered count
func (m Model) renderLyricsOverlay() string { visible := m.lyricsVisibleHeight() lines := []string{ titleStyle.Render("L Y R I C S"), "", } + headerLen := len(lines) if m.lyrics.loading { ... } - rendered := len(lines) - 2 // -2 for header and spacing + rendered := len(lines) - headerLen lines = padLines(lines, visible, rendered)🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@ui/model/view_overlays.go` around lines 498 - 575, renderLyricsOverlay currently computes rendered with a hardcoded "-2" which assumes exactly two header lines; capture the header length explicitly to avoid future breakage by: after the header lines are appended (title + spacing) record headerLen := len(lines), then after the various branches compute rendered := len(lines) - headerLen and pass that to padLines; update references around the existing rendered variable (and keep logic in renderLyricsOverlay and usage of padLines/lyrics.scroll unchanged) so the header size is decoupled from the padding calculation.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@ui/model/lyrics.go`:
- Around line 58-66: The helper lyricsVisibleHeight currently uses a hard-coded
overhead (m.height-4) and a value receiver; change it to use a named constant
(e.g. lyricsOverheadLines = 4) or compute the overhead via the same
lipgloss.Height(probeFrame) probe pattern used by renderLyricsOverlay to keep
renderer and helper in sync, and change the receiver from (m Model) to a pointer
receiver (m *Model) to avoid copying; update references to lyricsVisibleHeight
and ensure renderLyricsOverlay and related functions (lyricsArtistTitle,
lyricsSyncable) use the shared constant or probe logic.
In `@ui/model/overlays.go`:
- Around line 53-99: themePickerVisible/themePickerMaybeAdjustScroll duplicate
logic from keymapVisible/keymapMaybeAdjustScroll and
fbVisible/fbMaybeAdjustScroll; extract the shared behavior into two helpers and
call them from each overlay: add computeOverlayVisible(probeSections []string,
heightExpanded bool, cap int) to build the probe frame, compute fixedHeight and
return the visible limit, and add clampOverlayScroll(cursor *int, scroll *int,
visible, count int) to normalize cursor/scroll bounds and adjust scroll when
cursor moves; then update themePickerVisible to call computeOverlayVisible with
its probeSections and update themePickerMaybeAdjustScroll to call
clampOverlayScroll(&m.themePicker.cursor, &m.themePicker.scroll, visible,
len(m.themes)+1) (and similarly replace logic in keymap*/fb* to call these
helpers).
- Around line 53-55: The footer help line in themePickerHelpLine currently only
shows "↓↑ / Enter / Esc" but must include the newly supported keys from
handleThemeKey (PgUp/PgDn, Ctrl+U/Ctrl+D, Home/g, End/G) and at minimum Ctrl+X
for Expand; update themePickerHelpLine to add helpKey entries for these keys
(e.g., "PgUp/PgDn", "Ctrl+U/Ctrl+D", "Home/g", "End/G", "Ctrl+X") so the footer
reflects the responsive-height and paging controls and improves discoverability.
---
Outside diff comments:
In `@ui/model/view_overlays.go`:
- Around line 498-575: renderLyricsOverlay currently computes rendered with a
hardcoded "-2" which assumes exactly two header lines; capture the header length
explicitly to avoid future breakage by: after the header lines are appended
(title + spacing) record headerLen := len(lines), then after the various
branches compute rendered := len(lines) - headerLen and pass that to padLines;
update references around the existing rendered variable (and keep logic in
renderLyricsOverlay and usage of padLines/lyrics.scroll unchanged) so the header
size is decoupled from the padding calculation.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: ASSERTIVE
Plan: Pro
Run ID: 3214f1d1-c1eb-45a4-8897-ee809e51138b
📒 Files selected for processing (7)
ui/model/filebrowser.goui/model/keymap.goui/model/keys.goui/model/lyrics.goui/model/overlays.goui/model/state.goui/model/view_overlays.go
| // lyricsVisibleHeight returns the number of lyrics lines to show. | ||
| func (m Model) lyricsVisibleHeight() int { | ||
| limit := maxPlVisible | ||
| if m.heightExpanded { | ||
| limit = m.height | ||
| } | ||
| // Fixed overhead: header (2) + spacing/footer (2) = 4. | ||
| return max(3, min(limit, m.height-4)) | ||
| } |
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial | 💤 Low value
Helper looks correct; just verify the overhead constant stays in sync with the renderer.
max(3, min(limit, m.height-4)) mirrors the structure used by keymapVisible / fbVisible / themePickerVisible. The hard-coded 4 corresponds to the renderer's "title (1) + blank (1) + blank (1) + helpKey footer (1)" overhead. That's currently correct (renderLyricsOverlay in view_overlays.go opens with titleStyle… + "" and closes with "" + helpKey), but it's an implicit coupling — if anyone adds, say, a subtitle line to the header the overhead drifts silently.
Two small things to consider:
- Tie the constant to a named value (e.g.
lyricsOverheadLines = 4) shared with the renderer, or compute it via the samelipgloss.Height(probeFrame)probe pattern the other overlays use. - The receiver here is value (
m Model) whilelyricsArtistTitle/lyricsSyncablein the same file use pointer receivers. Minor, but(m *Model)would avoid copying the model on every render call.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@ui/model/lyrics.go` around lines 58 - 66, The helper lyricsVisibleHeight
currently uses a hard-coded overhead (m.height-4) and a value receiver; change
it to use a named constant (e.g. lyricsOverheadLines = 4) or compute the
overhead via the same lipgloss.Height(probeFrame) probe pattern used by
renderLyricsOverlay to keep renderer and helper in sync, and change the receiver
from (m Model) to a pointer receiver (m *Model) to avoid copying; update
references to lyricsVisibleHeight and ensure renderLyricsOverlay and related
functions (lyricsArtistTitle, lyricsSyncable) use the shared constant or probe
logic.
| func (m *Model) themePickerHelpLine() string { | ||
| return helpKey("↓↑", "Scroll ") + helpKey("Enter", "Select ") + helpKey("Esc", "Close") | ||
| } | ||
|
|
||
| func (m *Model) themePickerVisible() int { | ||
| probeSections := []string{ | ||
| titleStyle.Render("T H E M E S"), | ||
| "", | ||
| "x", // 1-line list placeholder | ||
| "", | ||
| dimStyle.Render(" 0/0 themes"), | ||
| "", | ||
| m.themePickerHelpLine(), | ||
| } | ||
|
|
||
| probeFrame := ui.FrameStyle.Render(strings.Join(probeSections, "\n")) | ||
| fixedHeight := lipgloss.Height(probeFrame) - 1 | ||
|
|
||
| limit := maxPlVisible | ||
| if m.heightExpanded { | ||
| limit = m.height | ||
| } | ||
| return max(3, min(limit, m.height-fixedHeight)) | ||
| } | ||
|
|
||
| func (m *Model) themePickerMaybeAdjustScroll(visible int) { | ||
| if visible <= 0 { | ||
| return | ||
| } | ||
| count := len(m.themes) + 1 | ||
| if m.themePicker.cursor < 0 { | ||
| m.themePicker.cursor = 0 | ||
| } | ||
| if m.themePicker.cursor >= count && count > 0 { | ||
| m.themePicker.cursor = count - 1 | ||
| } | ||
|
|
||
| if m.themePicker.cursor < m.themePicker.scroll { | ||
| m.themePicker.scroll = m.themePicker.cursor | ||
| } else if m.themePicker.cursor >= m.themePicker.scroll+visible { | ||
| m.themePicker.scroll = m.themePicker.cursor - visible + 1 | ||
| } | ||
|
|
||
| if m.themePicker.scroll+visible > count && count > 0 { | ||
| m.themePicker.scroll = max(0, count-visible) | ||
| } | ||
| } |
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial | ⚖️ Poor tradeoff
Theme-picker visibility/scroll helpers are correct but largely duplicate the keymap & file browser pattern.
themePickerVisible and themePickerMaybeAdjustScroll are near-line-for-line copies of keymapVisible/keymapMaybeAdjustScroll and fbVisible/fbMaybeAdjustScroll, only differing in the probe sections, the count source, and the cursor/scroll fields. With three overlays (and growing) reusing the same shape, a small generic helper — e.g. clampOverlayScroll(cursor, scroll *int, visible, count int) and a computeOverlayVisible(probeSections []string, heightExpanded bool, cap int) — would remove the drift risk if any of these handlers diverges.
Not blocking — just worth keeping in mind as the next overlay is added.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@ui/model/overlays.go` around lines 53 - 99,
themePickerVisible/themePickerMaybeAdjustScroll duplicate logic from
keymapVisible/keymapMaybeAdjustScroll and fbVisible/fbMaybeAdjustScroll; extract
the shared behavior into two helpers and call them from each overlay: add
computeOverlayVisible(probeSections []string, heightExpanded bool, cap int) to
build the probe frame, compute fixedHeight and return the visible limit, and add
clampOverlayScroll(cursor *int, scroll *int, visible, count int) to normalize
cursor/scroll bounds and adjust scroll when cursor moves; then update
themePickerVisible to call computeOverlayVisible with its probeSections and
update themePickerMaybeAdjustScroll to call
clampOverlayScroll(&m.themePicker.cursor, &m.themePicker.scroll, visible,
len(m.themes)+1) (and similarly replace logic in keymap*/fb* to call these
helpers).
| func (m *Model) themePickerHelpLine() string { | ||
| return helpKey("↓↑", "Scroll ") + helpKey("Enter", "Select ") + helpKey("Esc", "Close") | ||
| } |
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial | ⚡ Quick win
Help line omits the new keys you just wired up.
handleThemeKey now supports pgup/pgdn, ctrl+u/d, home/g, end/G, and ctrl+x, but the footer only advertises ↓↑ / Enter / Esc. Discoverability for the new responsive-height + paging behavior suffers. Consider extending — at minimum mention Ctrl+X (Expand) since that's the headline of this PR.
♻️ Suggested footer hint additions
func (m *Model) themePickerHelpLine() string {
- return helpKey("↓↑", "Scroll ") + helpKey("Enter", "Select ") + helpKey("Esc", "Close")
+ return helpKey("↓↑", "Scroll ") + helpKey("PgUp/Dn", "Page ") +
+ helpKey("Ctrl+X", "Expand ") + helpKey("Enter", "Select ") + helpKey("Esc", "Close")
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| func (m *Model) themePickerHelpLine() string { | |
| return helpKey("↓↑", "Scroll ") + helpKey("Enter", "Select ") + helpKey("Esc", "Close") | |
| } | |
| func (m *Model) themePickerHelpLine() string { | |
| return helpKey("↓↑", "Scroll ") + helpKey("PgUp/Dn", "Page ") + | |
| helpKey("Ctrl+X", "Expand ") + helpKey("Enter", "Select ") + helpKey("Esc", "Close") | |
| } |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@ui/model/overlays.go` around lines 53 - 55, The footer help line in
themePickerHelpLine currently only shows "↓↑ / Enter / Esc" but must include the
newly supported keys from handleThemeKey (PgUp/PgDn, Ctrl+U/Ctrl+D, Home/g,
End/G) and at minimum Ctrl+X for Expand; update themePickerHelpLine to add
helpKey entries for these keys (e.g., "PgUp/PgDn", "Ctrl+U/Ctrl+D", "Home/g",
"End/G", "Ctrl+X") so the footer reflects the responsive-height and paging
controls and improves discoverability.
2 participants
This PR improves the Lyrics and Themes views by adding support for the expanded-view setting
ctrl+x.Additionally, the Themes view now adopts the same scrolling behavior as the Keymap and File Browser views.
There is a small refactor to rename
toggleExpandPlaylistto the more accuratetoggleExpandedView.Summary by CodeRabbit