Code cleanup: minor best practice improvements for 1.0 #45

New Issue

clawbot · 2026-02-16T06:57:15+01:00

clawbot commented

2026-02-16 06:57:15 +01:00

Collection of LOW severity items and minor best-practice divergences found during the pre-1.0 code review.

1. `loggingResponseWriter` does not implement `http.Flusher` or `http.Hijacker`

File: internal/middleware/middleware.go
The wrapped response writer loses access to Flush() and Hijack() interfaces. This could break SSE or WebSocket upgrades if ever needed. Consider embedding http.Flusher delegation.

2. `relativeTime` Alpine component leaks `setInterval` timers

File: static/js/app.js — relativeTime component
The setInterval in init() is never cleared when the component is destroyed. On pages with many relative time elements, this accumulates timers. Add a destroy() method to clear the interval.

3. Dashboard N+1 query pattern

File: internal/handlers/dashboard.go
For each app, two additional queries are made (count deployments + latest deployment). With many apps this becomes slow. Consider a single SQL query with LEFT JOIN or subquery.

4. `HandleAppUpdate` bypasses the app service layer

File: internal/handlers/app.go (HandleAppUpdate)
The handler directly mutates the app model and calls Save(), bypassing appService.UpdateApp(). This is inconsistent with HandleAppCreate which uses the service. Should use svc.UpdateApp() for consistency.

5. Missing `Content-Security-Policy` header

File: internal/server/routes.go / middleware
No CSP header is set. For a security-conscious admin panel, a restrictive CSP would prevent XSS even if injection is found. Suggested: default-src 'self'; script-src 'self'

6. `env-vars` vs `env` URL inconsistency

File: internal/server/routes.go
Add endpoint is POST /apps/{id}/env-vars but delete is POST /apps/{id}/env/{varID}/delete. The URL prefix is inconsistent (env-vars vs env). Pick one.

7. SQLite connection pool not configured

File: internal/database/database.go
No SetMaxOpenConns, SetMaxIdleConns, or SetConnMaxLifetime is set on the *sql.DB. SQLite works best with SetMaxOpenConns(1) for write serialization. The default Go pool may open multiple connections, causing database is locked errors under load.

8. Deployment logs grow unbounded in database

File: internal/models/deployment.go (AppendLog)
Each AppendLog call reads the full log, appends, and writes back. For long builds, this is O(n²) in log size. Consider appending directly with SQL: UPDATE deployments SET logs = logs || ? WHERE id = ?

9. `HandleDeploymentLogDownload` sets Content-Disposition then calls `http.ServeFile`

File: internal/handlers/app.go (HandleDeploymentLogDownload)
http.ServeFile sets its own headers and may override the Content-Disposition. It also handles Range requests and If-Modified-Since, which is good, but the manual os.Stat check before it is redundant.

10. CSS file served from embedded FS but not present in repo

File: templates/base.html references /s/css/tailwind.css
The static/ embed includes css and js directories, but no CSS files are visible in the repo. If tailwind.css is generated at build time, the build process should be documented. If missing, the UI will have no styling.

11. `docker-compose.yml` missing `HOST_DATA_DIR` environment variable

File: docker-compose.yml
When running in Docker with a named volume, the HOST_DATA_DIR env var isn't set. This means containerToHostPath() will return container paths for Docker bind mounts, which won't work correctly for git clone operations that need host paths.

12. Health check endpoint at `/health` but `.well-known/healthcheck.json` in test

File: internal/server/routes.go vs internal/handlers/handlers_test.go
The route is GET /health but the test uses /.well-known/healthcheck.json. The test still passes because it calls the handler directly, but the URL in the test is misleading.

13. `AppendLog` does a full Save which updates all fields

File: internal/models/deployment.go
AppendLog calls Save() which runs a full UPDATE of all deployment fields. During concurrent operations, this could overwrite fields updated by other goroutines. Use a targeted UPDATE instead.

14. No request body size limit on form submissions

File: internal/server/routes.go
Webhook has a 1MB body limit but regular form POSTs have no explicit limit. The default Go limit is 10MB (http.MaxBytesReader in ParseForm), which is fine, but an explicit http.MaxBytesReader would be more defensive.

15. Missing index on `webhook_secret_hash`

File: internal/database/migrations/005_add_webhook_secret_hash.sql
The migration adds the column but no index. FindAppByWebhookSecret queries by this column. Without an index, it's a full table scan. Add: CREATE INDEX idx_apps_webhook_secret_hash ON apps(webhook_secret_hash);

Collection of LOW severity items and minor best-practice divergences found during the pre-1.0 code review. --- ### 1. `loggingResponseWriter` does not implement `http.Flusher` or `http.Hijacker` **File:** `internal/middleware/middleware.go` The wrapped response writer loses access to `Flush()` and `Hijack()` interfaces. This could break SSE or WebSocket upgrades if ever needed. Consider embedding `http.Flusher` delegation. ### 2. `relativeTime` Alpine component leaks `setInterval` timers **File:** `static/js/app.js` — `relativeTime` component The `setInterval` in `init()` is never cleared when the component is destroyed. On pages with many relative time elements, this accumulates timers. Add a `destroy()` method to clear the interval. ### 3. Dashboard N+1 query pattern **File:** `internal/handlers/dashboard.go` For each app, two additional queries are made (count deployments + latest deployment). With many apps this becomes slow. Consider a single SQL query with LEFT JOIN or subquery. ### 4. `HandleAppUpdate` bypasses the app service layer **File:** `internal/handlers/app.go` (HandleAppUpdate) The handler directly mutates the app model and calls `Save()`, bypassing `appService.UpdateApp()`. This is inconsistent with `HandleAppCreate` which uses the service. Should use `svc.UpdateApp()` for consistency. ### 5. Missing `Content-Security-Policy` header **File:** `internal/server/routes.go` / middleware No CSP header is set. For a security-conscious admin panel, a restrictive CSP would prevent XSS even if injection is found. Suggested: `default-src 'self'; script-src 'self'` ### 6. `env-vars` vs `env` URL inconsistency **File:** `internal/server/routes.go` Add endpoint is `POST /apps/{id}/env-vars` but delete is `POST /apps/{id}/env/{varID}/delete`. The URL prefix is inconsistent (`env-vars` vs `env`). Pick one. ### 7. SQLite connection pool not configured **File:** `internal/database/database.go` No `SetMaxOpenConns`, `SetMaxIdleConns`, or `SetConnMaxLifetime` is set on the `*sql.DB`. SQLite works best with `SetMaxOpenConns(1)` for write serialization. The default Go pool may open multiple connections, causing `database is locked` errors under load. ### 8. Deployment logs grow unbounded in database **File:** `internal/models/deployment.go` (AppendLog) Each `AppendLog` call reads the full log, appends, and writes back. For long builds, this is O(n²) in log size. Consider appending directly with SQL: `UPDATE deployments SET logs = logs || ? WHERE id = ?` ### 9. `HandleDeploymentLogDownload` sets Content-Disposition then calls `http.ServeFile` **File:** `internal/handlers/app.go` (HandleDeploymentLogDownload) `http.ServeFile` sets its own headers and may override the Content-Disposition. It also handles Range requests and If-Modified-Since, which is good, but the manual `os.Stat` check before it is redundant. ### 10. CSS file served from embedded FS but not present in repo **File:** `templates/base.html` references `/s/css/tailwind.css` The `static/` embed includes `css` and `js` directories, but no CSS files are visible in the repo. If `tailwind.css` is generated at build time, the build process should be documented. If missing, the UI will have no styling. ### 11. `docker-compose.yml` missing `HOST_DATA_DIR` environment variable **File:** `docker-compose.yml` When running in Docker with a named volume, the `HOST_DATA_DIR` env var isn't set. This means `containerToHostPath()` will return container paths for Docker bind mounts, which won't work correctly for git clone operations that need host paths. ### 12. Health check endpoint at `/health` but `.well-known/healthcheck.json` in test **File:** `internal/server/routes.go` vs `internal/handlers/handlers_test.go` The route is `GET /health` but the test uses `/.well-known/healthcheck.json`. The test still passes because it calls the handler directly, but the URL in the test is misleading. ### 13. `AppendLog` does a full Save which updates all fields **File:** `internal/models/deployment.go` `AppendLog` calls `Save()` which runs a full UPDATE of all deployment fields. During concurrent operations, this could overwrite fields updated by other goroutines. Use a targeted UPDATE instead. ### 14. No request body size limit on form submissions **File:** `internal/server/routes.go` Webhook has a 1MB body limit but regular form POSTs have no explicit limit. The default Go limit is 10MB (`http.MaxBytesReader` in `ParseForm`), which is fine, but an explicit `http.MaxBytesReader` would be more defensive. ### 15. Missing index on `webhook_secret_hash` **File:** `internal/database/migrations/005_add_webhook_secret_hash.sql` The migration adds the column but no index. `FindAppByWebhookSecret` queries by this column. Without an index, it's a full table scan. Add: `CREATE INDEX idx_apps_webhook_secret_hash ON apps(webhook_secret_hash);`

Sign in to join this conversation.