# Codebase Concerns

**Analysis Date:** 2026-03-23

## Tech Debt

**Global mutable state in library package:**
- Issue: The package uses package-level `var db *sql.DB`, `var mu sync.Mutex`, and `var webhookSecret string`. This makes the package non-reusable and harder to test — only one "instance" can exist per process.
- Files: `pkg/diunwebhook/diunwebhook.go` (lines 48-52)
- Impact: Cannot run multiple instances, cannot run tests in parallel safely, tight coupling to global state.
- Fix approach: Refactor to a struct-based design (e.g., `type Server struct { db *sql.DB; mu sync.Mutex; secret string }`) with methods instead of package functions. Priority: Medium.

**Module name is "awesomeProject":**
- Issue: The Go module is named `awesomeProject` (a Go IDE default placeholder), not a meaningful name like `github.com/user/diun-dashboard` or similar.
- Files: `go.mod` (line 1), `cmd/diunwebhook/main.go` (line 13), `pkg/diunwebhook/diunwebhook_test.go` (line 15)
- Impact: Confusing for contributors, unprofessional in imports, cannot be used as a Go library.
- Fix approach: Rename module to a proper path (e.g., `gitea.jeanlucmakiola.de/makiolaj/diun-dashboard`) and update all imports. Priority: Low.

**Empty error handlers on rows.Close():**
- Issue: Multiple `defer rows.Close()` wrappers silently swallow errors with empty `if err != nil {}` blocks.
- Files: `pkg/diunwebhook/diunwebhook.go` (lines 131-136, 248-253)
- Impact: Suppressed errors make debugging harder. Not functionally critical since close errors on read queries rarely matter, but the pattern is misleading.
- Fix approach: Either log the error or use a simple `defer rows.Close()` without the wrapper. Priority: Low.

**Silent error swallowing in tests:**
- Issue: Several tests do `if err != nil { return }` instead of `t.Fatal(err)`, silently passing on failure.
- Files: `pkg/diunwebhook/diunwebhook_test.go` (lines 38-40, 153-154, 228-231, 287-289)
- Impact: Tests can silently pass when they should fail, hiding bugs.
- Fix approach: Replace `return` with `t.Fatalf("...: %v", err)` in all test error checks. Priority: Medium.

**Ad-hoc SQL migration strategy:**
- Issue: Schema migrations are done inline with silent `ALTER TABLE` that ignores errors: `_, _ = db.Exec("ALTER TABLE updates ADD COLUMN acknowledged_at TEXT")`.
- Files: `pkg/diunwebhook/diunwebhook.go` (line 87)
- Impact: Works for a single column addition but does not scale. No version tracking, no rollback, no way to know which migrations have run.
- Fix approach: Introduce a `schema_version` table or use a lightweight migration library. Priority: Low (acceptable for current scope).

**INSERT OR REPLACE loses tag assignments:**
- Issue: `UpdateEvent()` uses `INSERT OR REPLACE` which deletes and re-inserts the row. Because `tag_assignments` references `updates.image` but there is no `ON DELETE CASCADE` on that FK (and SQLite FK enforcement may not be enabled), the assignment row becomes orphaned or the behavior is undefined.
- Files: `pkg/diunwebhook/diunwebhook.go` (line 109)
- Impact: When DIUN sends a new event for an already-tracked image, the tag assignment may be lost. Users would need to re-tag images after each update.
- Fix approach: Use `INSERT ... ON CONFLICT(image) DO UPDATE SET ...` (UPSERT) instead of `INSERT OR REPLACE`, or enable FK enforcement with `PRAGMA foreign_keys = ON` and add CASCADE. Priority: High.

**Foreign key enforcement not enabled:**
- Issue: SQLite does not enforce foreign keys by default. The `tag_assignments.tag_id REFERENCES tags(id) ON DELETE CASCADE` constraint exists in the schema but `PRAGMA foreign_keys = ON` is never executed.
- Files: `pkg/diunwebhook/diunwebhook.go` (lines 58-103)
- Impact: Deleting a tag may not cascade-delete assignments, leaving orphaned rows in `tag_assignments`. The test `TestDeleteTagHandler_CascadesAssignment` may pass due to the LEFT JOIN query hiding orphans rather than them actually being deleted.
- Fix approach: Add `db.Exec("PRAGMA foreign_keys = ON")` immediately after opening the database connection in `InitDB()`. Priority: High.

## Security Considerations

**No authentication on API endpoints:**
- Risk: All API endpoints (`GET /api/updates`, `PATCH /api/updates/*`, `GET/POST /api/tags`, etc.) are completely unauthenticated. Only `POST /webhook` supports optional token auth.
- Files: `cmd/diunwebhook/main.go` (lines 38-44), `pkg/diunwebhook/diunwebhook.go` (all handler functions)
- Current mitigation: The dashboard is presumably deployed on a private network.
- Recommendations: Add optional basic auth or token auth middleware for API endpoints. At minimum, document the assumption that the dashboard should not be exposed to the public internet. Priority: Medium.

**No request body size limit on webhook:**
- Risk: `json.NewDecoder(r.Body).Decode(&event)` reads the entire body without limit. A malicious client could send a multi-GB payload causing OOM.
- Files: `pkg/diunwebhook/diunwebhook.go` (line 179)
- Current mitigation: `ReadTimeout: 10 * time.Second` on the server provides some protection.
- Recommendations: Wrap `r.Body` with `http.MaxBytesReader(w, r.Body, maxSize)` (e.g., 1MB). Apply the same to `TagsHandler` POST and `TagAssignmentHandler`. Priority: Medium.

**No CORS headers configured:**
- Risk: In development the Vite proxy handles cross-origin, but if the API is accessed directly from a different origin in production, there are no CORS headers.
- Files: `cmd/diunwebhook/main.go` (lines 38-45)
- Current mitigation: SPA is served from the same origin as the API.
- Recommendations: Not urgent since the SPA and API share an origin. Document this constraint. Priority: Low.

**Webhook secret sent as raw Authorization header:**
- Risk: The webhook secret is compared against the raw `Authorization` header value, not using a standard scheme like `Bearer <token>`. This is non-standard but functionally fine.
- Files: `pkg/diunwebhook/diunwebhook.go` (lines 164-170)
- Current mitigation: Uses `crypto/subtle.ConstantTimeCompare` which prevents timing attacks.
- Recommendations: Consider supporting `Bearer <token>` format for standard compliance. Priority: Low.

## Performance Bottlenecks

**Frontend polls entire dataset every 5 seconds:**
- Problem: `GET /api/updates` returns ALL updates as a single JSON map. The query joins three tables every time. As the number of tracked images grows, both the query and the JSON payload grow linearly.
- Files: `frontend/src/hooks/useUpdates.ts` (line 4, `POLL_INTERVAL = 5000`), `pkg/diunwebhook/diunwebhook.go` (lines 120-161)
- Cause: No incremental/differential update mechanism. No pagination. No caching headers.
- Improvement path: Add `If-Modified-Since` / `ETag` support, or switch to Server-Sent Events (SSE) / WebSocket for push-based updates. Add pagination for large datasets. Priority: Medium (fine for <1000 images, problematic beyond).

**Global mutex on all write operations:**
- Problem: A single `sync.Mutex` serializes all database writes across all handlers.
- Files: `pkg/diunwebhook/diunwebhook.go` (line 49, used at lines 107, 224, 281, 317, 351, 369)
- Cause: SQLite single-writer limitation addressed with a process-level mutex.
- Improvement path: `SetMaxOpenConns(1)` already serializes at the driver level, so the mutex is redundant for correctness but adds belt-and-suspenders safety. For higher throughput, consider WAL mode (`PRAGMA journal_mode=WAL`) which allows concurrent reads. Priority: Low.

**GetUpdates() not protected by mutex but reads are not serialized:**
- Problem: `GetUpdates()` does not acquire the mutex, so it can read while a write is in progress. With `SetMaxOpenConns(1)`, the driver serializes connections, but the function could block waiting for the connection.
- Files: `pkg/diunwebhook/diunwebhook.go` (lines 120-161)
- Cause: Inconsistent locking strategy — writes lock the mutex, reads do not.
- Improvement path: Either lock reads too (for consistency) or enable WAL mode and document the strategy. Priority: Low.

## Scalability Limitations

**SQLite single-file database:**
- Current capacity: Suitable for hundreds to low thousands of tracked images.
- Limit: SQLite single-writer bottleneck. No replication. Database file grows unbounded since old updates are never purged.
- Scaling path: Add a retention/cleanup mechanism for old acknowledged updates. For multi-instance deployments, migrate to PostgreSQL. Priority: Low (appropriate for the use case).

**No data retention or cleanup:**
- Current capacity: Every image update is kept forever in the `updates` table.
- Limit: Database will grow indefinitely. No mechanism to archive or delete old, acknowledged entries.
- Scaling path: Add a configurable retention period (e.g., auto-delete acknowledged entries older than N days). Priority: Medium.

## Fragile Areas

**URL path parsing for route parameters:**
- Files: `pkg/diunwebhook/diunwebhook.go` (lines 219, 311)
- Why fragile: Image names and tag IDs are extracted via `strings.TrimPrefix(r.URL.Path, "/api/updates/")` and `strings.TrimPrefix(r.URL.Path, "/api/tags/")`. This works but is brittle — any change to the route prefix requires changing these strings in two places (handler + `main.go`).
- Safe modification: If adding new routes or refactoring, ensure the prefix strings stay in sync with `mux.HandleFunc` registrations in `cmd/diunwebhook/main.go`.
- Test coverage: Good — `TestDismissHandler_SlashInImageName` covers the tricky case of slashes in image names.

**Optimistic UI updates without rollback:**
- Files: `frontend/src/hooks/useUpdates.ts` (lines 60-84)
- Why fragile: `assignTag()` performs an optimistic state update before the API call. If the API call fails, the UI shows the new tag but the server still has the old one. No rollback occurs — only a `console.error`.
- Safe modification: Store previous state before optimistic update, restore on error.
- Test coverage: No frontend tests exist.

**Single monolithic handler file:**
- Files: `pkg/diunwebhook/diunwebhook.go` (380 lines)
- Why fragile: All database logic, HTTP handlers, data types, and initialization live in a single file. As features are added, this file will become increasingly difficult to navigate.
- Safe modification: Split into `models.go`, `storage.go`, `handlers.go`, and `init.go` within the same package.
- Test coverage: Good test coverage for existing functionality.

## Dependencies at Risk

**No pinned dependency versions in go.mod:**
- Risk: All Go dependencies are marked `// indirect` — the project has only one direct dependency (`modernc.org/sqlite`) but it is not explicitly listed as direct.
- Files: `go.mod`
- Impact: `go mod tidy` behavior may be unpredictable. The `go.sum` file provides integrity but the intent is unclear.
- Migration plan: Run `go mod tidy` and ensure `modernc.org/sqlite` is listed without the `// indirect` comment. Priority: Low.

## Missing Critical Features

**No frontend tests:**
- Problem: Zero test files exist for the React frontend. No unit tests, no integration tests, no E2E tests.
- Blocks: Cannot verify frontend behavior automatically, cannot catch regressions in UI logic (tag assignment, acknowledge flow, drag-and-drop).
- Priority: Medium.

**No "acknowledge all" or bulk operations:**
- Problem: Users must acknowledge images one by one. No bulk dismiss, no "acknowledge all in group" action.
- Blocks: Tedious workflow when many images have updates.
- Priority: Low.

**No dark/light theme toggle (hardcoded dark):**
- Problem: The UI uses CSS variables that assume a dark theme. No toggle or system preference detection.
- Files: `frontend/src/index.css`, `frontend/src/App.tsx`
- Blocks: Users who prefer light mode have no option.
- Priority: Low.

## Test Coverage Gaps

**No tests for TagAssignmentHandler edge cases:**
- What's not tested: Assigning a non-existent image (image not in `updates` table), assigning with `tag_id: 0` or negative values, malformed JSON bodies.
- Files: `pkg/diunwebhook/diunwebhook.go` (lines 333-379)
- Risk: Unknown behavior for invalid inputs.
- Priority: Low.

**No tests for concurrent tag operations:**
- What's not tested: Concurrent create/delete of tags, concurrent assign/unassign operations.
- Files: `pkg/diunwebhook/diunwebhook_test.go`
- Risk: Potential race conditions in tag operations under load.
- Priority: Low.

**No frontend test infrastructure:**
- What's not tested: All React components, hooks, drag-and-drop behavior, polling logic, optimistic updates.
- Files: `frontend/src/**/*.{ts,tsx}`
- Risk: UI regressions go undetected. The `useUpdates` hook contains business logic (polling, optimistic updates) that should be tested.
- Priority: Medium.

## Accessibility Concerns

**Drag handle only visible on hover:**
- Issue: The grip handle for drag-and-drop (`GripVertical` icon) has `opacity-0 group-hover:opacity-100`, making it invisible until hover. Keyboard-only and touch users cannot discover this interaction.
- Files: `frontend/src/components/ServiceCard.tsx` (line 96)
- Impact: Drag-and-drop is the only way to re-tag images. Users without hover capability cannot reorganize.
- Fix approach: Make the handle always visible (or visible on focus), and provide an alternative non-drag method for tag assignment (e.g., a dropdown). Priority: Medium.

**Delete button invisible until hover:**
- Issue: The tag section delete button has `opacity-0 group-hover:opacity-100`, same discoverability problem.
- Files: `frontend/src/components/TagSection.tsx` (line 62)
- Impact: Cannot discover delete action without hover.
- Fix approach: Keep visible or show on focus. Priority: Low.

**No skip-to-content link, no ARIA landmarks:**
- Issue: The page lacks skip navigation links and semantic ARIA roles beyond basic HTML.
- Files: `frontend/src/App.tsx`, `frontend/src/components/Header.tsx`
- Impact: Screen reader users must tab through the entire header to reach content.
- Fix approach: Add `<a href="#main" className="sr-only focus:not-sr-only">Skip to content</a>` and `role="main"` / `aria-label` attributes. Priority: Low.

---

*Concerns audit: 2026-03-23*