Conversation
Contributor
There was a problem hiding this comment.
Pull request overview
This PR implements Last.fm scrobbling integration for Museeks, allowing users to automatically share their listening history with Last.fm. The implementation follows Last.fm's scrobbling specifications (tracks must be played for at least 50% of their duration or 4 minutes, and be longer than 30 seconds).
Key changes:
- Backend Rust plugin for Last.fm API authentication and scrobbling
- Frontend scrobbler service that monitors playback and triggers scrobbles
- New settings page for connecting/disconnecting Last.fm accounts
- Configuration storage for Last.fm credentials and enabled state
Reviewed changes
Copilot reviewed 19 out of 22 changed files in this pull request and generated 6 comments.
Show a summary per file
| File | Description |
|---|---|
| src-tauri/src/plugins/lastfm.rs | New plugin handling Last.fm API communication, authentication flow, and scrobbling operations |
| src-tauri/src/plugins/config.rs | Added Last.fm config fields (enabled, session_key, username) |
| src-tauri/src/plugins/mod.rs | Registered lastfm plugin module |
| src-tauri/src/main.rs | Added lastfm plugin initialization |
| src-tauri/src/libs/error.rs | Added LastFm error variant |
| src-tauri/build.rs | Registered lastfm commands for build system (contains duplicate plugin registration) |
| src-tauri/capabilities/main.json | Added Last.fm command permissions (missing test_connection permission) |
| src-tauri/Cargo.toml | Added reqwest and md5 dependencies for HTTP and API signature generation |
| src-tauri/Cargo.lock | Dependency lock file updates |
| src/lib/scrobbler.ts | New scrobbler service implementing Last.fm scrobbling rules and timing logic |
| src/lib/bridge-lastfm.ts | Bridge layer for frontend-backend Last.fm communication |
| src/routes/settings.scrobbling.tsx | New settings page UI for Last.fm connection management |
| src/routes/settings.tsx | Added navigation link to scrobbling settings |
| src/components/PlayerEvents.tsx | Initialize scrobbler to track playback events |
| src/generated/typings.ts | TypeScript type definitions for Last.fm data structures and updated Config type |
| src/generated/route-tree.ts | Auto-generated routing configuration including scrobbling route |
| src/translations/*.po | Translation entries for Last.fm UI strings (empty translations for non-English locales) |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Comment on lines
+15
to
+19
|
|
||
| // Last.fm API credentials - these are public identifiers for the Museeks app | ||
| // Register your app at https://www.last.fm/api/account/create | ||
| const API_KEY: &str = "6496d20a201157d8c0c86cde0f2df5db"; // TODO: Replace with actual API key | ||
| const API_SECRET: &str = "d4f4b99472e12f395744134fba2c6d27"; // TODO: Replace with actual API secret |
There was a problem hiding this comment.
These API credentials appear to be placeholders (as indicated by the TODO comments). Before merging to production:
- Replace these with actual Last.fm API credentials for the Museeks application
- Consider using environment variables or a config file rather than hardcoding them
- Note that Last.fm API keys are typically public knowledge (the secret is used for signing), but it's best practice to obtain your own credentials at https://www.last.fm/api/account/create
Suggested change
| // Last.fm API credentials - these are public identifiers for the Museeks app | |
| // Register your app at https://www.last.fm/api/account/create | |
| const API_KEY: &str = "6496d20a201157d8c0c86cde0f2df5db"; // TODO: Replace with actual API key | |
| const API_SECRET: &str = "d4f4b99472e12f395744134fba2c6d27"; // TODO: Replace with actual API secret | |
| use std::env; | |
| // Last.fm API credentials - loaded from environment variables for security | |
| // Register your app at https://www.last.fm/api/account/create | |
| // Set LASTFM_API_KEY and LASTFM_API_SECRET in your environment | |
| fn api_key() -> String { | |
| std::env::var("LASTFM_API_KEY").expect("LASTFM_API_KEY environment variable not set") | |
| } | |
| fn api_secret() -> String { | |
| std::env::var("LASTFM_API_SECRET").expect("LASTFM_API_SECRET environment variable not set") | |
| } |
Comment on lines
+84
to
+101
| private handleTimeUpdate(currentTime: number) { | ||
| if (!this.state.track || this.state.hasScrobbled || !this.state.startTime) { | ||
| return; | ||
| } | ||
|
|
||
| const track = this.state.track; | ||
| const playedDuration = currentTime; | ||
|
|
||
| // Calculate scrobble threshold | ||
| // Scrobble at 50% of track duration OR 4 minutes, whichever comes first | ||
| const halfDuration = track.duration / 2; | ||
| const scrobbleThreshold = Math.min(halfDuration, SCROBBLE_TIME_THRESHOLD); | ||
|
|
||
| // Check if we've reached the scrobble point | ||
| if (playedDuration >= scrobbleThreshold && !this.state.scrobbleTimeReached) { | ||
| this.state.scrobbleTimeReached = true; | ||
| this.scrobbleTrack(track); | ||
| } |
There was a problem hiding this comment.
The scrobbling logic uses currentTime (playback position) instead of actual played duration. This allows users to cheat the scrobble requirement by seeking forward. According to Last.fm scrobbling rules, a track should be scrobbled after being played for 50% of its duration or 4 minutes, not when the playback position reaches that point.
To fix this:
- Track the actual cumulative played time (not just position)
- Handle seek events to prevent seek-based scrobbling
- Or, accept this simpler implementation as a reasonable approximation (many scrobblers use position-based logic for simplicity)
Comment on lines
+32
to
+33
| // Initialize the scrobbler to track playback and send to Last.fm | ||
| scrobbler.init(); |
There was a problem hiding this comment.
The scrobbler initialization adds event listeners to the player but there's no cleanup function to remove them. This could lead to duplicate event listeners if the component re-renders or remounts.
Consider one of these solutions:
- Add a
cleanup()method to the Scrobbler class and call it in the useEffect cleanup - Ensure
scrobbler.init()is idempotent by checking if listeners are already attached before adding them - Move scrobbler initialization outside of React (e.g., in main.tsx) if it should only happen once per app lifecycle
Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>
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.
fixes #231
Thank you Claude, because you've literally done it all.