From 0996b78c7b525e327e5812d5e07a28b6bccad523 Mon Sep 17 00:00:00 2001 From: Claudio Ortolina Date: Wed, 17 Sep 2025 10:26:38 +0300 Subject: [PATCH] Use Transforms to serve covers at the right size --- lib/music_library/assets/transform.ex | 6 +++ .../components/record_components.ex | 13 +++++- .../components/search_components.ex | 1 + .../components/stats_components.ex | 3 +- .../controllers/cover_controller.ex | 44 +++++++------------ lib/music_library_web/router.ex | 2 +- .../controllers/cover_controller_test.exs | 38 ++++++++++------ .../live/collection_live/index_test.exs | 31 +++++-------- .../live/collection_live/show_test.exs | 5 ++- .../live/wishlist_live/show_test.exs | 5 ++- 10 files changed, 80 insertions(+), 68 deletions(-) diff --git a/lib/music_library/assets/transform.ex b/lib/music_library/assets/transform.ex index 037c56fa..d1c72910 100644 --- a/lib/music_library/assets/transform.ex +++ b/lib/music_library/assets/transform.ex @@ -37,3 +37,9 @@ defmodule MusicLibrary.Assets.Transform do }) end end + +defimpl Phoenix.Param, for: MusicLibrary.Assets.Transform do + def to_param(transform) do + MusicLibrary.Assets.Transform.encode!(transform) + end +end diff --git a/lib/music_library_web/components/record_components.ex b/lib/music_library_web/components/record_components.ex index 52caa89f..ee84dd47 100644 --- a/lib/music_library_web/components/record_components.ex +++ b/lib/music_library_web/components/record_components.ex @@ -2,18 +2,26 @@ defmodule MusicLibraryWeb.RecordComponents do use MusicLibraryWeb, :html alias MusicBrainz.ReleaseSearchResult + alias MusicLibrary.Assets.Transform alias MusicLibrary.Records alias Phoenix.LiveView.JS attr :record, :map, required: true attr :class, :string, required: false, default: "rounded-lg" + attr :width, :integer, default: nil def record_cover(assigns) do + payload = + %Transform{hash: assigns.record.cover_hash, width: assigns.width} + |> Transform.encode!() + + assigns = assign(assigns, :payload, payload) + ~H""" {@record.title} """ end @@ -58,7 +66,7 @@ defmodule MusicLibraryWeb.RecordComponents do >
- <.record_cover record={record} /> + <.record_cover record={record} width={160} /> 0} class={[ @@ -230,6 +238,7 @@ defmodule MusicLibraryWeb.RecordComponents do <.record_cover record={record} class="pointer-events-none aspect-square object-cover group-hover:opacity-75" + width={460} /> 0} diff --git a/lib/music_library_web/components/search_components.ex b/lib/music_library_web/components/search_components.ex index 5a8b883e..dc2fb4f7 100644 --- a/lib/music_library_web/components/search_components.ex +++ b/lib/music_library_web/components/search_components.ex @@ -34,6 +34,7 @@ defmodule MusicLibraryWeb.SearchComponents do <.record_cover record={@record} class="w-12 h-12 rounded-md aspect-square object-cover" + width={96} />
diff --git a/lib/music_library_web/components/stats_components.ex b/lib/music_library_web/components/stats_components.ex index c7412743..72378e1d 100644 --- a/lib/music_library_web/components/stats_components.ex +++ b/lib/music_library_web/components/stats_components.ex @@ -23,6 +23,7 @@ defmodule MusicLibraryWeb.StatsComponents do <.record_cover record={@record} class="w-16 md:w-20 rounded-md shadow-sm" + width={160} />
@@ -167,7 +168,7 @@ defmodule MusicLibraryWeb.StatsComponents do >
- <.record_cover record={record} /> + <.record_cover record={record} width={96} /> 0} class={[ diff --git a/lib/music_library_web/controllers/cover_controller.ex b/lib/music_library_web/controllers/cover_controller.ex index 70e3d16d..9161382b 100644 --- a/lib/music_library_web/controllers/cover_controller.ex +++ b/lib/music_library_web/controllers/cover_controller.ex @@ -2,38 +2,32 @@ defmodule MusicLibraryWeb.CoverController do use MusicLibraryWeb, :controller alias MusicLibrary.Assets - alias MusicLibrary.Assets.Asset + alias MusicLibrary.Assets.Transform alias MusicLibrary.Records.Cover # 1 year in seconds @cache_duration 60 * 60 * 24 * 365 - def show(conn, %{"hash" => hash, "size" => size}) do - case Assets.get(hash) do + def show(conn, %{"transform_payload" => payload}) do + transform = Transform.decode!(payload) + + case Assets.get(transform.hash) do nil -> not_found(conn) %{content: content} -> - # TODO: find a way to cache computation, or pre-compute thumb and store it - {:ok, thumb_data} = Cover.resize(content, String.to_integer(size)) - hash = Asset.hash(thumb_data) + image_data = + if transform.width do + # TODO: find a way to cache computation, or pre-compute thumb and store it + {:ok, thumb_data} = Cover.resize(content, transform.width) + thumb_data + else + content + end case get_req_header(conn, "if-none-match") do - [^hash] -> extend_cache(conn) - _ -> respond_with_cache(conn, thumb_data, hash) - end - end - end - - def show(conn, %{"hash" => hash}) do - case Assets.get(hash) do - nil -> - not_found(conn) - - asset -> - case get_req_header(conn, "if-none-match") do - [^hash] -> extend_cache(conn) - _ -> respond_with_cache(conn, asset) + [^payload] -> extend_cache(conn) + _ -> respond_with_cache(conn, image_data, payload) end end end @@ -50,14 +44,6 @@ defmodule MusicLibraryWeb.CoverController do |> send_resp(304, "") end - defp respond_with_cache(conn, asset) do - conn - |> put_resp_content_type(asset.format, "utf-8") - |> put_resp_header("cache-control", "public, max-age=#{@cache_duration}") - |> put_resp_header("etag", asset.hash) - |> send_resp(200, asset.content) - end - defp respond_with_cache(conn, data, etag) do conn |> put_resp_content_type("image/jpeg", "utf-8") diff --git a/lib/music_library_web/router.ex b/lib/music_library_web/router.ex index 7c6f9c63..b3f3f048 100644 --- a/lib/music_library_web/router.ex +++ b/lib/music_library_web/router.ex @@ -36,7 +36,7 @@ defmodule MusicLibraryWeb.Router do get "/backup", ArchiveController, :backup - get "/covers/:hash", CoverController, :show + get "/covers/:transform_payload", CoverController, :show get "/artists/:musicbrainz_id/image", ArtistController, :image live_session :default, diff --git a/test/music_library_web/controllers/cover_controller_test.exs b/test/music_library_web/controllers/cover_controller_test.exs index 38b593c8..5dafdaed 100644 --- a/test/music_library_web/controllers/cover_controller_test.exs +++ b/test/music_library_web/controllers/cover_controller_test.exs @@ -4,6 +4,7 @@ defmodule MusicLibraryWeb.CoverControllerTest do import MusicLibrary.Fixtures.Records alias MusicLibrary.Assets + alias MusicLibrary.Assets.Transform defp create_asset(_config) do {:ok, asset} = Assets.store(%{content: marbles_cover_data(), format: "image/jpeg"}) @@ -15,42 +16,51 @@ defmodule MusicLibraryWeb.CoverControllerTest do setup [:create_asset] test "404s when asset doesn't exist", %{conn: conn} do - id = Ecto.UUID.generate() + transform = %Transform{hash: Ecto.UUID.generate()} + payload = Transform.encode!(transform) - conn = get(conn, ~p"/covers/#{id}") + conn = get(conn, ~p"/covers/#{payload}") assert text_response(conn, 404) == "Not found" end test "serves the cover without etag", %{conn: conn, asset: asset} do - conn = get(conn, ~p"/covers/#{asset.hash}") + transform = %Transform{hash: asset.hash} + payload = Transform.encode!(transform) + conn = get(conn, ~p"/covers/#{payload}") assert conn.status == 200 assert get_resp_header(conn, "content-type") == ["image/jpeg; charset=utf-8"] assert get_resp_header(conn, "cache-control") == ["public, max-age=31536000"] - assert get_resp_header(conn, "etag") == [asset.hash] + assert get_resp_header(conn, "etag") == [payload] assert conn.resp_body == asset.content end test "serves the cover when etag doesn't match", %{conn: conn, asset: asset} do + transform = %Transform{hash: asset.hash} + payload = Transform.encode!(transform) + conn = conn |> put_req_header("if-none-match", "invalid-etag") - |> get(~p"/covers/#{asset.hash}") + |> get(~p"/covers/#{payload}") assert conn.status == 200 assert get_resp_header(conn, "content-type") == ["image/jpeg; charset=utf-8"] assert get_resp_header(conn, "cache-control") == ["public, max-age=31536000"] - assert get_resp_header(conn, "etag") == [asset.hash] + assert get_resp_header(conn, "etag") == [payload] assert conn.resp_body == asset.content end test "serves a 304 when etag matches", %{conn: conn, asset: asset} do + transform = %Transform{hash: asset.hash} + payload = Transform.encode!(transform) + conn = conn - |> put_req_header("if-none-match", asset.hash) - |> get(~p"/covers/#{asset.hash}") + |> put_req_header("if-none-match", payload) + |> get(~p"/covers/#{payload}") assert conn.status == 304 assert get_resp_header(conn, "content-type") == [] @@ -60,18 +70,18 @@ defmodule MusicLibraryWeb.CoverControllerTest do assert conn.resp_body == <<>> end - test "accepts a size attribute for resizing", %{conn: conn, asset: asset} do - conn = get(conn, ~p"/covers/#{asset.hash}?size=480") + test "it handles transforms with width", %{conn: conn, asset: asset} do + transform = %Transform{hash: asset.hash, width: 480} + payload = Transform.encode!(transform) - thumb = marbles_thumb_data() - hash = Assets.Asset.hash(thumb) + conn = get(conn, ~p"/covers/#{payload}") assert conn.status == 200 assert get_resp_header(conn, "content-type") == ["image/jpeg; charset=utf-8"] assert get_resp_header(conn, "cache-control") == ["public, max-age=31536000"] - assert get_resp_header(conn, "etag") == [hash] + assert get_resp_header(conn, "etag") == [payload] - assert conn.resp_body == thumb + assert conn.resp_body == marbles_thumb_data() end end end diff --git a/test/music_library_web/live/collection_live/index_test.exs b/test/music_library_web/live/collection_live/index_test.exs index 00b94431..9c885d20 100644 --- a/test/music_library_web/live/collection_live/index_test.exs +++ b/test/music_library_web/live/collection_live/index_test.exs @@ -8,6 +8,7 @@ defmodule MusicLibraryWeb.CollectionLive.IndexTest do alias MusicBrainz.ReleaseGroupSearchResult alias MusicLibrary.Assets + alias MusicLibrary.Assets.Transform alias MusicLibrary.Records.{Cover, Record} # make it a multiple of 4 for easier calculations @@ -51,7 +52,7 @@ defmodule MusicLibraryWeb.CollectionLive.IndexTest do session = visit(conn, ~p"/collection?order=alphabetical&page_size=#{page_size}") for record <- expected_present do - cover_url = ~p"/covers/#{record.cover_hash}" + cover_url = cover_url(record, 160) session |> assert_has("#records-#{record.id}") @@ -140,7 +141,7 @@ defmodule MusicLibraryWeb.CollectionLive.IndexTest do session = visit(conn, ~p"/collection?#{qs}") - cover_url = ~p"/covers/#{record.cover_hash}" + cover_url = cover_url(record, 160) session |> assert_has("#records-#{record.id}") @@ -182,7 +183,7 @@ defmodule MusicLibraryWeb.CollectionLive.IndexTest do visit(conn, ~p"/collection?#{qs}") for record <- present do - cover_url = ~p"/covers/#{record.cover_hash}" + cover_url = cover_url(record, 160) session |> assert_has("#records-#{record.id}") @@ -219,7 +220,7 @@ defmodule MusicLibraryWeb.CollectionLive.IndexTest do test "can change the record cover", %{conn: conn} do record = record() - cover_url = ~p"/covers/#{record.cover_hash}" + cover_url = cover_url(record, nil) session = conn @@ -229,25 +230,11 @@ defmodule MusicLibraryWeb.CollectionLive.IndexTest do session = session |> upload("Cover art", raven_cover_fixture()) - # Shows a range warning - # - # warning: Range.new/2 and first..last default to a step of -1 when last < first. Use Range.new(first, last, -1) or first..last//-1, or pass 1 if that was your intention - # (phoenix_live_view 1.1.10) lib/phoenix_live_view/test/diff.ex:80: Phoenix.LiveViewTest.Diff.deep_merge_diff/2 - # (stdlib 7.0.2) maps.erl:405: :maps.merge_with_1/3 - # (phoenix_live_view 1.1.10) lib/phoenix_live_view/test/diff.ex:62: Phoenix.LiveViewTest.Diff.find_component/5 - # (phoenix_live_view 1.1.10) lib/phoenix_live_view/test/diff.ex:30: anonymous fn/4 in Phoenix.LiveViewTest.Diff.merge_diff/2 - # (stdlib 7.0.2) maps.erl:894: :maps.fold_1/4 - # (phoenix_live_view 1.1.10) lib/phoenix_live_view/test/diff.ex:29: Phoenix.LiveViewTest.Diff.merge_diff/2 - # (phoenix_live_view 1.1.10) lib/phoenix_live_view/test/client_proxy.ex:864: Phoenix.LiveViewTest.ClientProxy.merge_rendered/3 - # (phoenix_live_view 1.1.10) lib/phoenix_live_view/test/client_proxy.ex:488: Phoenix.LiveViewTest.ClientProxy.handle_info/2 - # (stdlib 7.0.2) gen_server.erl:2434: :gen_server.try_handle_info/3 - # (stdlib 7.0.2) gen_server.erl:2420: :gen_server.handle_msg/3 - # (stdlib 7.0.2) proc_lib.erl:333: :proc_lib.init_p_do_apply/3 |> click_button("Save") |> assert_has("p", text: "Record updated successfully") updated_record = MusicLibrary.Records.get_record!(record.id) - updated_cover_url = ~p"/covers/#{updated_record.cover_hash}" + updated_cover_url = cover_url(updated_record, 160) assert updated_record.cover_hash !== record.cover_hash assert_has(session, "img[src='#{updated_cover_url}']") @@ -455,4 +442,10 @@ defmodule MusicLibraryWeb.CollectionLive.IndexTest do musicbrainz_id: "1932f5b6-0b7b-4050-b1df-833ca89e5f44" } = marillion end + + defp cover_url(record, width) do + transform = %Transform{hash: record.cover_hash, width: width} + payload = Transform.encode!(transform) + ~p"/covers/#{payload}" + end end diff --git a/test/music_library_web/live/collection_live/show_test.exs b/test/music_library_web/live/collection_live/show_test.exs index 7ed01c62..6a342507 100644 --- a/test/music_library_web/live/collection_live/show_test.exs +++ b/test/music_library_web/live/collection_live/show_test.exs @@ -5,6 +5,7 @@ defmodule MusicLibraryWeb.CollectionLive.ShowTest do import MusicLibraryWeb.RecordComponents, only: [format_label: 1, type_label: 1] alias MusicBrainz.Fixtures + alias MusicLibrary.Assets.Transform alias MusicLibrary.Records.Record describe "Edit record from show page" do @@ -29,7 +30,9 @@ defmodule MusicLibraryWeb.CollectionLive.ShowTest do describe "Show record" do test "it includes all needed information", %{conn: conn} do record = record() - cover_url = ~p"/covers/#{record.cover_hash}" + transform = %Transform{hash: record.cover_hash, width: nil} + payload = Transform.encode!(transform) + cover_url = ~p"/covers/#{payload}" release_response = Fixtures.Release.release(:marbles) diff --git a/test/music_library_web/live/wishlist_live/show_test.exs b/test/music_library_web/live/wishlist_live/show_test.exs index 1ee4c3b2..8af1ec47 100644 --- a/test/music_library_web/live/wishlist_live/show_test.exs +++ b/test/music_library_web/live/wishlist_live/show_test.exs @@ -4,6 +4,7 @@ defmodule MusicLibraryWeb.WishlistLive.ShowTest do import MusicLibrary.Fixtures.Records import MusicLibraryWeb.RecordComponents, only: [format_label: 1, type_label: 1] + alias MusicLibrary.Assets.Transform alias MusicLibrary.Records.Record describe "Edit record from show page" do @@ -21,7 +22,9 @@ defmodule MusicLibraryWeb.WishlistLive.ShowTest do describe "Show record" do test "it includes all needed information", %{conn: conn} do record = record(purchased_at: nil) - cover_url = ~p"/covers/#{record.cover_hash}" + transform = %Transform{hash: record.cover_hash, width: nil} + payload = Transform.encode!(transform) + cover_url = ~p"/covers/#{payload}" session = conn