DiunDashboard/.planning/codebase/CONCERNS.md

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