187 lines
8.9 KiB
Markdown
187 lines
8.9 KiB
Markdown
# Bug Tracking
|
||
|
||
## Critical
|
||
|
||
### 1. MQTT enabled checkbox can never be unchecked
|
||
|
||
**File:** `spectra/web/templates/settings.html:90–97`, `spectra/web/server.py:118–120`
|
||
|
||
**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.
|
||
|
||
**To reproduce:** Go to Settings → MQTT → uncheck "Enable MQTT" → click Save. Browser sends PUT without `value` → server returns 400.
|
||
|
||
**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.
|
||
|
||
```html
|
||
<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) }}>
|
||
Enable MQTT
|
||
</label>
|
||
```
|
||
|
||
---
|
||
|
||
## Medium
|
||
|
||
### 2. Gallery delete shows "Saved" instead of removing the card
|
||
|
||
**Files:** `spectra/web/templates/gallery.html:84`, `spectra/web/static/js/app.js:9–11`
|
||
|
||
**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.
|
||
|
||
**Severity:** UX/data mismatch — user has no confirmation the image was actually deleted.
|
||
|
||
**Fix options:**
|
||
- **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:
|
||
|
||
```html
|
||
<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.
|
||
|
||
### 3. `MqttClient.reconfigure()` calls `self.__init__()` directly
|
||
|
||
**File:** `spectra/mqtt.py:85`
|
||
|
||
**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.
|
||
|
||
**Fix:** Extract the attribute setup into a private `_setup` method called from both `__init__` and `reconfigure`:
|
||
|
||
```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
|
||
|
||
**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.
|
||
|
||
**Fix:** Store the URL on the file item element and revoke it when the item is removed or after a failed upload:
|
||
|
||
```javascript
|
||
function renderFile(file) {
|
||
var url = URL.createObjectURL(file);
|
||
// ... store url for later revocation
|
||
fileItem.dataset.blobUrl = url;
|
||
}
|
||
|
||
function cleanupFileItem(el) {
|
||
var url = el.dataset.blobUrl;
|
||
if (url) URL.revokeObjectURL(url);
|
||
}
|
||
```
|
||
|
||
### 6. Dead CSS rules for old upload form
|
||
|
||
**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
|
||
|
||
### 7. Inconsistent `process_image` usage: processed dimensions stored vs originals shown
|
||
|
||
**File:** `spectra/library.py:61`, `spectra/web/server.py:182–183`
|
||
|
||
**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).
|
||
|
||
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.
|
||
|
||
### 8. `api_config_set` double-mutates config
|
||
|
||
**File:** `spectra/web/server.py:124–125`
|
||
|
||
**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.
|
||
|
||
**Fix:** Remove the redundant mutation; only `cm.set_nested()` is needed:
|
||
|
||
```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]"`.
|