arthur/xtablo-source
1
0
Fork 0
Code Issues Pull requests 1 Projects Releases Packages Wiki Activity Actions 5

docs(05): add code review fix report

Browse source
This commit is contained in:
Arthur Belleville 2026-05-15 12:51:29 +02:00
parent 49e84c8176
commit 3a7726e6ed
No known key found for this signature in database
1 changed files with 59 additions and 0 deletions
Show stats Download patch file Download diff file Expand all files Collapse all files

59
.planning/phases/05-files/05-REVIEW-FIX.md Normal file
View file

@ -0,0 +1,59 @@
---
phase: 05-files
fixed_at: 2026-05-15T00:00:00Z
review_path: .planning/phases/05-files/05-REVIEW.md
iteration: 1
findings_in_scope: 5
fixed: 5
skipped: 0
status: all_fixed
---
# Phase 05: Code Review Fix Report
**Fixed at:** 2026-05-15
**Source review:** .planning/phases/05-files/05-REVIEW.md
**Iteration:** 1
**Summary:**
- Findings in scope: 5 (Critical + Warning; IN-01 and IN-02 excluded per fix scope)
- Fixed: 5
- Skipped: 0
## Fixed Issues
### CR-01: S3 object orphaned silently when DB insert fails after successful upload
**Files modified:** `backend/internal/web/handlers_files.go`
**Commit:** 690ea2d
**Applied fix:** After `deps.Files.Upload` succeeds, if `deps.Queries.InsertTabloFile` fails, a background context with 10s timeout is created and `deps.Files.Delete` is called to clean up the S3 object. Both the DB error and any S3 cleanup error are logged. The 500 response is returned after the cleanup attempt.
### WR-01: Global 15s WriteTimeout will abort large file uploads in flight
**Files modified:** `backend/cmd/web/main.go`
**Commit:** 49e84c8
**Applied fix:** Raised both `ReadTimeout` and `WriteTimeout` from 15s to 120s. A 25MB upload at ~256 KB/s takes ~100s; 120s provides comfortable headroom. Added inline comment explaining the rationale (MaxBytesReader bounds size not time; WriteTimeout covers the full request lifecycle).
### WR-02: Empty `header.Filename` is accepted and stored to DB
**Files modified:** `backend/internal/web/handlers_files.go`
**Commit:** 690ea2d
**Applied fix:** Added `filename := strings.TrimSpace(header.Filename)` immediately after `r.FormFile("file")`. If `filename` is empty, returns 400 before any S3 operation. `filename` (trimmed) is used in `InsertTabloFileParams` instead of `header.Filename`. `strings` import added.
### WR-03: Dead variable `errMsg` left in production handler
**Files modified:** `backend/internal/web/handlers_files.go`
**Commit:** 690ea2d
**Applied fix:** Removed the dead `errMsg := "File too large (max %d MB)."` declaration and the `_ = errMsg` silencer. The `UploadErrorFragment` call now uses a locally constructed `errMsg` string built with `strconv.Itoa` (see WR-04 below).
### WR-04: Custom `itoa` reimplements `strconv.Itoa` without benefit
**Files modified:** `backend/internal/web/handlers_files.go`
**Commit:** 690ea2d
**Applied fix:** Deleted the `itoa` and `formatMBError` helper functions entirely. Their single call site now builds the error string inline: `errMsg := "File too large (max " + strconv.Itoa(deps.MaxUploadMB) + " MB)."`. `strconv` import added.
---
_Fixed: 2026-05-15_
_Fixer: Claude (gsd-code-fixer)_
_Iteration: 1_