lerko
e9ecc4c1f7
CI / test (pull_request) Successful in 2m13s
Fix goroutine-unsafe ULID entropy by wrapping in LockedMonotonicReader. Move PRAGMA foreign_keys outside transaction in v3 migration where SQLite was silently ignoring it. Escape LIKE wildcards in link resolution to prevent false matches. Add non-localhost binding warning, log writeJSON encoder errors, add ?permanent=true for explicit hard delete, preserve title/description during absorb, use millisecond backup timestamps, add path.Clean to spaHandler. Frontend gains checkedJSON() for resp.ok validation, consistent stopPropagation, and shared renderCardSections() to eliminate duplicate rendering.
71 lines
2.7 KiB
Markdown
71 lines
2.7 KiB
Markdown
# Code Review Fixes
|
|
|
|
## Phase 1: Critical — Concurrency & Data Safety
|
|
|
|
### 1. ULID entropy not goroutine-safe
|
|
- [x] Wrap MonotonicEntropy in LockedMonotonicReader (`internal/ulid/ulid.go`)
|
|
- [x] Add concurrent uniqueness test (100 goroutines)
|
|
- [x] Verify `go test -race ./internal/ulid/...`
|
|
|
|
### 2. Migration PRAGMA bug
|
|
- [x] Move `PRAGMA foreign_keys = OFF` before transaction in v3 migration (`internal/db/db.go`)
|
|
- [x] Re-enable after commit
|
|
- [x] Remove dead `currentSchema` constant while here
|
|
|
|
### 3. LIKE wildcard injection in link resolution
|
|
- [x] Escape `%` and `_` in link text before LIKE query (`internal/db/links.go`)
|
|
- [x] Add `ESCAPE '\'` clause
|
|
- [ ] Test with `[[%]]` and `[[_]]` link text
|
|
|
|
## Phase 2: High — Security & API Hygiene
|
|
|
|
### 4. No auth warning for non-localhost binding
|
|
- [x] Print loud warning when `--host != 127.0.0.1` and no auth configured (`cmd/serve.go`)
|
|
- [ ] Consider `--no-auth` flag requirement for non-localhost
|
|
|
|
### 5. writeJSON ignores encoder errors
|
|
- [x] Log error from `json.Encoder.Encode()` in `writeJSON` (`internal/api/helpers.go`)
|
|
|
|
### 6. DELETE endpoint semantics
|
|
- [x] Add `?permanent=true` query param for hard delete (`internal/api/entities.go`)
|
|
- [x] Add `HardDelete` store method (`internal/db/entities.go`)
|
|
- [ ] Update frontend and CLI to match
|
|
- [x] Keep backward compat: double-delete still works without param
|
|
|
|
## Phase 3: Medium — Frontend Robustness
|
|
|
|
### 7. No resp.ok checks on fetch calls
|
|
- [x] Add `checkedJSON()` wrapper with error extraction (`web/app.js`)
|
|
- [x] All API methods use `checkedJSON(resp)` instead of `resp.json()`
|
|
- [ ] Surface API errors to user via notification/toast
|
|
|
|
### 8. Inconsistent event.stopPropagation
|
|
- [x] Add stopPropagation to all renderStreamPeek action buttons
|
|
- [x] Add stopPropagation to all renderCardPeek action buttons and inline section buttons
|
|
|
|
### 9. Duplicate section rendering
|
|
- [x] Extract `renderCardSections()` shared function (`web/app.js`)
|
|
- [x] Refactor `renderInlineDetail` to use shared function
|
|
- [x] Refactor `renderCardPeek` to use shared function
|
|
|
|
## Phase 4: Medium — Data Integrity
|
|
|
|
### 10. Absorb discards source title/description
|
|
- [x] If target has no title, inherit from source (`internal/db/entities.go`)
|
|
- [x] If target has no description, inherit from source
|
|
- [ ] Test title/description preservation
|
|
|
|
## Phase 5: Low — Housekeeping & UX
|
|
|
|
### 11. Backup path collision
|
|
- [x] Use millisecond-precision timestamps (`cmd/backup.go`)
|
|
|
|
### 12. spaHandler path safety
|
|
- [x] Add explicit `path.Clean` in spaHandler (`internal/api/router.go`)
|
|
|
|
### 13. Focus-peek escape hatch
|
|
- [ ] Add visible close/back button in focus-peek mode (`web/app.js`)
|
|
|
|
### 14. Hard delete confirmation
|
|
- [ ] Add confirmation step or undo toast before permanent delete (`web/app.js`)
|