fix bugs

bug-tracking.md

@@ -1,186 +1,148 @@
 # Bug Tracking
 
+## Status Legend
+- **Fixed** — resolved and verified
+- **Open** — not yet addressed
+- **Wontfix** — accepted design/known limitation
+
+---
+
 ## Critical
 
-### 1. MQTT enabled checkbox can never be unchecked
+### 1. ~~All settings forms broken (send URL-encoded, API expected JSON)~~
 
-**File:** `spectra/web/templates/settings.html:90–97`, `spectra/web/server.py:118–120`
+**Fixed:** Every htmx form on the settings page (saturation, orientation, MQTT, schedule, Unsplash) sent URL-encoded form data (`application/x-www-form-urlencoded`) but the `/api/config/<section>/<key>` PUT endpoint used `request.get_json(force=True, silent=True)`. URL-encoded data is not valid JSON, so `get_json` returned `None` → the endpoint aborted with 400. Result: **every single settings form returned 400 in the browser**. The MQTT checkbox bug (old #1) was a symptom of this larger issue.
 
-**Problem:** The MQTT enable/disable checkbox uses htmx to PUT `{"value": "true"}` to `/api/config/mqtt/enabled`. When the user unchecks the box, the browser omits the field entirely (standard HTML behaviour for unchecked checkboxes). The server then receives an empty body, hits the `if data is None or "value" not in data: abort(400)` guard, and returns a 400 error. Result: MQTT can never be turned off via the web UI.
+**Fix** (`spectra/web/server.py`): `api_config_set()` now tries JSON first, then falls back to `request.form.get("value")`. A `_coerce_form_value(raw, current)` helper infers the correct Python type (bool/int/float/str) from the form string by comparing against the current config value's type.
 
-**To reproduce:** Go to Settings → MQTT → uncheck "Enable MQTT" → click Save. Browser sends PUT without `value` → server returns 400.
+### 2. ~~API config tests wrote to real `~/.config/spectra/config.yaml`~~
 
-**Fix:** Add a hidden input before the checkbox with the same `name` and `value="false"`. The checkbox's value (`"true"`) overrides the hidden when checked.
+**Fixed:** `ConfigManager._write()` resolved the config path by searching `CONFIG_SEARCH_PATHS`, and when no file existed (test environment), fell back to `~/.config/spectra/config.yaml`. Every test that called `PATCH /api/config` or `PUT /api/config/<section>/<key>` wrote test data into the real user's config file — **potential data corruption**.
 
-```html
+**Fix** (`tests/conftest.py`): The `app` fixture now writes a minimal `config.yaml` into the temp directory and passes its path to `create_app(config_path=...)`. The ConfigManager writes back to the same temp file.
-<label>
+
-  <input type="hidden" name="value" value="false">
+---
-  <input type="checkbox" id="mqtt-enabled" name="value" value="true"
+
-         {{ 'checked' if config.get('mqtt', {}).get('enabled', False) }}>
+## High
-  Enable MQTT
+
-</label>
+### 3. `_lookup_image_path` crashes if DB/table doesn't exist
-```
+
+**File:** `spectra/mqtt.py:19–25`
+
+**Problem:** When the web server hasn't created the database yet (or the `image` table is missing), `_lookup_image_path` raises `sqlite3.OperationalError: no such table: image`. The MQTT message handler catches this in paho, but the error is unhandled at the application level.
+
+**Fixed:** Wrapped the `conn.execute()` call in `try/except sqlite3.OperationalError`, logging a warning and returning `None`.
+
+### 4. `_MIME_MAP` dict redefined on every file download request
+
+**File:** `spectra/web/server.py:240`
+
+**Problem:** The `api_image_file()` route handler defined a `_MIME_MAP` dict inside the function body on every request. This is a minor performance issue — dictionary creation is cheap, but the dict is constant data that should be a module-level constant.
+
+**Fixed:** Moved `_MIME_MAP` to module level.
+
+### 5. Dead link `/docs/mqtt.md` in settings page
+
+**File:** `spectra/web/templates/settings.html:138`
+
+**Problem:** The settings template has `<a href="/docs/mqtt.md">MQTT docs</a>`. No Flask route serves this path, so browsers get a 404.
+
+**Fixed:** Replaced with text: `<code>docs/mqtt.md</code>` (no link).
+
+### 6. `_generate_thumbnail` leaks file descriptor
+
+**File:** `spectra/web/server.py:400–406`
+
+**Problem:** `PILImage.open(src)` without a context manager leaves the file handle open until garbage collection. On long-running servers serving many thumbnails, this can exhaust file descriptors.
+
+**Fixed:** Changed to `with PILImage.open(src) as img:`.
+
+### 7. Dead code: `TRIGGER_PATH` and `_trigger_path()`
+
+**File:** `spectra/web/server.py:69–70`
+
+**Problem:** `app.config["TRIGGER_PATH"]` was set but never read by any code. The `_trigger_path()` function was only used for that config key.
+
+**Fixed:** Removed both the config key and the function.
+
+### 8. Redundant `section_data` variable in `api_config_set`
+
+**File:** `spectra/web/server.py:122–124`
+
+**Problem:** `section_data = cm.config.get(section, {})` was assigned but only used for `isinstance(section_data, dict)` check. The old redundant mutation `section_data[key] = data["value"]` was already removed in a previous fix, leaving the variable as dead code.
+
+**Fixed:** Removed the `section_data` check entirely — `cm.set_nested()` handles missing sections.
 
 ---
 
 ## Medium
 
-### 2. Gallery delete shows "Saved" instead of removing the card
+### 9. `api_display_status` always reports `simulate: false`
 
-**Files:** `spectra/web/templates/gallery.html:84`, `spectra/web/static/js/app.js:9–11`
+**File:** `spectra/web/server.py:148`
 
-**Problem:** The Gallery's delete button uses `hx-delete` targeting `#gallery-msg`. After a successful delete, the server returns `{"status": "deleted"}`. The htmx response handler in `app.js` checks `data.status === 'triggered'` → shows trigger message, otherwise shows "Saved". Since `'deleted' !== 'triggered'`, the user sees "Saved" instead of "Deleted". The image card remains visible until the page is manually refreshed.
+**Problem:** `cm.get("display", "simulate")` reads a config key that doesn't exist (`display.simulate` is set via CLI `--simulate`, not in the config file). The field always reports `false` even when the display loop runs with `--simulate`.
 
-**Severity:** UX/data mismatch — user has no confirmation the image was actually deleted.
+**Fixed:** Removed the `simulate` field from the API response and the dashboard template. The web server and CLI are separate processes; the web server cannot know the display loop's `--simulate` state.
 
-**Fix options:**
+### 10. `_assert_safe_path` TOCTOU race
-- **Option A:** Make the delete endpoint return `{"status": "triggered"}` instead (hacky).
-- **Option B:** Update `app.js` to check for `data.status === 'deleted'` and show "Deleted" + remove the card from the DOM.
-- **Option C:** Use htmx's `hx-target="closest article" hx-swap="outerHTML"` on the delete button so the card is removed directly from the DOM without needing a message.
 
-**Recommended (Option C):** Change the delete button to:
+**File:** `spectra/web/server.py:319–323`
 
-```html
+**Problem:** The path safety check (`real_file.startswith(real_store + os.sep)`) and the subsequent `send_file()` are not atomic. A symlink could be swapped in between the check and the file read. An attacker needs write access to the uploads directory to exploit this.
-<button class="secondary"
-        hx-delete="/api/images/{{ img.id }}"
-        hx-target="closest article"
-        hx-swap="outerHTML swap:0.5s"
-        hx-confirm="Delete this image?">Delete</button>
-```
 
-Then remove the `#gallery-msg` target from the delete — the card disappears inline.
+**Status:** **Wontfix** — requires filesystem write access to exploit; mitigation would add complexity with minimal security gain.
 
-### 3. `MqttClient.reconfigure()` calls `self.__init__()` directly
+### 11. No cascade delete on Rotation FK
 
-**File:** `spectra/mqtt.py:85`
+**File:** `spectra/web/models.py:31`
 
-**Problem:** `reconfigure()` calls `self.__init__(new_config)` to re-initialise the instance. While this works (Python allows it), it's highly unusual, confuses static analysis tools, and breaks if a subclass overrides `__init__` differently. If the config is unchanged but the background thread has died, the method also unnecessarily destroys and recreates the client.
+**Problem:** `Rotation.image_id` has a foreign key to `image.id` but no `ondelete="CASCADE"`. Image deletion manually cleans up rotation entries (`Rotation.query.filter_by(image_id=...).delete()`). If a new deletion path is added without this cleanup, the FK constraint raises an error.
 
-**Fix:** Extract the attribute setup into a private `_setup` method called from both `__init__` and `reconfigure`:
+**Status:** **Wontfix** — explicit cleanup in the single delete route is sufficient for current code. Add cascade if new deletion paths are added.
-
-```python
-def __init__(self, config):
-    self._setup(config)
-    self._client = mqtt.Client(client_id=self.client_id, protocol=mqtt.MQTTv311)
-    ...
-
-def _setup(self, config):
-    self.broker = config.get("broker", "localhost")
-    self.port = config.get("port", 1883)
-    self.prefix = config.get("topic_prefix", "spectra")
-    self.client_id = config.get("client_id", "spectra-display")
-    self._username = config.get("username", "")
-    self._password = config.get("password", "")
-
-def reconfigure(self, new_config):
-    ...
-    self.stop()
-    self._setup(new_config)
-    self._client = mqtt.Client(client_id=self.client_id, protocol=mqtt.MQTTv311)
-    if self._username:
-        self._client.username_pw_set(self._username, self._password)
-    self._client.on_connect = self._on_connect
-    self._client.on_disconnect = self._on_disconnect
-    self._client.on_message = self._on_message
-    self.start()
-```
 
 ---
 
 ## Low
 
-### 4. Unsplash filename collision on duplicate fetch
+### 12. Object URLs never revoked in upload page
-
-**File:** `spectra/library.py:56`
-
-**Problem:** The filename is `f"unsplash_{unsplash_id}.png"`. If the same Unsplash photo is fetched twice (e.g. because the query doesn't change and there aren't many results), the second fetch overwrites the first file. Two DB records now reference the same filepath. If one record is deleted, `os.remove(dest)` succeeds, leaving the other record with a dangling path. On the next attempt to serve/thumbnail that record, the endpoint returns 404 from `send_file`.
-
-**Fix:** Add a UUID suffix to make filenames unique:
-
-```python
-filename = f"unsplash_{unsplash_id}_{uuid.uuid4().hex[:8]}.png"
-```
-
-Before saving, also check if the DB already has a record for this `unsplash_id` and skip the insert (avoid duplicates entirely):
-
-```python
-conn = sqlite3.connect(_db_path())
-existing = conn.execute("SELECT id FROM image WHERE source='unsplash' AND unsplash_id=?", (unsplash_id,)).fetchone()
-if existing:
-    conn.close()
-    os.remove(dest)  # clean up the duplicate file
-    return existing[0]
-```
-
-### 5. Object URLs never revoked in upload page
 
 **File:** `spectra/web/templates/upload.html:156`
 
-**Problem:** `URL.createObjectURL(file)` creates a blob URL for the file preview, but it is never released with `URL.revokeObjectURL()`. For a typical session (a handful of files), the memory impact is negligible. If a user uploads hundreds of files in one session, blob URLs accumulate in memory until the page is reloaded.
+**Problem:** `URL.createObjectURL(file)` creates blob URLs for file previews but never releases them with `URL.revokeObjectURL()`. For typical sessions (a few files) the memory impact is negligible.
 
-**Fix:** Store the URL on the file item element and revoke it when the item is removed or after a failed upload:
+**Status:** **Wontfix** — mitigated by the upload page: when a file's upload completes or fails, the `updateFileStatus` function checks `badgeClass === 'badge-ok' || badgeClass === 'badge-err'` and calls `URL.revokeObjectURL(blobUrl)` (lines 180–183). Blob URLs are already cleaned up per-file.
 
-```javascript
+### 13. Unsplash filename collision on duplicate fetch
-function renderFile(file) {
-    var url = URL.createObjectURL(file);
-    // ... store url for later revocation
-    fileItem.dataset.blobUrl = url;
-}
 
-function cleanupFileItem(el) {
+**File:** `spectra/library.py:56`
-    var url = el.dataset.blobUrl;
-    if (url) URL.revokeObjectURL(url);
-}
-```
 
-### 6. Dead CSS rules for old upload form
+**Problem:** Original filename `f"unsplash_{unsplash_id}.png"` could collide if the same photo is fetched twice. **Already fixed** in previous session: filename now uses `f"unsplash_{unsplash_id}_{uuid.uuid4().hex[:8]}.png"` and a dedup check on `unsplash_id` prevents duplicate DB records.
-
-**File:** `spectra/web/static/css/app.css:20`
-
-**Problem:** The CSS rule `#upload-result:empty { display: none; }` targets an element that was used by the old htmx-based upload form. The new multi-file upload form uses `#file-list`, `#upload-options`, and `#upload-summary` instead. The `#upload-result` element no longer exists anywhere in the templates.
-
-**Other dead selectors on the same line:** `#gallery-msg:empty`, `#action-result:empty`, `#unsplash-result:empty`, `#saturation-result:empty`, `#schedule-result:empty`.
-
-- `#gallery-msg` — still used by gallery delete/show htmx responses (if changed to `closest article` approach in bug #2, this becomes dead too)
-- `#action-result` — used by dashboard quick actions
-- `#unsplash-result`, `#saturation-result`, `#schedule-result` — used by settings page
-
-**Fix:** Remove only `#upload-result:empty` from the selector list. Keep the others as they are still in use.
 
 ---
 
-## Cosmetic / Design
+## Previously Fixed
 
-### 7. Inconsistent `process_image` usage: processed dimensions stored vs originals shown
+### Old #1 — MQTT checkbox can never be unchecked
+**Fixed:** Hidden `<input type="hidden" name="value" value="false">` added before the checkbox. Resolved as a side-effect of Bug #1 (form data now properly accepted with type coercion).
 
-**File:** `spectra/library.py:61`, `spectra/web/server.py:182–183`
+### Old #2 — Gallery delete shows "Saved"
+**Fixed:** Delete button uses `hx-target="closest article" hx-swap="delete"` with `hx-confirm`. The card is removed from the DOM directly.
 
-**Design issue:** `library.py` saves the processed (cropped + resized) dimensions as width/height in the DB, but stores the *original* unprocessed image on disk. The gallery shows the original file (via `/api/images/<id>/file`) which may have different dimensions than what is stored. The preview endpoint re-processes the original on demand, so the stored dimensions are only used for display purposes (and are somewhat misleading).
+### Old #3 — `MqttClient.reconfigure()` calls `__init__` directly
+**Fixed:** Extracted `_setup()` and `_create_client()` methods from `__init__`, called by both `__init__` and `reconfigure()`.
 
-This is intentional — the stored dimensions represent "how the image appears on the display" — but it could confuse API consumers who see width/height that doesn't match the actual file dimensions. Documentation should clarify this.
+### Old #4 — Unsplash filename collision
+**Fixed:** See #13 above.
 
-### 8. `api_config_set` double-mutates config
+### Old #5 — Object URLs never revoked
+**Fixed:** See #12 above — already handled in per-file cleanup.
 
-**File:** `spectra/web/server.py:124–125`
+### Old #6 — Dead CSS `#upload-result:empty`
+**Fixed:** Selector removed from `app.css`.
 
-**Design issue:** `section_data[key] = data["value"]` mutates `cm.config` in place (because `section_data` is a reference to `cm.config[section]`). Then `cm.set_nested()` does the same mutation again and calls `_write()`. The config is correct after both operations, but the first mutation is written only if `set_nested` completes, so this isn't a data-loss risk — just redundant code.
+### Old #8 — `api_config_set` double-mutates config
+**Fixed:** Redundant `section_data[key] = data["value"]` mutation removed.
 
-**Fix:** Remove the redundant mutation; only `cm.set_nested()` is needed:
+### Old #9 — Gallery delete test missing
-
+**Fixed:** `tests/test_gallery.py` added with upload-delete flow E2E test.
-```python
-# Remove line 124: section_data[key] = data["value"]
-cm.set_nested([section, key], data["value"])
-return jsonify({key: data["value"]})
-```
-
----
-
-## Tests
-
-### 9. ~~Gallery delete test missing from test suite~~
-
-**Fixed:** `tests/test_gallery.py` adds an end-to-end test (`test_upload_delete_flow`) that uploads an
-image (`POST /api/images`), confirms `total > 0` via `GET /api/images`, deletes it, then confirms
-`total` decreased by 1. Run with `pytest tests/`.
-
-**Dev dependency:** `pytest>=7.0.0` and `pytest-flask>=1.2.0` added under `[project.optional-dependencies] dev` in `pyproject.toml`. Install with `pip install "spectra-display[dev]"`.

spectra/mqtt.py

@@ -19,10 +19,15 @@ def _db_path():
 def _lookup_image_path(image_id):
     conn = sqlite3.connect(_db_path())
     conn.execute("PRAGMA journal_mode=WAL")
+    try:
         cur = conn.execute("SELECT filepath FROM image WHERE id = ?", (image_id,))
         row = cur.fetchone()
-    conn.close()
         return row[0] if row else None
+    except sqlite3.OperationalError:
+        logger.warning("Database table 'image' does not exist yet")
+        return None
+    finally:
+        conn.close()
 
 
 class MqttClient:

spectra/web/server.py

@@ -25,6 +25,7 @@ logger = logging.getLogger(__name__)
 _ALLOWED_EXTENSIONS = {".png", ".jpg", ".jpeg", ".gif", ".bmp", ".webp"}
 _MAX_UPLOAD_SIZE = 20 * 1024 * 1024
 _THUMBNAIL_SIZE = (320, 240)
+_MIME_MAP = {"jpg": "jpeg", "jpeg": "jpeg", "png": "png", "gif": "gif", "bmp": "bmp", "webp": "webp"}
 
 
 def create_app(config_path=None):
@@ -35,7 +36,6 @@ def create_app(config_path=None):
         SQLALCHEMY_DATABASE_URI="sqlite:///" + _db_path(),
         SQLALCHEMY_TRACK_MODIFICATIONS=False,
         IMAGE_STORE=_image_store_path(),
-        TRIGGER_PATH=_trigger_path(),
     )
 
     app._config_manager = ConfigManager(config_path)
@@ -66,10 +66,6 @@ def _image_store_path():
     return str(store)
 
 
-def _trigger_path():
-    return str(_cache_dir() / "trigger.json")
-
-
 def _register_routes(app):
     cm = app._config_manager
     store = app.config["IMAGE_STORE"]
@@ -117,14 +113,16 @@ def _register_routes(app):
     @app.route("/api/config/<section>/<key>", methods=["PUT"])
     def api_config_set(section, key):
         data = request.get_json(force=True, silent=True)
-        if data is None or "value" not in data:
+        if data is not None and "value" in data:
-            abort(400, "Request body must contain a 'value' field")
+            value = data["value"]
-        section_data = cm.config.get(section, {})
-        if isinstance(section_data, dict):
-            cm.set_nested([section, key], data["value"])
         else:
-            abort(400, "Invalid config section")
+            raw = request.form.get("value")
-        return jsonify({key: data["value"]})
+            if raw is None:
+                abort(400, "Request body must contain a 'value' field")
+            current = cm.get(section, key)
+            value = _coerce_form_value(raw, current)
+        cm.set_nested([section, key], value)
+        return jsonify({key: value})
 
     # --- Display API ---
 
@@ -144,12 +142,9 @@ def _register_routes(app):
         last_shown = (
             Image.query.order_by(Image.last_shown.desc()).first()
         )
-        from ..display import InkyDisplay
-        sim = cm.get("display", "simulate") or False
         width = cm.get("display", "resolution", "width") or 1600
         height = cm.get("display", "resolution", "height") or 1200
         return jsonify({
-            "simulate": sim,
             "resolution": {"width": width, "height": height},
             "pending_trigger": trigger,
             "last_shown": {
@@ -237,7 +232,6 @@ def _register_routes(app):
             abort(404)
         _assert_safe_path(image.filepath, store)
         ext = image.filename.rsplit(".", 1)[-1].lower() if "." in image.filename else ""
-        _MIME_MAP = {"jpg": "jpeg", "jpeg": "jpeg", "png": "png", "gif": "gif", "bmp": "bmp", "webp": "webp"}
         mime = "image/" + _MIME_MAP.get(ext, "png")
         return send_file(image.filepath, mimetype=mime)
 
@@ -399,7 +393,7 @@ def _image_to_dict(image):
 
 def _generate_thumbnail(src, dest, size=_THUMBNAIL_SIZE):
     try:
-        img = PILImage.open(src)
+        with PILImage.open(src) as img:
             img.thumbnail(size, PILImage.LANCZOS)
             img.save(dest, "PNG")
     except Exception as e:
@@ -426,6 +420,24 @@ def _register_template_filters(app):
             return ""
 
 
+def _coerce_form_value(raw, current):
+    if current is None:
+        return raw
+    if isinstance(current, bool):
+        return raw.lower() in ("true", "1", "yes")
+    if isinstance(current, int):
+        try:
+            return int(raw)
+        except ValueError:
+            return raw
+    if isinstance(current, float):
+        try:
+            return float(raw)
+        except ValueError:
+            return raw
+    return raw
+
+
 def _register_error_handlers(app):
     @app.errorhandler(400)
     def bad_request(e):

spectra/web/templates/index.html

@@ -8,7 +8,6 @@
   <article>
     <header>Display Status</header>
     <div id="display-status">
-      <p><strong>Simulation:</strong> <span id="status-simulate">—</span></p>
       <p><strong>Resolution:</strong> <span id="status-resolution">—</span></p>
       <p><strong>Last image:</strong> <span id="status-image">—</span></p>
       <p><strong>Shown at:</strong> <span id="status-timestamp">—</span></p>
@@ -58,7 +57,6 @@
     try {
       const resp = await fetch('/api/display/status');
       const data = await resp.json();
-      document.getElementById('status-simulate').textContent = data.simulate ? 'Yes' : 'No';
       document.getElementById('status-resolution').textContent = data.resolution.width + '×' + data.resolution.height;
       document.getElementById('status-trigger').textContent = data.pending_trigger
         ? (data.pending_trigger.action || 'Unknown')

spectra/web/templates/settings.html

@@ -135,7 +135,7 @@
     </form>
 
     <p id="mqtt-result"></p>
-    <p><small>See <a href="/docs/mqtt.md">MQTT docs</a> for available commands and status topics.</small></p>
+    <p><small>See <code>docs/mqtt.md</code> in the repository for available commands and status topics.</small></p>
   </article>
 </div>
 {% endblock %}

tests/conftest.py

@@ -1,10 +1,10 @@
 import io
-import json
 import os
 import tempfile
 from pathlib import Path
 
 import pytest
+import yaml
 from PIL import Image as PILImage
 
 from spectra.web.server import create_app
@@ -18,11 +18,16 @@ def app():
     with tempfile.TemporaryDirectory() as tmpdir:
         tmp = Path(tmpdir)
         cache_dir = tmp / "cache"
+        config_path = tmp / "config.yaml"
+
+        # Write a minimal config so ConfigManager writes into the temp dir
+        with open(config_path, "w") as f:
+            yaml.dump({"display": {"saturation": 0.5}}, f)
 
         old_environ = os.environ.get("SPECTRA_CACHE_DIR")
         os.environ["SPECTRA_CACHE_DIR"] = str(cache_dir)
 
-        app = create_app()
+        app = create_app(config_path=str(config_path))
 
         yield app
 

tests/test_api.py

@@ -203,7 +203,6 @@ class TestDisplayAPI:
         resp = client.get("/api/display/status")
         assert resp.status_code == 200
         data = resp.get_json()
-        assert "simulate" in data
         assert "resolution" in data
         assert "pending_trigger" in data
 
@@ -318,12 +317,14 @@ class TestCORSAndErrors:
         data = resp.get_json()
         assert "error" in data
 
-    def test_url_encoded_form_not_accepted(self, client):
+    def test_url_encoded_form_accepted(self, client):
         resp = client.put(
             "/api/config/display/saturation",
             data={"value": "0.5"},
         )
-        assert resp.status_code == 400 or resp.status_code == 200
+        assert resp.status_code == 200
+        resp = client.get("/api/config/display")
+        assert resp.get_json()["saturation"] == 0.5
 
 
 class TestGallery: