Cache patches instead of HTML

Author: ericmj

lib/diff/storage/gcs.ex

@@ -62,11 +62,11 @@ defmodule Diff.Storage.GCS do
     end
   end
 
-  def put_patch(package, from_version, to_version, patch_id, patch_html) do
+  def put_patch(package, from_version, to_version, patch_id, patch_data) do
     with {:ok, hash} <- combined_checksum(package, from_version, to_version),
          url = url(patch_key(package, from_version, to_version, hash, patch_id)),
          {:ok, 200, _headers, _body} <-
-           Diff.HTTP.retry("gs", fn -> Diff.HTTP.put(url, headers(), patch_html) end) do
+           Diff.HTTP.retry("gs", fn -> Diff.HTTP.put(url, headers(), patch_data) end) do
       :ok
     else
       {:ok, status, _headers, _body} ->
@@ -154,7 +154,7 @@ defmodule Diff.Storage.GCS do
   end
 
   defp patch_key(package, from_version, to_version, hash, patch_id) do
-    "diffs/patches/#{package}-#{from_version}-#{to_version}-#{hash}-#{patch_id}.html"
+    "diffs/patches/#{package}-#{from_version}-#{to_version}-#{hash}-#{patch_id}.patch"
   end
 
   defp metadata_key(package, from_version, to_version, hash) do

lib/diff/storage/local.ex

@@ -51,12 +51,12 @@ defmodule Diff.Storage.Local do
     end
   end
 
-  def put_patch(package, from_version, to_version, patch_id, patch_html) do
+  def put_patch(package, from_version, to_version, patch_id, patch_data) do
     with {:ok, hash} <- combined_checksum(package, from_version, to_version),
          filename = patch_key(package, from_version, to_version, hash, patch_id),
          path = Path.join([dir(), package, "patches", filename]),
          :ok <- File.mkdir_p(Path.dirname(path)) do
-      File.write!(path, patch_html)
+      File.write!(path, patch_data)
       :ok
     else
       {:error, reason} ->
@@ -79,7 +79,7 @@ defmodule Diff.Storage.Local do
             |> Enum.map(fn filename ->
               filename
               |> String.replace_prefix(prefix, "")
-              |> String.replace_suffix(".html", "")
+              |> String.replace_suffix(".patch", "")
             end)
             |> Enum.sort_by(fn patch_id ->
               # Extract numeric part for proper sorting (e.g., "patch-10" -> 10)
@@ -157,7 +157,7 @@ defmodule Diff.Storage.Local do
   end
 
   defp patch_key(package, from_version, to_version, hash, patch_id) do
-    "#{package}-#{from_version}-#{to_version}-#{hash}-#{patch_id}.html"
+    "#{package}-#{from_version}-#{to_version}-#{hash}-#{patch_id}.patch"
   end
 
   defp metadata_key(package, from_version, to_version, hash) do

lib/diff_web/live/diff_live_view.ex

@@ -281,13 +281,12 @@ defmodule DiffWeb.DiffLiveView do
         case element do
           {:ok, patch} ->
             if patch.chunks != [] do
-              patch_html =
-                Phoenix.View.render_to_string(DiffWeb.RenderView, "patch.html", patch: patch)
-                |> sanitize_utf8()
-
+              # Store raw patch data directly - convert struct to map and sanitize for JSON encoding
+              sanitized_patch = sanitize_patch_content(patch)
+              patch_data = Jason.encode!(struct_to_map(sanitized_patch))
               patch_id = "patch-#{index}"
 
-              Diff.Storage.put_patch(package, from, to, patch_id, patch_html)
+              Diff.Storage.put_patch(package, from, to, patch_id, patch_data)
 
               additions = count_lines(patch, "+")
               deletions = count_lines(patch, "-")
@@ -306,13 +305,11 @@ defmodule DiffWeb.DiffLiveView do
             end
 
           {:too_large, file_path} ->
-            too_large_html =
-              Phoenix.View.render_to_string(DiffWeb.RenderView, "too_large.html", file: file_path)
-              |> sanitize_utf8()
-
+            # Store raw too_large data directly
+            too_large_data = Jason.encode!(%{type: "too_large", file: file_path})
             patch_id = "patch-#{index}"
 
-            Diff.Storage.put_patch(package, from, to, patch_id, too_large_html)
+            Diff.Storage.put_patch(package, from, to, patch_id, too_large_data)
 
             updated_metadata = %{
               acc_metadata
@@ -390,41 +387,171 @@ defmodule DiffWeb.DiffLiveView do
   defp parse_version(input), do: Version.parse(input)
 
   defp sanitize_utf8(content) when is_binary(content) do
-    # Replace invalid UTF-8 bytes with replacement character
     case String.valid?(content) do
       true ->
         content
 
       false ->
-        # Convert to UTF-8, replacing invalid bytes
+        # Multiple fallback strategies for invalid UTF-8
         content
-        |> :unicode.characters_to_binary(:latin1, :utf8)
-        |> case do
-          result when is_binary(result) ->
-            result
-
-          _ ->
-            # If conversion fails, scrub invalid bytes
-            content
-            |> String.codepoints()
-            |> Enum.filter(&String.valid?/1)
-            |> Enum.join()
-        end
+        |> sanitize_invalid_bytes()
     end
   end
 
   defp sanitize_utf8(content), do: content
 
+  defp sanitize_invalid_bytes(content) do
+    # Try different encoding conversions and fallbacks
+    cond do
+      # Try converting from common encodings
+      latin1_result = safe_unicode_convert(content, :latin1, :utf8) ->
+        latin1_result
+
+      # Try converting from ISO-8859-1
+      iso_result = safe_unicode_convert(content, {:encoding, :latin1}, :utf8) ->
+        iso_result
+
+      # Last resort: replace invalid bytes with replacement character
+      true ->
+        content
+        |> :binary.bin_to_list()
+        # Replace high bytes with '?'
+        |> Enum.map(fn byte -> if byte > 127, do: 63, else: byte end)
+        |> :binary.list_to_bin()
+    end
+  end
+
+  defp safe_unicode_convert(content, from, to) do
+    case :unicode.characters_to_binary(content, from, to) do
+      result when is_binary(result) -> result
+      _ -> nil
+    end
+  rescue
+    _ -> nil
+  end
+
   defp load_patch_content(package, from, to, patch_id) do
     case Diff.Storage.get_patch(package, from, to, patch_id) do
-      {:ok, content} ->
-        sanitize_utf8(content)
+      {:ok, patch_json} ->
+        case Jason.decode(patch_json) do
+          {:ok, %{"type" => "too_large", "file" => file_path}} ->
+            # Handle too_large patches
+            Phoenix.View.render_to_string(DiffWeb.RenderView, "too_large.html", file: file_path)
+            |> sanitize_utf8()
+
+          {:ok, patch_data} when is_map(patch_data) ->
+            # Handle normal patches - raw patch data stored directly as map
+            # Check if this looks like a patch (has expected keys)
+            if Map.has_key?(patch_data, "chunks") or Map.has_key?(patch_data, :chunks) do
+              Phoenix.View.render_to_string(DiffWeb.RenderView, "patch.html",
+                patch: atomize_patch(patch_data)
+              )
+              |> sanitize_utf8()
+            else
+              "<div class='patch-error'>Invalid patch format</div>"
+            end
+
+          {:error, _} ->
+            "<div class='patch-error'>Invalid patch data</div>"
+        end
 
       {:error, _reason} ->
         "<div class='patch-error'>Failed to load patch</div>"
     end
   end
 
+  # Convert string keys back to atom keys for patch rendering
+  # Also handle converting lists back to tuples where needed
+  defp atomize_patch(patch) when is_map(patch) do
+    patch
+    |> Enum.map(fn {k, v} ->
+      atom_key = if is_binary(k), do: String.to_existing_atom(k), else: k
+
+      converted_value =
+        case {atom_key, v} do
+          # Convert index field back to tuple (was {nil, nil, nil} originally)
+          {:index, [a, b, c]} -> {a, b, c}
+          {"index", [a, b, c]} -> {a, b, c}
+          _ -> atomize_patch(v)
+        end
+
+      {atom_key, converted_value}
+    end)
+    |> Map.new()
+  rescue
+    ArgumentError ->
+      # If atom doesn't exist, return original map with string keys
+      patch
+      |> Enum.map(fn {k, v} ->
+        converted_value =
+          case {k, v} do
+            {"index", [a, b, c]} -> {a, b, c}
+            _ -> atomize_patch(v)
+          end
+
+        {k, converted_value}
+      end)
+      |> Map.new()
+  end
+
+  defp atomize_patch(list) when is_list(list) do
+    Enum.map(list, &atomize_patch/1)
+  end
+
+  defp atomize_patch(value), do: value
+
+  # Convert structs to maps recursively for JSON encoding
+  defp struct_to_map(%{__struct__: _} = struct) do
+    struct
+    |> Map.from_struct()
+    |> Enum.map(fn {k, v} -> {k, struct_to_map(v)} end)
+    |> Map.new()
+  end
+
+  defp struct_to_map(tuple) when is_tuple(tuple) do
+    # Convert tuples to lists for JSON compatibility
+    tuple
+    |> Tuple.to_list()
+    |> Enum.map(&struct_to_map/1)
+  end
+
+  defp struct_to_map(list) when is_list(list) do
+    Enum.map(list, &struct_to_map/1)
+  end
+
+  defp struct_to_map(map) when is_map(map) do
+    map
+    |> Enum.map(fn {k, v} -> {k, struct_to_map(v)} end)
+    |> Map.new()
+  end
+
+  defp struct_to_map(value), do: value
+
+  # Sanitize all binary content in patch data to prevent JSON encoding errors
+  defp sanitize_patch_content(%{__struct__: _} = struct) do
+    struct
+    |> Map.from_struct()
+    |> Enum.map(fn {k, v} -> {k, sanitize_patch_content(v)} end)
+    |> Map.new()
+    |> then(fn map -> struct(struct.__struct__, map) end)
+  end
+
+  defp sanitize_patch_content(list) when is_list(list) do
+    Enum.map(list, &sanitize_patch_content/1)
+  end
+
+  defp sanitize_patch_content(map) when is_map(map) do
+    map
+    |> Enum.map(fn {k, v} -> {k, sanitize_patch_content(v)} end)
+    |> Map.new()
+  end
+
+  defp sanitize_patch_content(binary) when is_binary(binary) do
+    sanitize_utf8(binary)
+  end
+
+  defp sanitize_patch_content(value), do: value
+
   defp parse_diff(diff) do
     case String.split(diff, ":", trim: true) do
       [app, from, to] -> {app, from, to, build_url(app, from, to)}