Skip to content

Commit 19bf5d8

Browse files
committed
fix(tui): address slack-handling review findings
Apply review feedback on the bottom-slack work: - Rename onAnimationTick to handleAnimationTick to match the surrounding handle* convention (handleMouseClick, handleKeyPress, etc.). - Apply maxBottomSlack() to the public AdjustBottomSlack API so external callers cannot bypass the cap that protects the viewport from going mostly empty. - Strengthen TestBottomSlackAnimationSubscribesWhileDecaying so it also asserts that Update returns a non-nil tick command — without it, the subscription would be marked active but no tick would ever be scheduled. - Add TestBottomSlackDecayPausesWhenUserScrollsAway covering the case where the user scrolls away mid-decay. - Add TestAdjustBottomSlackRespectsCapAndFloor covering the new cap and the existing floor on AdjustBottomSlack. Assisted-By: docker-agent
1 parent 355ab32 commit 19bf5d8

2 files changed

Lines changed: 67 additions & 12 deletions

File tree

pkg/tui/components/messages/messages.go

Lines changed: 10 additions & 10 deletions
Original file line numberDiff line numberDiff line change
@@ -277,7 +277,7 @@ func (m *model) Update(msg tea.Msg) (layout.Model, tea.Cmd) {
277277
// applied their fade state, and before tui.go's HasActive() check so the
278278
// subscription is registered when the next tick is scheduled.
279279
if _, ok := msg.(animation.TickMsg); ok {
280-
cmds = append(cmds, m.onAnimationTick())
280+
cmds = append(cmds, m.handleAnimationTick())
281281
}
282282

283283
return m, tea.Batch(cmds...)
@@ -526,9 +526,9 @@ func (m *model) View() string {
526526
}
527527

528528
m.updateScrollState()
529-
// Release the slack subscription once it's no longer needed. The reverse
530-
// (starting it) is only done from Update via onAnimationTick, where the
531-
// returned tea.Cmd can be propagated to actually schedule the next tick.
529+
// Release the slack subscription once it's no longer needed. Starting it
530+
// is only done from Update via handleAnimationTick, where the returned
531+
// tea.Cmd can be propagated to actually schedule the next tick.
532532
if m.bottomSlack == 0 {
533533
m.slackAnimationSub.Stop()
534534
}
@@ -614,11 +614,11 @@ func (m *model) maxBottomSlack() int {
614614
return max(1, min(5, m.height/3))
615615
}
616616

617-
// onAnimationTick refreshes scroll state, decays any leftover slack by one
618-
// line, and keeps the slack subscription alive while slack > 0 so further
619-
// ticks fire even after fade animations finish. Returns the command to
620-
// schedule the next tick when the subscription transitions to active.
621-
func (m *model) onAnimationTick() tea.Cmd {
617+
// handleAnimationTick refreshes scroll state, decays any leftover slack by
618+
// one line, and keeps the slack subscription alive while slack > 0 so
619+
// further ticks fire even after fade animations finish. Returns the command
620+
// to schedule the next tick when the subscription transitions to active.
621+
func (m *model) handleAnimationTick() tea.Cmd {
622622
m.updateScrollState()
623623
if !m.userHasScrolled && m.bottomSlack > 0 {
624624
m.bottomSlack--
@@ -1584,7 +1584,7 @@ func (m *model) AdjustBottomSlack(delta int) {
15841584
if delta == 0 {
15851585
return
15861586
}
1587-
m.bottomSlack = max(0, m.bottomSlack+delta)
1587+
m.bottomSlack = max(0, min(m.bottomSlack+delta, m.maxBottomSlack()))
15881588
}
15891589

15901590
// contentWidth returns the width available for content.

pkg/tui/components/messages/slack_test.go

Lines changed: 57 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -111,11 +111,14 @@ func TestBottomSlackAnimationSubscribesWhileDecaying(t *testing.T) {
111111
"no slack means no animation subscription")
112112

113113
// Slack > 0: a tick should subscribe so further ticks keep firing
114-
// even after fade animations finish.
114+
// even after fade animations finish, and Update must return the
115+
// scheduling command so Bubbletea actually delivers the next tick.
115116
m.bottomSlack = 2
116-
m.Update(animation.TickMsg{Frame: 1})
117+
_, cmd := m.Update(animation.TickMsg{Frame: 1})
117118
assert.True(t, m.slackAnimationSub.IsActive(),
118119
"subscription should be active while slack > 0")
120+
assert.NotNil(t, cmd,
121+
"Update must return a tick command so the next tick is scheduled")
119122

120123
// Once slack hits zero, the subscription must release the global tick.
121124
m.Update(animation.TickMsg{Frame: 2})
@@ -198,3 +201,55 @@ func TestBottomSlackIsZeroWhenUserHasScrolledAway(t *testing.T) {
198201
assert.Equal(t, 0, m.bottomSlack,
199202
"slack must remain zero when the user scrolled away from the bottom")
200203
}
204+
205+
func TestBottomSlackDecayPausesWhenUserScrollsAway(t *testing.T) {
206+
t.Parallel()
207+
208+
sessionState := &service.SessionState{}
209+
m := NewScrollableView(80, 24, sessionState).(*model)
210+
m.SetSize(80, 24)
211+
212+
addShrinkingView(m, 10)
213+
m.View()
214+
215+
// Pretend a previous shrinkage left some slack while auto-following.
216+
m.bottomSlack = 3
217+
218+
// First tick decays one line as expected.
219+
m.Update(animation.TickMsg{Frame: 1})
220+
require.Equal(t, 2, m.bottomSlack)
221+
222+
// User scrolls away mid-decay. updateScrollState resets slack to zero
223+
// for the userHasScrolled path; the next tick should leave it there
224+
// and not produce a negative value.
225+
m.userHasScrolled = true
226+
m.Update(animation.TickMsg{Frame: 2})
227+
assert.Equal(t, 0, m.bottomSlack,
228+
"slack must drop to zero (not be decayed below it) once the user scrolls away")
229+
}
230+
231+
func TestAdjustBottomSlackRespectsCapAndFloor(t *testing.T) {
232+
t.Parallel()
233+
234+
sessionState := &service.SessionState{}
235+
m := NewScrollableView(80, 24, sessionState).(*model)
236+
m.SetSize(80, 24)
237+
238+
maxSlack := m.maxBottomSlack()
239+
240+
// Adding more than the cap must clamp to the cap.
241+
m.AdjustBottomSlack(100)
242+
assert.Equal(t, maxSlack, m.bottomSlack,
243+
"AdjustBottomSlack must clamp positive deltas to maxBottomSlack")
244+
245+
// Subtracting below zero must clamp to zero.
246+
m.AdjustBottomSlack(-100)
247+
assert.Equal(t, 0, m.bottomSlack,
248+
"AdjustBottomSlack must clamp negative deltas at zero")
249+
250+
// Zero delta is a no-op.
251+
m.bottomSlack = 2
252+
m.AdjustBottomSlack(0)
253+
assert.Equal(t, 2, m.bottomSlack,
254+
"AdjustBottomSlack(0) must leave slack unchanged")
255+
}

0 commit comments

Comments
 (0)