Skip to content

Add comprehensive test coverage and improve error handling - #10

Open
emakko wants to merge 2 commits into
mainfrom
chore/code-review-fixes-and-tests
Open

emakko wants to merge 2 commits into
mainfrom
chore/code-review-fixes-and-tests

Conversation

@emakko

@emakko emakko commented Sep 25, 2026

Copy link
Copy Markdown
Owner

This PR adds extensive test coverage for core functionality and improves error handling and resilience across the application.

Summary

Added 400+ lines of new test files covering player initialization, gesture handling, playback retry logic, and session/dedupe operations. Enhanced error handling in authentication, API calls, and history storage to be more robust against edge cases and concurrent operations.

Key Changes

New Test Files

  • src/spotify/player.test.ts: Comprehensive tests for Web Player SDK loading, device management, playback state tracking, and error handling
  • src/components/gestures.test.ts: Tests for swipe gesture detection and keyboard shortcut mapping
  • src/hooks/usePlayer.test.ts: Tests for playback retry logic with 404 handling and cancellation

Enhanced Session Management

  • Added tracking of unconfirmed deletions and restoring songs to prevent duplicate additions when operations fail and retry
  • Improved restoreEntry to trim History entries after each insert, preventing duplicate copies on retry
  • Fixed restore logic to respect songs still queued for deletion and handle out-of-order positions correctly

Improved Authentication & API

  • Added forceRefresh(rejectedToken) parameter to track which token was rejected, allowing detection when another tab already rotated it
  • Added generation counter to prevent stale refresh operations from overwriting newer tokens
  • Improved error handling to distinguish between auth failures and server errors
  • Added validation of stored tokens and state during callback handling
  • Added MAX_RETRY_AFTER_S limit to prevent indefinite waits on rate limiting

Better Error Resilience

  • SDK load failures now reset state so subsequent players can retry loading
  • History storage now validates and drops malformed entries instead of failing
  • Playback retry skips the second attempt if user has moved to another card
  • Dedupe operations properly handle partial failures and retries without duplicating rows

UI/UX Improvements

  • Extracted gesture logic (swipeIntent, shortcutFor) to dedicated gestures.ts module for reusability
  • History panel now handles focus management and Escape key to close
  • Keyboard shortcuts respect when History panel is open (blocks ←/→ but allows Undo and Space)
  • Added MotionConfig wrapper in main.tsx for animation configuration

Code Quality

  • Added type validation for stored History entries
  • Improved test infrastructure with fake Spotify Player and playlist simulation helpers
  • Enhanced CI workflow with proper permissions declaration

https://claude.ai/code/session_01RofS2KTGcgYYorEqtuB5Uo

CI
- Run checkout before setup-node and pass node-version/cache to setup-node
  (a merge had swapped them, so CI never pinned Node 22); read-only token.

Spotify auth/API/player
- A refresh rejected because another tab already rotated the refresh token
  now uses that tab's tokens instead of logging every tab out.
- Logout during an in-flight refresh stays logged out.
- forceRefresh(rejectedToken) skips the refresh when a concurrent request
  already replaced the token.
- The OAuth callback requires a stored state and always clears the
  verifier/state; logout clears them too.
- Cap Retry-After at 30 s and fail with a readable message beyond that.
- A failed SDK load is retried by the next player; disconnect() before the
  SDK loads no longer leaves a stray connected player.

Swipe session / dedupe / history
- Undo followed immediately by removing the same song no longer loses its
  History entry (the restore leaves a newer entry alone).
- Restores ignore removals that are still queued, so they land at the
  right index.
- A restore that fails partway trims the entry, so retrying or Undo does
  not add a copy twice (session and dedupe).
- A second Restore click while one is queued is ignored.
- Undo on Liked Songs re-likes in reverse so the original order is kept.
- Malformed stored History entries are dropped instead of crashing.

UI
- Swipe gestures and keyboard shortcuts are pure, tested functions:
  flicking a keep-drag back no longer removes the song; Alt/Cmd+Arrow
  (browser Back) and other chords no longer swipe; Space on a focused
  button presses the button.
- A stale 404 play-retry no longer starts the previous card's song.
- Login errors are shown instead of being swallowed; reduced motion is
  respected; History panel takes focus, closes on Escape and labels each
  Restore button; dedupe empty-state Back is disabled while busy.

Tests: 129 -> 184, including a new player.ts suite, gestures and
playWithRetry tests; each regression test fails on the previous code.

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01RofS2KTGcgYYorEqtuB5Uo
- restoreEntry re-checks before every History write that the entry is
  still the one it was asked to restore, so removing the song again while
  its restore POST is in flight no longer loses the new entry (the earlier
  fix only covered a remove queued before the restore started).
- History Restore is ignored while the song's own DELETE is unconfirmed:
  if that DELETE failed without being applied, the restore duplicated it.
- Dedupe Undo trims the song's History entry after each re-added row, so
  Restore after a partly failed Undo no longer adds extra copies.
- History panel focuses itself rather than its Close button, so Space
  still toggles playback while it is open; focus returns on close.

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01RofS2KTGcgYYorEqtuB5Uo
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants