Fix expanded-height drift#217
Conversation
📝 WalkthroughWalkthroughThis PR refactors overlay scrolling and visibility management. It adds a ChangesOverlay Scrolling and Height Management
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: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
ui/model/update.go (1)
433-458:⚠️ Potential issue | 🟡 Minor | ⚡ Quick win
feedsLoadedMsgis missingapplyHeightMode()/adjustScroll()afterm.playlist.Add(), inconsistent with every other playlist-mutation handler.
tracksLoadedMsg(line 307),feedTrackResolvedMsg(line 426), andfbTracksResolvedMsg(line 500) all callapplyHeightMode()/adjustScroll(). The PR description explicitly lists "feed load" among the events that should trigger recalculation. Without the call here, the playlist scroll position can drift after URL feed loading completes (especially when transitioning fromfeedLoading=trueto showing real tracks while in expanded-height mode).🐛 Proposed fix
case feedsLoadedMsg: m.feedLoading = false if len(msg.tracks) > 0 { m.playlist.Add(msg.tracks...) m.status.Showf(statusTTLDefault, "Loaded %d track(s)", len(msg.tracks)) } else { m.status.Show("No tracks found at URL.", statusTTLDefault) } if len(msg.tracks) > 0 { + m.applyHeightMode() + m.adjustScroll() batchCmd := m.initYTDLBatch(msg.urls)🤖 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/update.go` around lines 433 - 458, The feedsLoadedMsg handler updates the playlist via m.playlist.Add(...) but omits calling applyHeightMode() and adjustScroll(), causing scroll drift; after adding tracks in the feedsLoadedMsg case (immediately after m.playlist.Add(msg.tracks...)), call m.applyHeightMode() and m.adjustScroll() before any play/notify/return logic (keep the existing m.initYTDLBatch(msg.urls) and autoplay handling intact).
🤖 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 57-66: The lyricsVisibleHeight function hardcodes an overhead of 4
which breaks the probe-based pattern used elsewhere; replace the hardcoded
subtraction with a probe-frame height calculation like the other visible-height
helpers: build a representative probe of the lyrics overlay (mirroring
renderLyricsOverlay's header/top spacing and footer/help key structure), call
lipgloss.Height on that probe, and use that value instead of the literal 4 when
computing return max(3, min(limit, m.height - probeHeight)); keep references to
maxPlVisible, m.heightExpanded and m.height as in the current function.
---
Outside diff comments:
In `@ui/model/update.go`:
- Around line 433-458: The feedsLoadedMsg handler updates the playlist via
m.playlist.Add(...) but omits calling applyHeightMode() and adjustScroll(),
causing scroll drift; after adding tracks in the feedsLoadedMsg case
(immediately after m.playlist.Add(msg.tracks...)), call m.applyHeightMode() and
m.adjustScroll() before any play/notify/return logic (keep the existing
m.initYTDLBatch(msg.urls) and autoplay handling intact).
🪄 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: dc6d10b8-2b44-4272-aa42-8b02f4e9cf23
📒 Files selected for processing (10)
ui/model/filebrowser.goui/model/keymap.goui/model/keys.goui/model/lyrics.goui/model/overlays.goui/model/scroll.goui/model/state.goui/model/update.goui/model/view.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
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Locate renderLyricsOverlay and inspect its structure to count fixed sections.
ast-grep --pattern 'func ($_ Model) renderLyricsOverlay() string {
$$$
}'Repository: bjarneo/cliamp
Length of output: 4845
Hardcoded overhead - 4 is correct but deviates from the probe-based pattern used elsewhere.
The fixed overhead of 4 lines is accurate: renderLyricsOverlay has header (1) + spacing (1) at the top and footer spacing (1) + help key (1) at the bottom. However, this implementation diverges from the established pattern used by every other visible-height calculator (themePickerVisible, keymapVisible, fbVisible, etc.), which render a representative probe frame with lipgloss.Height() to stay resilient to layout changes.
If the lyrics overlay structure ever changes, this hardcoded value will silently become incorrect. Aligning with the probe-based pattern would eliminate that fragility:
♻️ Probe-based refactor
-// 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))
-}
+// lyricsVisibleHeight returns the number of lyrics lines to show.
+func (m *Model) lyricsVisibleHeight() int {
+ probeSections := []string{
+ titleStyle.Render("L Y R I C S"),
+ "",
+ "x",
+ "",
+ helpKey("Esc", "Close"),
+ }
+ probeFrame := ui.FrameStyle.Render(strings.Join(probeSections, "\n"))
+ fixedHeight := lipgloss.Height(probeFrame)
+ limit := maxPlVisible
+ if m.heightExpanded {
+ limit = m.height
+ }
+ return max(3, min(limit, m.height-fixedHeight))
+}🤖 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 57 - 66, The lyricsVisibleHeight function
hardcodes an overhead of 4 which breaks the probe-based pattern used elsewhere;
replace the hardcoded subtraction with a probe-frame height calculation like the
other visible-height helpers: build a representative probe of the lyrics overlay
(mirroring renderLyricsOverlay's header/top spacing and footer/help key
structure), call lipgloss.Height on that probe, and use that value instead of
the literal 4 when computing return max(3, min(limit, m.height - probeHeight));
keep references to maxPlVisible, m.heightExpanded and m.height as in the current
function.
|
Thank you for both. Looks like it will be fine. Great additions! |
2 participants
Note
This PR should be stacked on PR #215, but unfortunately it looks like this PR also includes the commits from PR #215. If PR #215 is approved to be merged, then I think I can rebase this branch on main afterward so those commits won't be on this PR. Github might be smart enough to do this automatically if PR #215 is merged. Apologies for this confusion.
Summary
Fixes expanded-height
ctrl+xdrift by recalculating playlist viewport height/scroll when dynamic UI rows change (provider pill, status/log/footer, stream title, and playlist load events), and by clamping/padding empty states to the visible budget.Changes
Summary by CodeRabbit
New Features
UI/UX Improvements