fix(session): address recent navigation review
- commit
- ef5a4d1b5ae43e3696071ca323d602e0d61a466c
- author
- brice · · 2026-07-05 14:50 UTC
- parent
- 717a6bd
- signature
- GPG Verified
CD88F9E564E0102646FB6077A1A07C7C7C3E349D· arch-gpg Verified for the committer email
5 files · +42 -17
M internal/usecase/daemon/input_test.go +13 -0
| @@ -320,6 +320,19 @@ func TestPaletteBackForwardSessionStaleTrail(t *testing.T) { | |||
| 320 | 320 | runPaletteCommand(t, d, current, ac, "BSK") | |
| 321 | 321 | require.Same(t, newer, ac.currentSession()) | |
| 322 | 322 | }) | |
| 323 | + | ||
| 324 | + | t.Run("failed switch does not advance navigator index", func(t *testing.T) { | |
| 325 | + | d, current, ac, _, releases := newRecentNavigationTestSessions(t) | |
| 326 | + | defer releaseAll(releases) | |
| 327 | + | current.mu.Lock() | |
| 328 | + | current.client = nil | |
| 329 | + | current.mu.Unlock() | |
| 330 | + | ||
| 331 | + | runPaletteCommand(t, d, current, ac, "BSK") | |
| 332 | + | ||
| 333 | + | require.Same(t, current, ac.currentSession()) | |
| 334 | + | require.Zero(t, ac.recentNav.index) | |
| 335 | + | }) | |
| 323 | 336 | } | |
| 324 | 337 | ||
| 325 | 338 | func newRecentNavigationTestSessions(t *testing.T) (*Daemon, *session, *attachedClient, chan ports.Frame, []func()) { | |
| @@ -320,6 +320,19 @@ func TestPaletteBackForwardSessionStaleTrail(t *testing.T) { | |||||
| 320 | runPaletteCommand(t, d, current, ac, "BSK") | 320 | runPaletteCommand(t, d, current, ac, "BSK") | ||
| 321 | require.Same(t, newer, ac.currentSession()) | 321 | require.Same(t, newer, ac.currentSession()) | ||
| 322 | }) | 322 | }) | ||
| 323 | + | ||||
| 324 | + | t.Run("failed switch does not advance navigator index", func(t *testing.T) { | |||
| 325 | + | d, current, ac, _, releases := newRecentNavigationTestSessions(t) | |||
| 326 | + | defer releaseAll(releases) | |||
| 327 | + | current.mu.Lock() | |||
| 328 | + | current.client = nil | |||
| 329 | + | current.mu.Unlock() | |||
| 330 | + | ||||
| 331 | + | runPaletteCommand(t, d, current, ac, "BSK") | |||
| 332 | + | ||||
| 333 | + | require.Same(t, current, ac.currentSession()) | |||
| 334 | + | require.Zero(t, ac.recentNav.index) | |||
| 335 | + | }) | |||
| 323 | } | 336 | } | ||
| 324 | 337 | ||||
| 325 | func newRecentNavigationTestSessions(t *testing.T) (*Daemon, *session, *attachedClient, chan ports.Frame, []func()) { | 338 | func newRecentNavigationTestSessions(t *testing.T) (*Daemon, *session, *attachedClient, chan ports.Frame, []func()) { | ||
M internal/usecase/daemon/recent_nav.go +10 -2
| @@ -46,8 +46,16 @@ func (d *Daemon) navigateRecentSession(sess *session, ac *attachedClient, delta | |||
| 46 | 46 | d.paint(sess, ac, true) | |
| 47 | 47 | return | |
| 48 | 48 | } | |
| 49 | - | ac.recentNav.index = next | |
| 50 | - | d.switchToTarget(sess, ac, picker.Target{Session: ac.recentNav.ids[next], TabIndex: -1}) | |
| 49 | + | targetID := ac.recentNav.ids[next] | |
| 50 | + | targetSess := d.sessionByID(targetID) | |
| 51 | + | if targetSess == nil { | |
| 52 | + | d.paint(sess, ac, true) | |
| 53 | + | return | |
| 54 | + | } | |
| 55 | + | d.switchToTarget(sess, ac, picker.Target{Session: targetID, TabIndex: -1}) | |
| 56 | + | if ac.currentSession() == targetSess { | |
| 57 | + | ac.recentNav.index = next | |
| 58 | + | } | |
| 51 | 59 | } | |
| 52 | 60 | ||
| 53 | 61 | func (d *Daemon) pruneRecentNav(n *recentSessionNavigator) bool { | |
| @@ -46,8 +46,16 @@ func (d *Daemon) navigateRecentSession(sess *session, ac *attachedClient, delta | |||||
| 46 | d.paint(sess, ac, true) | 46 | d.paint(sess, ac, true) | ||
| 47 | return | 47 | return | ||
| 48 | } | 48 | } | ||
| 49 | - | ac.recentNav.index = next | 49 | + | targetID := ac.recentNav.ids[next] |
| 50 | - | d.switchToTarget(sess, ac, picker.Target{Session: ac.recentNav.ids[next], TabIndex: -1}) | 50 | + | targetSess := d.sessionByID(targetID) |
| 51 | + | if targetSess == nil { | |||
| 52 | + | d.paint(sess, ac, true) | |||
| 53 | + | return | |||
| 54 | + | } | |||
| 55 | + | d.switchToTarget(sess, ac, picker.Target{Session: targetID, TabIndex: -1}) | |||
| 56 | + | if ac.currentSession() == targetSess { | |||
| 57 | + | ac.recentNav.index = next | |||
| 58 | + | } | |||
| 51 | } | 59 | } | ||
| 52 | 60 | ||||
| 53 | func (d *Daemon) pruneRecentNav(n *recentSessionNavigator) bool { | 61 | func (d *Daemon) pruneRecentNav(n *recentSessionNavigator) bool { | ||
M internal/usecase/daemon/session_test.go +4 -8
| @@ -705,9 +705,7 @@ func TestTouchMRUConcurrentUpdatesRemainMonotonic(t *testing.T) { | |||
| 705 | 705 | var wg sync.WaitGroup | |
| 706 | 706 | var observer sync.WaitGroup | |
| 707 | 707 | ||
| 708 | - | observer.Add(1) | |
| 709 | - | go func() { | |
| 710 | - | defer observer.Done() | |
| 708 | + | observer.Go(func() { | |
| 711 | 709 | previous := sess.mruAt.Load() | |
| 712 | 710 | for { | |
| 713 | 711 | select { | |
| @@ -722,17 +720,15 @@ func TestTouchMRUConcurrentUpdatesRemainMonotonic(t *testing.T) { | |||
| 722 | 720 | } | |
| 723 | 721 | previous = current | |
| 724 | 722 | } | |
| 725 | - | }() | |
| 723 | + | }) | |
| 726 | 724 | ||
| 727 | 725 | for range goroutines { | |
| 728 | - | wg.Add(1) | |
| 729 | - | go func() { | |
| 730 | - | defer wg.Done() | |
| 726 | + | wg.Go(func() { | |
| 731 | 727 | <-start | |
| 732 | 728 | for range iterations { | |
| 733 | 729 | d.touchMRU(sess) | |
| 734 | 730 | } | |
| 735 | - | }() | |
| 731 | + | }) | |
| 736 | 732 | } | |
| 737 | 733 | close(start) | |
| 738 | 734 | wg.Wait() | |
| @@ -705,9 +705,7 @@ func TestTouchMRUConcurrentUpdatesRemainMonotonic(t *testing.T) { | |||||
| 705 | var wg sync.WaitGroup | 705 | var wg sync.WaitGroup | ||
| 706 | var observer sync.WaitGroup | 706 | var observer sync.WaitGroup | ||
| 707 | 707 | ||||
| 708 | - | observer.Add(1) | 708 | + | observer.Go(func() { |
| 709 | - | go func() { | |||
| 710 | - | defer observer.Done() | |||
| 711 | previous := sess.mruAt.Load() | 709 | previous := sess.mruAt.Load() | ||
| 712 | for { | 710 | for { | ||
| 713 | select { | 711 | select { | ||
| @@ -722,17 +720,15 @@ func TestTouchMRUConcurrentUpdatesRemainMonotonic(t *testing.T) { | |||||
| 722 | } | 720 | } | ||
| 723 | previous = current | 721 | previous = current | ||
| 724 | } | 722 | } | ||
| 725 | - | }() | 723 | + | }) |
| 726 | 724 | ||||
| 727 | for range goroutines { | 725 | for range goroutines { | ||
| 728 | - | wg.Add(1) | 726 | + | wg.Go(func() { |
| 729 | - | go func() { | |||
| 730 | - | defer wg.Done() | |||
| 731 | <-start | 727 | <-start | ||
| 732 | for range iterations { | 728 | for range iterations { | ||
| 733 | d.touchMRU(sess) | 729 | d.touchMRU(sess) | ||
| 734 | } | 730 | } | ||
| 735 | - | }() | 731 | + | }) |
| 736 | } | 732 | } | ||
| 737 | close(start) | 733 | close(start) | ||
| 738 | wg.Wait() | 734 | wg.Wait() | ||
M internal/usecase/daemon/status.go +13 -6
| @@ -244,16 +244,23 @@ func fitMRU(entries []mruSession, rowLen, leftUsed int, feedback string) []mruSe | |||
| 244 | 244 | return entries | |
| 245 | 245 | } | |
| 246 | 246 | ||
| 247 | + | const ( | |
| 248 | + | mruReserveMinRow = 40 | |
| 249 | + | mruReserveDivisor = 4 | |
| 250 | + | mruReserveMin = 12 | |
| 251 | + | mruReserveMax = 24 | |
| 252 | + | ) | |
| 253 | + | ||
| 247 | 254 | func mruFutureRightReserve(rowLen int) int { | |
| 248 | - | if rowLen < 40 { | |
| 255 | + | if rowLen < mruReserveMinRow { | |
| 249 | 256 | return 0 | |
| 250 | 257 | } | |
| 251 | - | reserve := rowLen / 4 | |
| 252 | - | if reserve < 12 { | |
| 253 | - | return 12 | |
| 258 | + | reserve := rowLen / mruReserveDivisor | |
| 259 | + | if reserve < mruReserveMin { | |
| 260 | + | return mruReserveMin | |
| 254 | 261 | } | |
| 255 | - | if reserve > 24 { | |
| 256 | - | return 24 | |
| 262 | + | if reserve > mruReserveMax { | |
| 263 | + | return mruReserveMax | |
| 257 | 264 | } | |
| 258 | 265 | return reserve | |
| 259 | 266 | } | |
| @@ -244,16 +244,23 @@ func fitMRU(entries []mruSession, rowLen, leftUsed int, feedback string) []mruSe | |||||
| 244 | return entries | 244 | return entries | ||
| 245 | } | 245 | } | ||
| 246 | 246 | ||||
| 247 | + | const ( | |||
| 248 | + | mruReserveMinRow = 40 | |||
| 249 | + | mruReserveDivisor = 4 | |||
| 250 | + | mruReserveMin = 12 | |||
| 251 | + | mruReserveMax = 24 | |||
| 252 | + | ) | |||
| 253 | + | ||||
| 247 | func mruFutureRightReserve(rowLen int) int { | 254 | func mruFutureRightReserve(rowLen int) int { | ||
| 248 | - | if rowLen < 40 { | 255 | + | if rowLen < mruReserveMinRow { |
| 249 | return 0 | 256 | return 0 | ||
| 250 | } | 257 | } | ||
| 251 | - | reserve := rowLen / 4 | 258 | + | reserve := rowLen / mruReserveDivisor |
| 252 | - | if reserve < 12 { | 259 | + | if reserve < mruReserveMin { |
| 253 | - | return 12 | 260 | + | return mruReserveMin |
| 254 | } | 261 | } | ||
| 255 | - | if reserve > 24 { | 262 | + | if reserve > mruReserveMax { |
| 256 | - | return 24 | 263 | + | return mruReserveMax |
| 257 | } | 264 | } | ||
| 258 | return reserve | 265 | return reserve | ||
| 259 | } | 266 | } | ||
M internal/usecase/daemon/status_test.go +2 -1
| @@ -290,7 +290,7 @@ func TestBarStateForMRUFreshestFirstCapCurrentExcludedAndAttention(t *testing.T) | |||
| 290 | 290 | defer releasePTY() | |
| 291 | 291 | sess.name = "current" | |
| 292 | 292 | sess.mruAt.Store(100) | |
| 293 | - | for i := 0; i < 10; i++ { | |
| 293 | + | for i := range 10 { | |
| 294 | 294 | other := &session{id: domain.SessionID("s" + strconv.Itoa(i)), name: "s" + strconv.Itoa(i), tabs: []*tab{{attention: i == 8}}} | |
| 295 | 295 | other.mruAt.Store(uint64(i + 1)) | |
| 296 | 296 | d.sessions[other.id] = other | |
| @@ -404,6 +404,7 @@ func TestStatusBarMRUGradientTruecolorAndPlainFallback(t *testing.T) { | |||
| 404 | 404 | ||
| 405 | 405 | plain := mruStyle(renderer.DefaultStyle(), themeui.Theme{}, 1, 3) | |
| 406 | 406 | require.False(t, plain.HasForegroundRGB) | |
| 407 | + | require.False(t, plain.HasBackgroundRGB) | |
| 407 | 408 | } | |
| 408 | 409 | ||
| 409 | 410 | func TestStatusBarNarrowRowsDropWholeOldestMRUEntries(t *testing.T) { | |
| @@ -290,7 +290,7 @@ func TestBarStateForMRUFreshestFirstCapCurrentExcludedAndAttention(t *testing.T) | |||||
| 290 | defer releasePTY() | 290 | defer releasePTY() | ||
| 291 | sess.name = "current" | 291 | sess.name = "current" | ||
| 292 | sess.mruAt.Store(100) | 292 | sess.mruAt.Store(100) | ||
| 293 | - | for i := 0; i < 10; i++ { | 293 | + | for i := range 10 { |
| 294 | other := &session{id: domain.SessionID("s" + strconv.Itoa(i)), name: "s" + strconv.Itoa(i), tabs: []*tab{{attention: i == 8}}} | 294 | other := &session{id: domain.SessionID("s" + strconv.Itoa(i)), name: "s" + strconv.Itoa(i), tabs: []*tab{{attention: i == 8}}} | ||
| 295 | other.mruAt.Store(uint64(i + 1)) | 295 | other.mruAt.Store(uint64(i + 1)) | ||
| 296 | d.sessions[other.id] = other | 296 | d.sessions[other.id] = other | ||
| @@ -404,6 +404,7 @@ func TestStatusBarMRUGradientTruecolorAndPlainFallback(t *testing.T) { | |||||
| 404 | 404 | ||||
| 405 | plain := mruStyle(renderer.DefaultStyle(), themeui.Theme{}, 1, 3) | 405 | plain := mruStyle(renderer.DefaultStyle(), themeui.Theme{}, 1, 3) | ||
| 406 | require.False(t, plain.HasForegroundRGB) | 406 | require.False(t, plain.HasForegroundRGB) | ||
| 407 | + | require.False(t, plain.HasBackgroundRGB) | |||
| 407 | } | 408 | } | ||
| 408 | 409 | ||||
| 409 | func TestStatusBarNarrowRowsDropWholeOldestMRUEntries(t *testing.T) { | 410 | func TestStatusBarNarrowRowsDropWholeOldestMRUEntries(t *testing.T) { | ||