Keep bot usable when Spotify refresh token becomes invalid - #73
Open
vtvz wants to merge 3 commits into
Open
Conversation
Previously token_refresh returned an error on invalid_grant, which made user_state construction fail and every Telegram update die before any reply was sent, locking the user out of the bot entirely. Now only the in-memory token is cleared (the spotify_auth row is kept), so handlers treat the user as unauthenticated and prompt them to /login again.
📝 WalkthroughWalkthroughSpotify token refresh outcomes now use ChangesSpotify token state propagation
Estimated code review effort: 3 (Moderate) | ~20 minutes Sequence Diagram(s)sequenceDiagram
participant AppUserState
participant SpotifyManager
participant Database
participant Telegram
participant TickUser
AppUserState->>SpotifyManager: resolve Spotify client and TokenState
SpotifyManager-->>AppUserState: TokenState::Invalid
AppUserState->>Database: update user status
AppUserState->>Telegram: notify_token_invalid
TickUser->>TickUser: check Spotify authentication
TickUser-->>TickUser: return Complete when unauthenticated
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
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. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@src/spotify/mod.rs`:
- Around line 390-394: Change the tracing call in the Spotify refresh-token
handling path from warn-level to debug-level for the expected invalid-token
case, while preserving the existing user_id field and message.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro
Run ID: 28f9b909-c936-4a0a-8928-7a1afae722f0
📒 Files selected for processing (1)
src/spotify/mod.rs
The invalid-token alert previously depended on the background tick being the first to hit the dead token; if the user interacted with the bot first, the status flipped silently and the alert was never sent. Now user_state detects the transition itself, marks the user and sends the localized /login prompt directly, whichever path discovers it first. The spotify layer no longer touches user status and only reports TokenState.
Owner
Author
|
@CodeRabbit full review |
✅ Action performedFull review finished. Your plan includes PR reviews subject to rate limits. More reviews will be available in 36 minutes. |
Owner
Author
|
@CodeRabbit full review |
✅ Action performedFull review finished. |
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@src/app.rs`:
- Around line 377-391: Update the invalid-token branch in the flow around
UserService::set_status to also set the in-memory user.status to
UserStatus::SpotifyTokenInvalid before constructing UserState, while preserving
the existing database update and notification behavior.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro
Run ID: 0e769585-1aa5-4d80-82a7-69c10f00f2fc
📒 Files selected for processing (4)
src/app.rssrc/spotify/mod.rssrc/telegram/mod.rssrc/tick/user.rs
Comment on lines
377
to
+391
| let (user, newly_created) = UserService::upsert_by_id(self.db(), user_id).await?; | ||
| let (spotify, token_state) = self.spotify_manager.for_user(&self.db, user_id).await?; | ||
|
|
||
| if matches!(token_state, TokenState::Invalid) | ||
| && user.status != UserStatus::SpotifyTokenInvalid | ||
| { | ||
| UserService::set_status(self.db(), user_id, UserStatus::SpotifyTokenInvalid).await?; | ||
|
|
||
| // NOTE: Yes, it's a hack. I don't know how to handle invalid Spotify token in the right way | ||
| // with error_handler module | ||
| if let Err(err) = telegram::notify_token_invalid(self, &user).await { | ||
| tracing::error!(err = ?err, %user_id, "Failed to notify about invalid Spotify token"); | ||
| } | ||
| } | ||
|
|
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick win
Stale user status in UserState.
When the token is determined to be invalid, the user status is updated in the database via UserService::set_status, but the in-memory user variable is not updated. Consequently, the UserState created on line 392 will carry a stale status. Any downstream logic expecting the status to be SpotifyTokenInvalid during this execution flow will behave incorrectly.
Update the user model's status in memory before constructing UserState.
♻️ Proposed fix
- let (user, newly_created) = UserService::upsert_by_id(self.db(), user_id).await?;
+ let (mut user, newly_created) = UserService::upsert_by_id(self.db(), user_id).await?;
let (spotify, token_state) = self.spotify_manager.for_user(&self.db, user_id).await?;
if matches!(token_state, TokenState::Invalid)
&& user.status != UserStatus::SpotifyTokenInvalid
{
UserService::set_status(self.db(), user_id, UserStatus::SpotifyTokenInvalid).await?;
+ user.status = UserStatus::SpotifyTokenInvalid;
// NOTE: Yes, it's a hack. I don't know how to handle invalid Spotify token in the right way📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| let (user, newly_created) = UserService::upsert_by_id(self.db(), user_id).await?; | |
| let (spotify, token_state) = self.spotify_manager.for_user(&self.db, user_id).await?; | |
| if matches!(token_state, TokenState::Invalid) | |
| && user.status != UserStatus::SpotifyTokenInvalid | |
| { | |
| UserService::set_status(self.db(), user_id, UserStatus::SpotifyTokenInvalid).await?; | |
| // NOTE: Yes, it's a hack. I don't know how to handle invalid Spotify token in the right way | |
| // with error_handler module | |
| if let Err(err) = telegram::notify_token_invalid(self, &user).await { | |
| tracing::error!(err = ?err, %user_id, "Failed to notify about invalid Spotify token"); | |
| } | |
| } | |
| let (mut user, newly_created) = UserService::upsert_by_id(self.db(), user_id).await?; | |
| let (spotify, token_state) = self.spotify_manager.for_user(&self.db, user_id).await?; | |
| if matches!(token_state, TokenState::Invalid) | |
| && user.status != UserStatus::SpotifyTokenInvalid | |
| { | |
| UserService::set_status(self.db(), user_id, UserStatus::SpotifyTokenInvalid).await?; | |
| user.status = UserStatus::SpotifyTokenInvalid; | |
| // NOTE: Yes, it's a hack. I don't know how to handle invalid Spotify token in the right way | |
| // with error_handler module | |
| if let Err(err) = telegram::notify_token_invalid(self, &user).await { | |
| tracing::error!(err = ?err, %user_id, "Failed to notify about invalid Spotify token"); | |
| } | |
| } |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@src/app.rs` around lines 377 - 391, Update the invalid-token branch in the
flow around UserService::set_status to also set the in-memory user.status to
UserStatus::SpotifyTokenInvalid before constructing UserState, while preserving
the existing database update and notification behavior.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Previously an invalid_grant on token refresh made user_state construction fail, so every Telegram update died before any reply was sent and the user was permanently locked out of the bot. Now token_refresh clears only the in-memory token (keeping the spotify_auth row), so the state behaves as unauthenticated and handlers prompt the user to /login again.