diff --git a/.claude/settings.local.json b/.claude/settings.local.json index 60a80a0a..3521e41a 100644 --- a/.claude/settings.local.json +++ b/.claude/settings.local.json @@ -4,17 +4,21 @@ }, "permissions": { "allow": [ - "Bash(mix compile:*)", - "Bash(mix test *)", - "Bash(fly status *)", - "Bash(fly volumes *)", + "Bash(MIX_ENV=test mix test *)", + "Bash(fly machine *)", "Bash(fly machines *)", "Bash(fly secrets *)", "Bash(fly ssh *)", - "Bash(fly machine *)", - "WebFetch(domain:developers.soundcloud.com)", + "Bash(fly status *)", + "Bash(fly volumes *)", + "Bash(gh run *)", "Bash(git stash *)", - "Bash(gh run *)" + "Bash(mix compile:*)", + "Bash(mix run -e ' *)", + "Bash(mix sass *)", + "Bash(mix test *)", + "WebFetch(domain:developers.soundcloud.com)", + "WebFetch(domain:raw.githubusercontent.com)" ] } } diff --git a/assets/css/layout.scss b/assets/css/layout.scss index 9964244c..f596df73 100644 --- a/assets/css/layout.scss +++ b/assets/css/layout.scss @@ -185,6 +185,18 @@ body { gap: 10px 0; } +.provider-indicator { + margin-right: auto; + display: flex; + align-items: center; + + .provider-icon { + width: 18px; + height: 18px; + margin-right: 0; + } +} + .profile-image { width: 45px; height: auto; diff --git a/docs/soundcloud-plan.md b/docs/soundcloud-plan.md index cf852d5b..f1ce51b8 100644 --- a/docs/soundcloud-plan.md +++ b/docs/soundcloud-plan.md @@ -1,6 +1,40 @@ # Plan: Mixed-provider queue (Spotify + SoundCloud) -Status: proposal. Not started. +Status: in progress (updated 2026-06-25). + +## Progress + +Legend: [deployed] in prod, [merged] on master, [wip] open PR / branch, [todo] not started. + +- [deployed] Data model (`provider` + `external_id`) + provider abstraction (#112). +- [deployed] SoundCloud OAuth 2.1 with PKCE (#113). Token exchange verified end to end. +- [deployed] Read path: `Provider.SoundCloud` search + get_track, provider registry, + token refresh, `SearchTrack` mapping. +- [deployed] Provider toggle in the search UI, behind `FF_PROVIDER_SWITCHER` and + shown to trusted users. +- [wip] Playlist editing + Sonos favourite trigger + metadata matching (PR #115). + Verified on prod during the spike (see gates below). +- [wip] Run-slicing: `Queue.current_run/0`, run-aware `sync_playlist`, + provider-aware `find_playlist` (`soundcloud-runs` branch). Pending staging verify. + +### Section 0 gates - RESOLVED + +- Paid API access + playlist editing: yes. `PUT /playlists/{id}` works (verified, + track added to a real playlist). +- Sonos metadata id shape: confirmed `track->soundcloud:tracks:{id}` (Sonos wraps + the SoundCloud urn in its `universalMusicObjectId` format). `parse_object_id` + handles it; the feedback loop resolves SoundCloud tracks. + +### SoundCloud API quirks discovered (already handled in code) + +- authorization_code token exchange: `client_secret` must be in the request + BODY; SoundCloud ignores the HTTP Basic header the oauth2 lib sends. +- Token refresh: same - `client_id` + `client_secret` go in the body. +- PUT requests need `Content-Type: application/json` or SoundCloud returns 422. +- Playlist track items must be `{"urn": "soundcloud:tracks:"}`, not `{"id": ...}`. +- API auth header: both `OAuth ` and `Bearer ` work (we send Bearer). +- `replace_playlist` currently targets a hardcoded `SOUNDCLOUD_PLAYLIST_ID` env - + temporary until the setup work below. Goal: let users queue both Spotify and SoundCloud tracks into the same live session, played out of the same Sonos speakers. The DB queue stays the single @@ -17,6 +51,10 @@ Sonos webhooks. ## 0. Open question / TODO (gating, not yet decided) +> RESOLVED (2026-06-25): both gates passed - see the Progress section above. API +> playlist editing works and the Sonos metadata id is `track->soundcloud:tracks:{id}`. +> Original notes kept below for context. + **SoundCloud API access requires a paid account.** Whether this feature is worth building at all depends on: @@ -276,21 +314,51 @@ and Sonos (`how-it-works.md` section 6). Reuse `PR.Apis.TokenHelper` and the ## 11. Suggested order of work -1. (Gate) Research SoundCloud paid API access + playlist edit support. Go/no-go. -2. (Spike) Save a SoundCloud playlist as a Sonos favourite, play it, capture the - metadata webhook. Confirm a matchable id. Go/no-go. -3. Data model: `provider` + `external_id` migration, constraint, novelty views, - structs. -4. Provider behaviour + extract `Provider.Spotify` from current code. No behaviour - change yet (Spotify-only, still works end to end). -5. Run slicing: `Queue.current_run/0`, run-aware `sync_playlist`, provider-aware - `find_playlist`. Still Spotify-only - verify the boundary loop works with an - all-Spotify queue split into artificial runs. -6. `Provider.SoundCloud`: auth, search, get track, replace playlist, object-id. -7. Metadata: generalise `SonosItem` / `cast_metadata`. -8. UI: provider toggle in search; setup screen for SoundCloud auth + playlist. -9. Integration testing focused on boundary transitions and skips across providers. +1. [done] (Gate) SoundCloud paid API access + playlist edit support. Verified. +2. [done] (Spike) Save a SoundCloud playlist as a Sonos favourite, play it, + capture the metadata webhook. Confirmed `track->soundcloud:tracks:{id}`. +3. [done] Data model: `provider` + `external_id` migration, constraint, novelty + views, structs. +4. [done] Provider behaviour + extract `Provider.Spotify`. +5. [wip] Run slicing: `Queue.current_run/0`, run-aware `sync_playlist`, + provider-aware `find_playlist` (`soundcloud-runs` branch). Tested with unit + tests; pending staging verification of the boundary loop. +6. [done] `Provider.SoundCloud`: auth, search, get track, replace playlist, + object-id. (replace_playlist still uses the hardcoded playlist env - see 10.) +7. [done] Metadata: `SonosItem` / `cast_metadata` are provider-generic via + `Provider.match_object_id`. +8. [partial] UI: provider toggle in search is done. Setup screen for SoundCloud + playlist selection is step 10. +9. [todo] Integration testing focused on boundary transitions and skips across + providers. +10. [todo] Setup parity (see section 12). Steps 4 and 5 are valuable on their own: they make the system provider-agnostic and exercise the run-boundary loop using only Spotify, so the riskiest mechanics are proven before SoundCloud is added. + +--- + +## 12. Setup parity: SoundCloud playlist selection + +Goal: make SoundCloud setup mirror Spotify and keep both as simple as possible. +Today SoundCloud's target playlist is a hardcoded `SOUNDCLOUD_PLAYLIST_ID` env var +(spike shortcut); Spotify creates its playlist from the setup screen +(`SpotifyAPI.create_playlist`, stored in `spotify_playlists`). + +Bring SoundCloud to the same shape, and improve on it: + +- Replace the env var with a stored SoundCloud playlist id (mirror + `SpotifyData.Playlist` / `spotify_playlists` with a `soundcloud_playlists` + table, or a generic playlists store). +- On the setup screen, instead of (or in addition to) "create playlist", **fetch + the user's existing playlists from the API** (`GET /me/playlists`) and let the + operator **pick which one** PlayRequest should write to. Selecting it stores the + id. This avoids creating yet another playlist and lets them point at an existing + one (e.g. the `sonosnow` playlist already favourited in Sonos). +- Consider applying the same picker to Spotify so both setups are identical. +- `Provider.SoundCloud.replace_playlist` then reads the stored id instead of the + env var; drop `SOUNDCLOUD_PLAYLIST_ID`. + +Keep the setup flow minimal: authorise -> pick (or create) playlist -> favourite +it in Sonos. Same three steps for both providers. diff --git a/lib/pr/music/music.ex b/lib/pr/music/music.ex index c6d78a70..9f0ab3bf 100644 --- a/lib/pr/music/music.ex +++ b/lib/pr/music/music.ex @@ -52,14 +52,22 @@ defmodule PR.Music do end end + @doc """ + Load the current run (the head same-provider block of the unplayed queue) into + that provider's playlist. Returns `{:ok, provider}` so the trigger knows which + favourite to play, or `{:ok, nil}` when the queue is empty. + """ def sync_playlist do - Logger.info("Syncing tracks to provider playlist") - - Queue.list_track_uris() - |> Enum.map(fn {id} -> id end) - |> default_provider().replace_playlist() - - {:ok} + case Queue.current_run() do + {nil, _} -> + Logger.info("Sync playlist: queue empty, nothing to sync") + {:ok, nil} + + {provider, ids} -> + Logger.info("Syncing #{length(ids)} #{provider} track(s) to provider playlist") + Provider.for(provider).replace_playlist(ids) + {:ok, provider} + end end def clear_playlist do @@ -71,14 +79,18 @@ defmodule PR.Music do # This take a little while to run, so there can be race conditions if it gets called # a few times, before it's had a chance to affect the play state def trigger_playlist(force \\ :dont_force) do - with {:ok} <- sync_playlist(), + with {:ok, provider} <- sync_playlist(), {:ok} <- check_unplayed(), %Group{group_id: group_id} <- SonosHouseholds.get_active_group!(), {:ok, %{items: sonos_favorites}, _} <- SonosAPI.get_favorites(), - {:ok, fav_id} <- find_playlist(sonos_favorites), + {:ok, fav_id} <- find_playlist(sonos_favorites, Provider.for(provider)), {:ok} <- check_current_playstate(PlayState.get(:play_state), force), %{} <- SonosAPI.set_favorite(fav_id, group_id) do - Logger.info("Trigger playlist: OK") + Logger.info( + "Trigger playlist OK: provider=#{provider}, " <> + "favourite=#{Provider.for(provider).favourite_name()} (#{fav_id})" + ) + {:ok} else {:error, :nothing_in_local_queue} -> @@ -210,9 +222,6 @@ defmodule PR.Music do Phoenix.PubSub.broadcast(PR.PubSub, @topic, {__MODULE__, data, key}) end - @spec find_playlist([map()]) :: {:ok, String.t()} | {:error, atom()} - defp find_playlist(sonos_favorites), do: find_playlist(sonos_favorites, default_provider()) - @spec find_playlist([map()], module()) :: {:ok, String.t()} | {:error, atom()} defp find_playlist(sonos_favorites, provider_mod) do case Enum.find(sonos_favorites, &(&1.name == provider_mod.favourite_name())) do diff --git a/lib/pr/play_state.ex b/lib/pr/play_state.ex index 4c7b3bb4..98aaf159 100644 --- a/lib/pr/play_state.ex +++ b/lib/pr/play_state.ex @@ -13,7 +13,10 @@ defmodule PR.PlayState do @topic inspect(__MODULE__) def start_link(_) do - Agent.start_link(fn -> %{play_state: %{}, metadata: %{}, progress: nil, error_mode: nil} end, + Agent.start_link( + fn -> + %{play_state: %{}, metadata: %{}, progress: nil, error_mode: nil, current_provider: nil} + end, name: __MODULE__ ) end @@ -159,8 +162,27 @@ defmodule PR.PlayState do # TODO might be worth checking metadata here case Queue.has_unplayed() do num when num > 0 -> - Logger.info("Player IDLE. But, queue has more tracks. Loading them in 1000ms") + {finishing_provider, _} = Queue.current_run() + + Logger.info( + "Player IDLE with #{num} unplayed. Finishing #{finishing_provider} run, " <> + "bumping last track and re-triggering in 1000ms" + ) + Process.sleep(1000) + # The run that just finished left its last track marked playing, not + # played. Mark it played before re-triggering, otherwise current_run + # returns the same run again and the provider never switches. (Same as + # what skip does.) + Queue.bump() + + {next_provider, next_ids} = Queue.current_run() + + Logger.info( + "Re-triggering: next run is #{next_provider} (#{length(next_ids)} tracks)" <> + if(next_provider != finishing_provider, do: " - PROVIDER SWITCH", else: "") + ) + trigger_on_sonos_system() state @@ -241,6 +263,7 @@ defmodule PR.PlayState do defp update_playing(%{current_item: %{name: name} = current} = state) do Logger.metadata(playback_state: Map.get(get(:play_state), :state)) + track_provider_change(current) if get(:error_mode) do Logger.warn("Update playing: Cancelled, cos error mode") @@ -273,6 +296,28 @@ defmodule PR.PlayState do end end + # Detect when the actually-playing provider changes, log it loudly and push + # it to the header indicator. Only fires on a real change, so it's quiet + # while a single provider's run plays out. + defp track_provider_change(%{provider: provider, name: name, artist: artist}) + when is_binary(provider) do + case get(:current_provider) do + ^provider -> + :ok + + previous -> + Logger.info( + "🔀 Now playing from #{provider}: #{name} - #{artist}" <> + if(previous, do: " (switched from #{previous})", else: "") + ) + + update_state(provider, :current_provider) + broadcast(provider, :provider) + end + end + + defp track_provider_change(_), do: :ok + # Called on an interval by supervisor def tick do with %{ diff --git a/lib/pr/queue/queue.ex b/lib/pr/queue/queue.ex index 2c83108f..c6b3d28e 100644 --- a/lib/pr/queue/queue.ex +++ b/lib/pr/queue/queue.ex @@ -78,6 +78,36 @@ defmodule PR.Queue do |> Repo.all() end + @doc """ + The current "run": the contiguous block of same-provider tracks at the head of + the unplayed queue (in play order). Returns `{provider, [external_id]}`, or + `{nil, []}` when the queue is empty. Only this block is loaded into a provider + playlist at a time, so Sonos goes idle at each provider boundary. + """ + @spec current_run() :: {String.t() | nil, [String.t()]} + def current_run do + rows = + Track + |> query_unplayed() + |> order() + |> limit(100) + |> select([t], {t.provider, t.external_id}) + |> Repo.all() + + case rows do + [] -> + {nil, []} + + [{provider, _} | _] -> + ids = + rows + |> Enum.take_while(fn {p, _} -> p == provider end) + |> Enum.map(fn {_, id} -> id end) + + {provider, ids} + end + end + @spec list_track_uris(String.t()) :: [String.t()] def list_track_uris(provider) do Track diff --git a/lib/pr_web/controllers/service/service_setup_controller.ex b/lib/pr_web/controllers/service/service_setup_controller.ex index 28a3e5a9..c3031a1e 100644 --- a/lib/pr_web/controllers/service/service_setup_controller.ex +++ b/lib/pr_web/controllers/service/service_setup_controller.ex @@ -174,7 +174,7 @@ defmodule PRWeb.Service.ServiceSetupController do def sync_playlist(conn, _) do case Music.sync_playlist() do - {:ok} -> + {:ok, _} -> conn |> put_flash(:info, "Playlist synced") |> redirect(to: ~p"/setup") diff --git a/lib/pr_web/live/user_header_live/user_header_live.ex b/lib/pr_web/live/user_header_live/user_header_live.ex index 02858de3..9a37c23f 100644 --- a/lib/pr_web/live/user_header_live/user_header_live.ex +++ b/lib/pr_web/live/user_header_live/user_header_live.ex @@ -38,6 +38,13 @@ defmodule PRWeb.UserHeaderLive do show_super_like={@show_super_like} />
+
+ +
<%= if (@show_toggle_playback or @current_user.is_trusted == true) and @num_unplayed > 0 do %> <.play_pause play_state={@play_state} show_skip={@show_skip && @num_unplayed > 1} /> <% end %> @@ -136,6 +143,7 @@ defmodule PRWeb.UserHeaderLive do points: likes, super_likes: super_likes, play_state: play_state, + current_provider: current_provider(), num_unplayed: Queue.num_unplayed(), participated: Queue.has_participated?(%User{id: user_id}), max_vol: max_vol() @@ -160,6 +168,11 @@ defmodule PRWeb.UserHeaderLive do {:ok, socket} end + defp current_provider, do: PlayState.get(:current_provider) + + defp provider_img("soundcloud"), do: ~p"/images/soundcloud.svg" + defp provider_img(_), do: ~p"/images/spotify.svg" + def max_vol() do case Application.get_env(:pr, :max_vol) do nil -> default_max_vol() @@ -203,6 +216,10 @@ defmodule PRWeb.UserHeaderLive do {:noreply, assign(socket, play_state: play_state)} end + def handle_info({PlayState, provider, :provider}, socket) when is_binary(provider) do + {:noreply, assign(socket, current_provider: provider)} + end + def handle_info( {Music, num_unplayed, :queue_updated}, %{assigns: %{current_user: %User{id: user_id}}} = socket diff --git a/plans/mark-played-investigation.md b/plans/mark-played-investigation.md new file mode 100644 index 00000000..99a6a2f5 --- /dev/null +++ b/plans/mark-played-investigation.md @@ -0,0 +1,46 @@ +# Investigate: tracks marked played when they were not played + +Status: parked, to analyse. Long-standing issue; several fixes already attempted +(see the commented-out `query_has_been_playing` calls in `lib/pr/queue/queue.ex`). + +## Symptom + +Queue rows get `played_at` set when the track was never actually played (or was +only partway through), so they drop out of the unplayed queue incorrectly. + +## Where to look + +- `Queue.set_current_transaction/3` (`lib/pr/queue/queue.ex:263`). When a track + starts, every *currently-playing* row (`playing_since` set) with a different + `(provider, external_id)` is marked played (`queue.ex:269`). Bounded to rows + flagged playing, which is reasonable - but if a row was wrongly flagged + playing, or boundary timing is off, the wrong row gets marked played. + +- `Queue.set_current/1` no-match clause (`queue.ex:213-259`), used by `bump/0` + and when Sonos reports empty/foreign metadata. This has a suspicious two-pass + structure: + 1. First `update_all` over `query_is_playing` sets `played_at` from + `datetime_add(playing_since, duration)` AND sets `playing_since: nil`. + 2. A second `update_all` over `query_is_playing` then tries to set + `played_at: nil` - but pass 1 already cleared `playing_since`, so this + query matches nothing. The first `case` result is also discarded. + Looks like dead/contradictory code. The intent (only mark played if it had + been playing long enough, else un-play) is not actually achieved. Prime + suspect for premature played marking. + +## Relevance to runs (step 5) + +Run boundaries re-trigger far more often (idle -> re-trigger per run), so +`set_current` fires more frequently - wider exposure to whatever is wrong here. +Also, now that SoundCloud object_ids resolve (post `track->` fix), a track that +Sonos plays but that is NOT in our queue still resolves to a SonosItem, and +`set_current_transaction` will mark the previously-playing queued row as played +even though our track did not actually finish. + +## Suggested approach + +- Untangle the `set_current/1` two-pass logic; decide the real rule for + "played vs un-played" based on `playing_since` + elapsed vs `duration`. +- Pin down the `playing_since` lifecycle (who sets/clears it, and when). +- Add tests around boundary transitions and foreign-track metadata before + changing behaviour. diff --git a/test/pr/queue_test.exs b/test/pr/queue_test.exs index 74eb2a5a..f8a99d0a 100644 --- a/test/pr/queue_test.exs +++ b/test/pr/queue_test.exs @@ -40,6 +40,42 @@ defmodule PR.QueueTest do end end + describe "current_run" do + test "empty queue returns {nil, []}" do + assert {nil, []} = Queue.current_run() + end + + test "all same provider returns the whole queue" do + t0 = DateTime.add(DateTime.utc_now(), -3, :second) + insert(:track, provider: "spotify", external_id: "s1", inserted_at: t0) + insert(:track, provider: "spotify", external_id: "s2", inserted_at: DateTime.add(t0, 1)) + assert {"spotify", ["s1", "s2"]} = Queue.current_run() + end + + test "returns only the leading same-provider block of a mixed queue" do + t0 = DateTime.add(DateTime.utc_now(), -5, :second) + insert(:track, provider: "spotify", external_id: "s1", inserted_at: t0) + insert(:track, provider: "spotify", external_id: "s2", inserted_at: DateTime.add(t0, 1)) + insert(:track, provider: "soundcloud", external_id: "c1", inserted_at: DateTime.add(t0, 2)) + insert(:track, provider: "spotify", external_id: "s3", inserted_at: DateTime.add(t0, 3)) + assert {"spotify", ["s1", "s2"]} = Queue.current_run() + end + + test "leading run can be the second provider" do + t0 = DateTime.add(DateTime.utc_now(), -3, :second) + insert(:track, provider: "soundcloud", external_id: "c1", inserted_at: t0) + insert(:track, provider: "spotify", external_id: "s1", inserted_at: DateTime.add(t0, 1)) + assert {"soundcloud", ["c1"]} = Queue.current_run() + end + + test "ignores played tracks when finding the run head" do + t0 = DateTime.add(DateTime.utc_now(), -3, :second) + insert(:played_track, provider: "spotify", external_id: "old", inserted_at: t0) + insert(:track, provider: "soundcloud", external_id: "c1", inserted_at: DateTime.add(t0, 1)) + assert {"soundcloud", ["c1"]} = Queue.current_run() + end + end + describe "points" do test "user sees that they did a point" do me = insert(:user)