diff --git a/BUGS.md b/BUGS.md new file mode 100644 index 0000000..2e749de --- /dev/null +++ b/BUGS.md @@ -0,0 +1,284 @@ +# Known Bugs and Issues + +This document lists identified bugs and potential issues in the Labelmaker codebase. + +## Critical Issues + +### 1. Race Condition in Image Generation + +**Location**: `lib/labelmaker_web/controllers/label_controller.ex:21-31` + +**Status**: ⚠️ **UNRESOLVED** + +**Issue**: When multiple requests arrive simultaneously for the same non-existent image, both will start generating it concurrently. Additionally, `send_file/3` is called immediately after starting generation without waiting for it to complete, which will cause a file-not-found error. + +```elixir +unless File.exists?(filepath) do + basic_settings(options) + |> outline_settings(options) + |> size_settings(options) + |> final_settings(options) + |> generate_image() +end + +conn +|> put_resp_content_type("image/png") +|> send_file(200, filepath) # Called before generation completes! +``` + +**Impact**: High - Will cause 500 errors when requests race or when generation is slow + +**Suggested Fix**: +- Use a mutex/lock mechanism (e.g., via `:global` or a GenServer) to ensure only one process generates a given image +- Ensure `send_file` is only called after generation completes +- Consider using a temporary file with atomic rename to prevent serving partially-written images + +```elixir +# Example approach with synchronization +case ensure_image_exists(filepath, options) do + :ok -> + conn + |> put_resp_content_type("image/png") + |> send_file(200, filepath) + {:error, reason} -> + conn + |> put_status(500) + |> json(%{error: "Failed to generate image"}) +end +``` + +### 2. Crash on Invalid Numeric Inputs + +**Location**: `lib/labelmaker_web/tools.ex:87-88, 136-137, 140-141` + +**Status**: ⚠️ **UNRESOLVED** + +**Issue**: When `height`, `width`, or `size` parameters contain non-numeric strings (e.g., "abc", "12.5px"), `String.to_integer/1` raises an `ArgumentError` that isn't caught, causing the entire request to crash. + +```elixir +defp process_height(height, _parameters) do + height |> String.to_integer() |> max(0) |> min(Constants.max_height()) +end + +defp process_width(width, _parameters) do + width |> String.to_integer() |> max(0) |> min(Constants.max_width()) +end + +defp calculate_preview_height(parameters) do + size = parameters["size"] |> String.to_integer() + # ... +end +``` + +**Impact**: High - User-supplied invalid input causes crashes + +**Suggested Fix**: +```elixir +defp process_height(height, _parameters) do + case Integer.parse(height) do + {int, _} -> int |> max(0) |> min(Constants.max_height()) + :error -> String.to_integer(Constants.defaults().height) + end +end + +defp process_width(width, _parameters) do + case Integer.parse(width) do + {int, _} -> int |> max(0) |> min(Constants.max_width()) + :error -> String.to_integer(Constants.defaults().width) + end +end + +defp calculate_preview_height(parameters) do + size = case Integer.parse(parameters["size"]) do + {int, _} -> int + :error -> 72 # default + end + rows = calculate_rows(parameters["label"]) + size + size * rows +end +``` + +## Moderate Issues + +### 3. Missing Error Handling on ImageMagick Command + +**Location**: `lib/labelmaker_web/controllers/label_controller.ex:112` + +**Status**: ⚠️ **UNRESOLVED** + +**Issue**: Pattern matching on `{_, 0}` will crash if ImageMagick returns a non-zero exit code (e.g., invalid arguments, ImageMagick not installed, permission errors, disk full). + +```elixir +{_, 0} = System.cmd("magick", args) +``` + +**Impact**: Moderate - Crashes on ImageMagick errors instead of gracefully handling + +**Suggested Fix**: +```elixir +case System.cmd("magick", args, stderr_to_stdout: true) do + {_output, 0} -> + :ok + {error, code} -> + require Logger + Logger.error("ImageMagick failed with code #{code}: #{error}") + {:error, :image_generation_failed} +end +``` + +Then handle the error tuple in the controller's `show/2` function and return appropriate HTTP error response. + +### 4. Potential nil Crash on Font Lookup + +**Location**: `lib/labelmaker_web/tools.ex:34-35` + +**Status**: ⚠️ **UNRESOLVED** + +**Issue**: If an invalid font is passed that isn't in the font_map, the map lookup returns `nil`. While later validation (line 64) filters it out, if defaults somehow don't include `font`, `nil` could be passed to ImageMagick causing cryptic errors. + +```elixir +{:font, font} -> + {:font, Constants.font_map()[String.downcase(font)]} +``` + +**Impact**: Low-Moderate - Edge case but could cause ImageMagick errors + +**Suggested Fix**: +```elixir +{:font, font} -> + {:font, Map.get(Constants.font_map(), String.downcase(font), Constants.defaults().font)} +``` + +### 5. label_too_long Flag Logic Bug + +**Location**: `lib/labelmaker_web/tools.ex:40-41` + +**Status**: 🐛 **CONFIRMED BUG** + +**Issue**: The `label_too_long` flag is calculated AFTER the label has already been truncated in `process_label/1` (line 16-17). This means the flag will always be `false` because it's checking the length of the already-truncated label, not the original input. + +```elixir +# Line 16-17: Label gets truncated here +{"label", label} -> + {"label", process_label(label)} + +# Line 40-41: This checks the ALREADY TRUNCATED label +{:label_too_long, _} -> + {:label_too_long, String.length(parameters["label"]) > Constants.max_label_length()} +``` + +**Impact**: Low - UI warning for long labels never appears + +**Suggested Fix**: +Check the length before truncation, or pass the original length through: +```elixir +def process_parameters(parameters) do + # Calculate label_too_long before any processing + original_label_length = String.length(Map.get(parameters, "label", "")) + + parameters = + Constants.defaults() + |> Map.new(fn {k, v} -> {Atom.to_string(k), v} end) + |> Map.merge(parameters) + |> Map.put("_original_label_length", original_label_length) + # ... rest of processing + + # Later when setting the flag: + {:label_too_long, _} -> + {:label_too_long, parameters["_original_label_length"] > Constants.max_label_length()} +``` + +## Low Priority Issues + +### 6. Hash Collision Risk from inspect() + +**Location**: `lib/labelmaker_web/controllers/label_controller.ex:11-15` + +**Status**: ⚠️ **UNRESOLVED** + +**Issue**: Using `inspect/1` on a map for hashing is fragile. Map ordering isn't guaranteed to be consistent across Erlang/OTP versions or runtime conditions. Two identical maps could theoretically produce different inspect strings, leading to cache misses. + +```elixir +filename = + options + |> inspect() + |> (fn str -> :crypto.hash(:sha256, str) end).() + |> Base.encode16(case: :lower) +``` + +**Impact**: Low - Mostly theoretical, but could cause unnecessary cache misses + +**Suggested Fix**: Hash a consistently-ordered representation: +```elixir +filename = + options + |> Map.take([:label, :color, :font, :outline, :size, :width, :height, :align]) + |> Enum.sort() + |> :erlang.term_to_binary() + |> :crypto.hash(:sha256) + |> Base.encode16(case: :lower) +``` + +### 7. Missing Type Guards on String Operations + +**Location**: `lib/labelmaker_web/tools.ex:32, 35` + +**Status**: ⚠️ **UNRESOLVED** + +**Issue**: `String.downcase/1` is called on `align` and `font` parameters without verifying they're strings. Given the data flow this is unlikely to be a problem, but defensive programming would add guards. + +```elixir +{:align, align} -> + {:align, align |> String.downcase()} + +{:font, font} -> + {:font, Constants.font_map()[String.downcase(font)]} +``` + +**Impact**: Very Low - Input validation earlier in the pipeline makes this unlikely + +**Suggested Fix**: Add type guards or convert to string first: +```elixir +{:align, align} when is_binary(align) -> + {:align, String.downcase(align)} + +# Or alternatively: +{:align, align} -> + {:align, align |> to_string() |> String.downcase()} +``` + +## Fixed Issues + +### ✅ 8. ImageMagick Property Interpolation Warning + +**Location**: `lib/labelmaker_web/controllers/label_controller.ex:50, 65, 96` + +**Status**: ✅ **FIXED** + +**Issue**: Labels containing `%` characters (like "Test!@#$%" or URL-encoded text) caused ImageMagick to emit warnings about unknown properties like `%2`, `%20`, etc., because ImageMagick treats `%` as a special character for property interpolation. + +**Fix Applied**: All `%` characters are now escaped to `%%` in: +1. Label text passed to `label:` and `caption:` commands +2. Metadata stored in image comments + +```elixir +# Lines 47-50, 62-65 +escaped_label = + options.label + |> String.slice(0, Constants.max_label_length()) + |> String.replace("%", "%%") + +# Lines 91-96 +comment = + options + |> Map.drop([:filepath, :link]) + |> Jason.encode!() + |> inspect() + |> String.replace("%", "%%") +``` + +## Notes + +- All critical and moderate issues should be addressed before deploying to production +- The test suite (105 tests) currently passes but doesn't cover error conditions for many of these bugs +- Consider adding integration tests that specifically test error handling paths diff --git a/CLAUDE.md b/CLAUDE.md new file mode 100644 index 0000000..bfdf3d9 --- /dev/null +++ b/CLAUDE.md @@ -0,0 +1,152 @@ +# CLAUDE.md + +This file provides guidance to Claude Code (claude.ai/code) when working with code in this repository. + +## Project Overview + +Labelmaker is a Phoenix LiveView web application that generates text-based images via URL. Users can visit `labelmaker.xyz/Your Text` to instantly get an image with their text, with various customization options (font, color, outline, size). The app is designed for creating decals in Tabletop Simulator. + +## Technology Stack + +- **Elixir + Phoenix Framework** (Phoenix 1.7+ with LiveView) +- **ImageMagick** (`magick` command) for image generation +- **Tailwind CSS** for styling +- **esbuild** for JavaScript bundling + +## Common Commands + +### Development +```sh +# Install dependencies and set up assets +mix setup + +# Start Phoenix server (runs on http://localhost:4000) +mix phx.server + +# Start server with interactive Elixir shell +iex -S mix phx.server +``` + +### Testing +```sh +# Run all tests +mix test + +# Run a specific test file +mix test test/labelmaker_web/controllers/label_controller_test.exs + +# Run a specific test +mix test test/labelmaker_web/controllers/label_controller_test.exs:42 +``` + +### Assets +```sh +# Install asset build tools (Tailwind, esbuild) +mix assets.setup + +# Build assets for development +mix assets.build + +# Build assets for production (minified + digest) +mix assets.deploy +``` + +### Docker +```sh +docker build -t labelmaker . +docker run -p 4000:4000 labelmaker +``` + +## Architecture + +### Request Flow + +1. **Homepage (`/`)**: `LabelmakerWeb.Home` LiveView provides an interactive form where users can preview text with different styling options +2. **Image Generation (`/:label`)**: `LabelmakerWeb.LabelController.show/2` handles dynamic image generation + +### Core Components + +**`LabelmakerWeb.LabelController`** (lib/labelmaker_web/controllers/label_controller.ex) +- Main entry point for image generation requests +- Takes URL path (label text) and query parameters (color, font, outline, size, width, height, align) +- Generates SHA256 hash from parameters to create unique filename for caching +- Checks if cached image exists; if not, builds ImageMagick command and generates image +- Stores generated images in `priv/static/labels/` +- Returns image as PNG response + +**`LabelmakerWeb.Tools`** (lib/labelmaker_web/tools.ex) +- Contains parameter processing and validation logic +- `process_parameters/1`: Validates and normalizes user input against permitted values +- Handles two sizing modes: + - **Font mode** (default): Uses `label:` with `-pointsize` for natural text sizing + - **Width x Height mode**: Uses `caption:` with `-size WxH` for fixed dimensions +- `generate_link/1`: Creates URLs based on current parameters and sizing mode +- `process_label/1`: Handles `\n` to actual newline conversion for multi-line labels +- Input validation: filters colors, fonts, sizes, outlines against permitted lists + +**`LabelmakerWeb.Constants`** (lib/labelmaker_web/constants.ex) +- Single source of truth for all permitted values and defaults +- Defines available colors, fonts, sizes, alignments +- Font map supports shortcuts (e.g., "h" → "Helvetica", "cs" → "Comic-Sans-MS") +- Max dimensions: 1024x1024 pixels +- Max label length: 1024 characters +- Available fonts: Comic Sans, Courier, Georgia, Helvetica, Impact, Verdana + +**`LabelmakerWeb.Home`** (lib/labelmaker_web/live/home.ex) +- LiveView for homepage with interactive preview +- Handles real-time updates via `handle_event/3`: + - `update_label`: Updates label text and styling parameters + - `update_preview`: Changes preview background (gradient/solid) + - `update_sizing`: Toggles between font size mode and width×height mode + - `update_alignment`: Changes text alignment (left/center/right) +- Redirects to `/:label` route when user submits the form + +**`LabelmakerWeb.RadioComponent`** (lib/labelmaker_web/live/components/radio_component.ex) +- Reusable Phoenix Component for radio button groups +- Used in the homepage form for alignment selection + +### ImageMagick Command Construction + +The controller builds ImageMagick commands programmatically: + +1. **Basic settings**: background, fill color, font +2. **Outline settings**: stroke color and width (if not "none") +3. **Size settings**: Either pointsize + `label:` OR width×height + `caption:` +4. **Final settings**: Adds metadata comment and output filepath + +Example generated command: +```sh +magick -background none -fill black -font Helvetica -stroke white -strokewidth 1 -pointsize 72 label:Hello World output.png +``` + +### Sizing Modes + +The app supports two distinct sizing modes (controlled by `sizing` parameter): + +- **Font mode** (`sizing=font`): ImageMagick calculates image size based on font size; natural text rendering +- **Width × Height mode** (`sizing=wxh`): Fixed canvas dimensions with text wrapped/aligned within bounds + +### Parameter Processing + +All parameters flow through `Tools.process_parameters/1`: +1. Merge with defaults from `Constants.defaults()` +2. Process label (convert `\n` escapes) +3. Validate each parameter against permitted lists +4. Filter out invalid values +5. Calculate derived values (preview_height, rows, link) +6. Return merged map with all parameters + +### Caching Strategy + +Images are cached using SHA256 hash of the processed options map: +- Hash includes: label, color, font, outline, size, width, height, align +- Cached files stored in `priv/static/labels/` +- Before generating, checks if file exists +- No expiration mechanism (cache persists until manually cleared) + +## Important Notes + +- **ImageMagick dependency**: The `magick` command must be available in PATH +- **Alignment/Gravity mapping**: User-facing "left/center/right" maps to ImageMagick "west/center/east" gravity values (see `Tools.process_gravity/1`) +- **URL escaping**: Special characters in labels must be URL-encoded; `\n` is processed as literal newline +- **Main branch**: This repo uses `trunk` as the main branch (not `main` or `master`)