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.
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.
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);`
Blocking a user prevents them from interacting with repositories, such as opening or commenting on pull requests or issues. Learn more about blocking a user.
Collection of LOW severity items and minor best-practice divergences found during the pre-1.0 code review.
1.
loggingResponseWriterdoes not implementhttp.Flusherorhttp.HijackerFile:
internal/middleware/middleware.goThe wrapped response writer loses access to
Flush()andHijack()interfaces. This could break SSE or WebSocket upgrades if ever needed. Consider embeddinghttp.Flusherdelegation.2.
relativeTimeAlpine component leakssetIntervaltimersFile:
static/js/app.js—relativeTimecomponentThe
setIntervalininit()is never cleared when the component is destroyed. On pages with many relative time elements, this accumulates timers. Add adestroy()method to clear the interval.3. Dashboard N+1 query pattern
File:
internal/handlers/dashboard.goFor 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.
HandleAppUpdatebypasses the app service layerFile:
internal/handlers/app.go(HandleAppUpdate)The handler directly mutates the app model and calls
Save(), bypassingappService.UpdateApp(). This is inconsistent withHandleAppCreatewhich uses the service. Should usesvc.UpdateApp()for consistency.5. Missing
Content-Security-PolicyheaderFile:
internal/server/routes.go/ middlewareNo 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-varsvsenvURL inconsistencyFile:
internal/server/routes.goAdd endpoint is
POST /apps/{id}/env-varsbut delete isPOST /apps/{id}/env/{varID}/delete. The URL prefix is inconsistent (env-varsvsenv). Pick one.7. SQLite connection pool not configured
File:
internal/database/database.goNo
SetMaxOpenConns,SetMaxIdleConns, orSetConnMaxLifetimeis set on the*sql.DB. SQLite works best withSetMaxOpenConns(1)for write serialization. The default Go pool may open multiple connections, causingdatabase is lockederrors under load.8. Deployment logs grow unbounded in database
File:
internal/models/deployment.go(AppendLog)Each
AppendLogcall 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.
HandleDeploymentLogDownloadsets Content-Disposition then callshttp.ServeFileFile:
internal/handlers/app.go(HandleDeploymentLogDownload)http.ServeFilesets its own headers and may override the Content-Disposition. It also handles Range requests and If-Modified-Since, which is good, but the manualos.Statcheck before it is redundant.10. CSS file served from embedded FS but not present in repo
File:
templates/base.htmlreferences/s/css/tailwind.cssThe
static/embed includescssandjsdirectories, but no CSS files are visible in the repo. Iftailwind.cssis generated at build time, the build process should be documented. If missing, the UI will have no styling.11.
docker-compose.ymlmissingHOST_DATA_DIRenvironment variableFile:
docker-compose.ymlWhen running in Docker with a named volume, the
HOST_DATA_DIRenv var isn't set. This meanscontainerToHostPath()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
/healthbut.well-known/healthcheck.jsonin testFile:
internal/server/routes.govsinternal/handlers/handlers_test.goThe route is
GET /healthbut 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.
AppendLogdoes a full Save which updates all fieldsFile:
internal/models/deployment.goAppendLogcallsSave()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.goWebhook has a 1MB body limit but regular form POSTs have no explicit limit. The default Go limit is 10MB (
http.MaxBytesReaderinParseForm), which is fine, but an explicithttp.MaxBytesReaderwould be more defensive.15. Missing index on
webhook_secret_hashFile:
internal/database/migrations/005_add_webhook_secret_hash.sqlThe migration adds the column but no index.
FindAppByWebhookSecretqueries 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);