# 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