diff --git a/.github/workflows/release.yml b/.github/workflows/release.yml index 461afc0..5c9a38c 100644 --- a/.github/workflows/release.yml +++ b/.github/workflows/release.yml @@ -20,7 +20,7 @@ jobs: - name: Set up Go uses: actions/setup-go@v5 with: - go-version: "1.23" + go-version-file: go.mod cache: false - name: Run GoReleaser diff --git a/.github/workflows/test.yml b/.github/workflows/test.yml index a48af9b..9c7a2b3 100644 --- a/.github/workflows/test.yml +++ b/.github/workflows/test.yml @@ -14,38 +14,37 @@ jobs: fail-fast: false # Continue running other jobs even if one fails matrix: os: [ubuntu-latest, macos-latest] - go-version: ['1.21', '1.22'] - + steps: - name: Checkout code uses: actions/checkout@v4 - + - name: Set up Go uses: actions/setup-go@v5 with: - go-version: ${{ matrix.go-version }} - + go-version-file: go.mod + - name: Verify Go installation run: go version - + - name: Download dependencies run: go mod download - + - name: Verify dependencies run: go mod verify - + - name: Run tests run: go test -v -race -coverprofile=coverage.out ./... - + - name: Generate coverage report run: go tool cover -html=coverage.out -o coverage.html - + - name: Upload coverage report uses: actions/upload-artifact@v4 with: - name: coverage-${{ matrix.os }}-go${{ matrix.go-version }} + name: coverage-${{ matrix.os }} path: coverage.html - + - name: Check test coverage run: | coverage=$(go tool cover -func=coverage.out | grep total | awk '{print $3}' | sed 's/%//') @@ -55,33 +54,33 @@ jobs: # echo "Coverage is below 50%" # exit 1 # fi - + - name: Test summary if: always() run: | - echo "Tests completed for ${{ matrix.os }} with Go ${{ matrix.go-version }}" + echo "Tests completed for ${{ matrix.os }}" build: runs-on: ${{ matrix.os }} strategy: matrix: os: [ubuntu-latest, macos-latest] - + steps: - name: Checkout code uses: actions/checkout@v4 - + - name: Set up Go uses: actions/setup-go@v5 with: - go-version: '1.22' - + go-version-file: go.mod + - name: Build binary run: go build -v -o tera ./cmd/tera - + - name: Verify binary run: './tera --version || echo "Note: --version flag may not be implemented yet"' - + - name: Build summary run: | echo "Build completed for ${{ matrix.os }}" @@ -89,16 +88,16 @@ jobs: lint: runs-on: ubuntu-latest - + steps: - name: Checkout code uses: actions/checkout@v4 - + - name: Set up Go uses: actions/setup-go@v5 with: - go-version: '1.22' - + go-version-file: go.mod + - name: Run golangci-lint uses: golangci/golangci-lint-action@v6 with: diff --git a/.gitignore b/.gitignore index a34ce82..5da79f9 100644 --- a/.gitignore +++ b/.gitignore @@ -7,7 +7,7 @@ .env # AI development documents -ai-documents +ai-docs /tera /coverage.out \ No newline at end of file diff --git a/internal/timer/sleep_timer.go b/internal/timer/sleep_timer.go index c7c4b2c..bd17f14 100644 --- a/internal/timer/sleep_timer.go +++ b/internal/timer/sleep_timer.go @@ -27,7 +27,11 @@ func NewSleepTimer(onExpire func()) *SleepTimer { func (s *SleepTimer) Start(d time.Duration) { s.mu.Lock() defer s.mu.Unlock() + s.startLocked(d) +} +// startLocked implements Start. The caller must hold s.mu. +func (s *SleepTimer) startLocked(d time.Duration) { // Cancel any existing timer if s.cancelFn != nil { s.cancelFn() @@ -44,6 +48,12 @@ func (s *SleepTimer) Start(d time.Duration) { select { case <-t.C: s.mu.Lock() + // The timer may have been cancelled or replaced after t.C became + // ready but before we got the lock; a stale timer must not fire. + if ctx.Err() != nil { + s.mu.Unlock() + return + } s.active = false s.mu.Unlock() if s.onExpire != nil { @@ -69,18 +79,17 @@ func (s *SleepTimer) Cancel() { } // Extend adds d to the current deadline. If no timer is active, it is a no-op. +// The deadline is read and the timer restarted under a single lock, so the +// timer cannot fire in between. func (s *SleepTimer) Extend(d time.Duration) { s.mu.Lock() + defer s.mu.Unlock() if !s.active { - s.mu.Unlock() return } - remaining := time.Until(s.expiresAt) + d - s.mu.Unlock() - - s.Start(remaining) + s.startLocked(time.Until(s.expiresAt) + d) } // Remaining returns the time left and whether a timer is currently active. diff --git a/internal/timer/sleep_timer_test.go b/internal/timer/sleep_timer_test.go index f28ef84..3657468 100644 --- a/internal/timer/sleep_timer_test.go +++ b/internal/timer/sleep_timer_test.go @@ -46,23 +46,41 @@ func TestTimerIdempotentCancel(t *testing.T) { } // TestTimerExtend verifies that Extend postpones the expiry. +// +// Timing margins are deliberately wide (>=100ms) and the deadline check uses +// Remaining() rather than sleeping across the original expiry, because CI +// runners (especially macOS with -race) can oversleep by tens of milliseconds. func TestTimerExtend(t *testing.T) { + const d = 300 * time.Millisecond + var fired atomic.Bool st := NewSleepTimer(func() { fired.Store(true) }) - st.Start(80 * time.Millisecond) + st.Start(d) + st.Extend(d) - // Extend before expiry - time.Sleep(40 * time.Millisecond) - st.Extend(80 * time.Millisecond) + // Deterministic check: the deadline must now be beyond the original duration. + rem, active := st.Remaining() + if !active { + t.Fatal("timer should still be active after Extend") + } + if rem <= d { + t.Errorf("expected remaining > %v after Extend, got %v", d, rem) + } + extendedDeadline := time.Now().Add(rem) - // At the original expiry the timer should NOT have fired yet - time.Sleep(60 * time.Millisecond) - if fired.Load() { + // Past the original expiry (300ms) but before the extended one (600ms). + time.Sleep(d + 100*time.Millisecond) + // Only a genuine early fire is a bug; if the sleep overshot past the + // extended deadline, a fired callback is correct, not an error. + if fired.Load() && time.Now().Before(extendedDeadline) { t.Error("timer fired too early after Extend") } - // After the extended expiry it should have fired - time.Sleep(100 * time.Millisecond) + // It must still fire eventually; poll instead of a fixed sleep. + deadline := time.Now().Add(2 * time.Second) + for !fired.Load() && time.Now().Before(deadline) { + time.Sleep(10 * time.Millisecond) + } if !fired.Load() { t.Error("timer did not fire after extended duration") } diff --git a/internal/ui/most_played.go b/internal/ui/most_played.go index 5f52b69..ffedddb 100644 --- a/internal/ui/most_played.go +++ b/internal/ui/most_played.go @@ -198,7 +198,7 @@ func createMostPlayedHelp() []components.HelpSection { { Title: "Now Playing", Items: []components.HelpItem{ - {Key: "p", Description: "Pause/Resume"}, + {Key: "Space/p", Description: "Pause/Resume"}, {Key: "s", Description: "Stop"}, {Key: "+/-", Description: "Adjust volume"}, {Key: "r", Description: "Rate station (1-5)"}, @@ -532,10 +532,15 @@ func (m MostPlayedModel) handlePlayingInput(msg tea.KeyMsg) (MostPlayedModel, te } return m, nil - case "p": - // Toggle pause + case "p", " ": + // Toggle pause (Space matches the other player screens; p kept as alias) if m.player != nil { - _ = m.player.TogglePause() + if err := m.player.TogglePause(); err != nil { + m.saveMessage = fmt.Sprintf("✗ Pause failed: %v", err) + m.saveMessageSuccess = false + m.saveMessageTime = 3 + return m, tickEverySecond() + } } return m, nil @@ -915,7 +920,7 @@ func (m MostPlayedModel) viewPlaying() string { } } - helpText := "p: Pause • s: Stop • r: Rate • t: Tag • f: Fav • 0: Main Menu • ?: Help • Esc: Back" + helpText := "Space/p: Pause • s: Stop • r: Rate • t: Tag • f: Fav • 0: Main Menu • ?: Help • Esc: Back" return m.renderPageWithBottomHelp(PageLayout{ Title: "📊 Most Played - Now Playing", Content: content.String(), diff --git a/internal/ui/pause_key_test.go b/internal/ui/pause_key_test.go new file mode 100644 index 0000000..18fff0b --- /dev/null +++ b/internal/ui/pause_key_test.go @@ -0,0 +1,55 @@ +package ui + +import ( + "strings" + "testing" + + tea "github.com/charmbracelet/bubbletea" + "github.com/shinokada/tera/v3/internal/api" + "github.com/shinokada/tera/v3/internal/player" +) + +// spaceKey is the key message Bubble Tea sends for the space bar. +var spaceKey = tea.KeyMsg{Type: tea.KeySpace, Runes: []rune{' '}} + +// TestMostPlayed_SpaceHandledWhilePlaying verifies Space is routed to the +// pause handler (an idle player yields a visible "Pause failed" message +// instead of the key being silently ignored). +func TestMostPlayed_SpaceHandledWhilePlaying(t *testing.T) { + m := NewMostPlayedModel(nil, t.TempDir(), nil) + m.state = mostPlayedStatePlaying + m.selectedStation = &api.Station{StationUUID: "u1", Name: "Test"} + + got, cmd := m.handlePlayingInput(spaceKey) + + if !strings.Contains(got.saveMessage, "Pause failed") { + t.Errorf("expected Space to reach TogglePause, saveMessage=%q", got.saveMessage) + } + if cmd == nil { + t.Error("expected a tick command to clear the message") + } + if got.state != mostPlayedStatePlaying { + t.Errorf("state changed unexpectedly: %v", got.state) + } +} + +// TestTopRated_SpaceHandledWhilePlaying verifies Space is routed to the +// pause handler on the Top Rated screen. +func TestTopRated_SpaceHandledWhilePlaying(t *testing.T) { + m := NewTopRatedModel(nil, nil, nil, t.TempDir(), nil) + m.player = player.NewMPVPlayer() + m.state = topRatedStatePlaying + m.selectedStation = &api.Station{StationUUID: "u1", Name: "Test"} + + got, cmd := m.handlePlayingInput(spaceKey) + + if !strings.Contains(got.saveMessage, "Pause failed") { + t.Errorf("expected Space to reach TogglePause, saveMessage=%q", got.saveMessage) + } + if cmd == nil { + t.Error("expected a tick command to clear the message") + } + if got.state != topRatedStatePlaying { + t.Errorf("state changed unexpectedly: %v", got.state) + } +} diff --git a/internal/ui/top_rated.go b/internal/ui/top_rated.go index 5340d78..85c1576 100644 --- a/internal/ui/top_rated.go +++ b/internal/ui/top_rated.go @@ -653,6 +653,18 @@ func (m TopRatedModel) handlePlayingInput(msg tea.KeyMsg) (TopRatedModel, tea.Cm m.saveMessageTime = 3 return m, tickEverySecond() + case " ", "p": + // Toggle pause/resume (Space matches the other player screens) + if m.player != nil { + if err := m.player.TogglePause(); err != nil { + m.saveMessage = fmt.Sprintf("Pause failed: %v", err) + m.saveMessageSuccess = false + m.saveMessageTime = 3 + return m, tickEverySecond() + } + } + return m, nil + case "s": // Stop playback if m.player != nil { @@ -904,7 +916,7 @@ func (m TopRatedModel) View() string { case topRatedStateRating: helpText = "1-5: Set rating • 0/r: Remove rating • Esc: Cancel" case topRatedStatePlaying: - helpText = "s: Stop • *1-5: Rate • 0: Main Menu • Esc: Back" + helpText = "Space/p: Pause/Resume • s: Stop • *1-5: Rate • 0: Main Menu • Esc: Back" } return m.renderPageWithBottomHelp(PageLayout{