Skip to content

fix(spotify): propagate token update failures - #74

Merged
mmattbtw merged 2 commits into
mainfrom
fix/spotify-token-update-errors
Aug 27, 2026
Merged

fix(spotify): propagate token update failures#74
mmattbtw merged 2 commits into
mainfrom
fix/spotify-token-update-errors

Conversation

@mmattbtw

@mmattbtw mmattbtw commented Aug 27, 2026

Copy link
Copy Markdown
Member

Summary

  • return database token-update errors for existing Spotify users
  • keep the existing in-memory token unchanged when persistence fails
  • add a regression test using a real SQLite failure trigger

Test plan

  • go test ./service/spotify -run TestSetAccessToken_ExistingUserUpdateFailureReturnsError -count=1
  • go test -race ./...
  • git diff --check

Stacked on #72. The base branch mirrors the current head of that PR.

Summary by CodeRabbit

  • Bug Fixes

    • Authentication now stops and reports an error when an existing user’s access token cannot be updated.
    • Prevents continued authentication with an outdated token after a persistence failure.
  • Tests

    • Added coverage to verify error reporting and preservation of existing token data when updates fail.

@coderabbitai

coderabbitai Bot commented Aug 27, 2026

Copy link
Copy Markdown

Review Change Stack

Caution

Review failed

The pull request is closed.

ℹ️ Recent review info
⚙️ Run configuration

Configuration used: Organization UI

Review profile: CHILL

Plan: Pro Plus

Run ID: 78567f0d-954f-4ead-8370-4bc2c9f4cf14

📥 Commits

Reviewing files that changed from the base of the PR and between f1cc45d and b20656a.

📒 Files selected for processing (2)
  • service/spotify/spotify.go
  • service/spotify/spotify_test.go

📝 Walkthrough

Walkthrough

Changes

Spotify token persistence

Layer / File(s) Summary
Propagate existing-user token update failures
service/spotify/spotify.go, service/spotify/spotify_test.go
identifyAndStoreUser now returns update errors immediately. The test verifies a zero user ID and unchanged cached and stored tokens when the update fails.

Estimated code review effort: 2 (Simple) | ~10 minutes

Suggested reviewers: charlesharries

✨ Finishing Touches
📝 Generate docstrings
  • Create stacked PR
  • Commit on current branch
🧪 Generate unit tests (beta)
  • Create PR with unit tests
  • Commit unit tests in branch fix/spotify-token-update-errors

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.

❤️ Share

Comment @coderabbitai help to get the list of available commands.

@greptile-apps

greptile-apps Bot commented Aug 27, 2026

Copy link
Copy Markdown

Greptile Summary

The PR makes Spotify token persistence failures abort authentication before the in-memory token is changed.

  • Returns existing-user token update errors to the OAuth callback layer.
  • Updates the cached token only after successful database persistence.
  • Adds a SQLite-trigger regression test verifying the error, zero user ID, and unchanged persisted and cached tokens.

Confidence Score: 5/5

The PR appears safe to merge, with token-update failures consistently propagated through the existing OAuth error path.

The database error now exits before either the user object or shared token cache is updated, and the sole runtime caller already handles the error before using the returned user ID.

Important Files Changed

Filename Overview
service/spotify/spotify.go Propagates existing-user token update failures before mutating the cached token; no actionable defect identified.
service/spotify/spotify_test.go Adds focused regression coverage using a database trigger to verify persistence and cache consistency after failure.

Reviews (1): Last reviewed commit: "fix(spotify): propagate token update fai..." | Re-trigger Greptile

@mmattbtw
mmattbtw changed the base branch from push-lmmyonqyytyn to main August 27, 2026 14:40
@mmattbtw
mmattbtw enabled auto-merge August 27, 2026 14:40
@mmattbtw
mmattbtw disabled auto-merge August 27, 2026 14:40
@mmattbtw
mmattbtw merged commit af638a2 into main Aug 27, 2026
1 of 2 checks passed
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.

1 participant