fix: address code review issues in album and generation skeleton packages

- Add ErrAnchorRequired sentinel to pkg/album — replaces fragile string equality check
  used for 422 detection; callers now use errors.Is()
- Extract parseShotIndex() helper in album handler — replaces fmt.Sscanf which silently
  accepted partial parses like "12abc"; strconv.Atoi requires the full string to be numeric
- Restructure caption saves in album/handler — captions now written outside the
  len(img.Data) > 0 gate, so URL-only providers (no bytes returned) still get captions
- Add storage.FetchURL() shared utility — removes fetchBytes/downloadURL duplication
  across album and generation packages; callers control timeout via their http.Client
- Add video captions to VideoHandler — same caption sidecar pattern applied to videos
- Add persona generation event types to realtime package — persona_spec_*, persona_image_*,
  persona_video_* events added to EventType union and usePersonaGeneration hook exported

Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
This commit is contained in:
jordan 2026-02-23 03:01:37 -07:00
parent 062a828a00
commit 9be5c7d81b
8 changed files with 110 additions and 79 deletions

View File

@ -1,8 +1,9 @@
package handlers package handlers
import ( import (
"fmt" "errors"
"net/http" "net/http"
"strconv"
"github.com/go-chi/chi/v5" "github.com/go-chi/chi/v5"
@ -207,7 +208,7 @@ func (h *Album) GenerateAllShots(w http.ResponseWriter, r *http.Request) error {
jobIDs, err := h.albums.GenerateAllShots(r.Context(), id, user.ID) jobIDs, err := h.albums.GenerateAllShots(r.Context(), id, user.ID)
if err != nil { if err != nil {
if err.Error() == "anchor must be generated before shots" { if errors.Is(err, album.ErrAnchorRequired) {
return httperror.UnprocessableEntity("anchor must be generated before shots") return httperror.UnprocessableEntity("anchor must be generated before shots")
} }
return httperror.NotFound("album not found") return httperror.NotFound("album not found")
@ -234,16 +235,14 @@ func (h *Album) GenerateShot(w http.ResponseWriter, r *http.Request) error {
return httperror.BadRequest("album ID is required") return httperror.BadRequest("album ID is required")
} }
shotIndex := 0 shotIndex, err := parseShotIndex(chi.URLParam(r, "index"))
if idx := chi.URLParam(r, "index"); idx != "" { if err != nil {
if _, err := fmt.Sscanf(idx, "%d", &shotIndex); err != nil { return httperror.BadRequest("shot index must be a non-negative integer")
return httperror.BadRequest("invalid shot index")
}
} }
jobID, err := h.albums.GenerateShot(r.Context(), id, user.ID, shotIndex) jobID, err := h.albums.GenerateShot(r.Context(), id, user.ID, shotIndex)
if err != nil { if err != nil {
if err.Error() == "anchor must be generated before shots" { if errors.Is(err, album.ErrAnchorRequired) {
return httperror.UnprocessableEntity("anchor must be generated before shots") return httperror.UnprocessableEntity("anchor must be generated before shots")
} }
return httperror.NotFound("album or shot not found") return httperror.NotFound("album or shot not found")
@ -265,11 +264,9 @@ func (h *Album) ResetShot(w http.ResponseWriter, r *http.Request) error {
return httperror.BadRequest("album ID is required") return httperror.BadRequest("album ID is required")
} }
shotIndex := 0 shotIndex, err := parseShotIndex(chi.URLParam(r, "index"))
if idx := chi.URLParam(r, "index"); idx != "" { if err != nil {
if _, err := fmt.Sscanf(idx, "%d", &shotIndex); err != nil { return httperror.BadRequest("shot index must be a non-negative integer")
return httperror.BadRequest("invalid shot index")
}
} }
if err := h.albums.ResetShot(r.Context(), id, user.ID, shotIndex); err != nil { if err := h.albums.ResetShot(r.Context(), id, user.ID, shotIndex); err != nil {
@ -279,3 +276,16 @@ func (h *Album) ResetShot(w http.ResponseWriter, r *http.Request) error {
httpresponse.NoContent(w) httpresponse.NoContent(w)
return nil return nil
} }
// parseShotIndex parses and validates the shot index URL parameter.
// Returns an error if the value is missing, non-numeric, or negative.
func parseShotIndex(idx string) (int, error) {
if idx == "" {
return 0, errors.New("missing shot index")
}
n, err := strconv.Atoi(idx)
if err != nil || n < 0 {
return 0, errors.New("shot index must be a non-negative integer")
}
return n, nil
}

View File

@ -122,7 +122,7 @@ func (s *AlbumService) GenerateAllShots(ctx context.Context, id album.AlbumID, u
return nil, fmt.Errorf("album not found: %w", err) return nil, fmt.Errorf("album not found: %w", err)
} }
if a.AnchorURL == "" { if a.AnchorURL == "" {
return nil, fmt.Errorf("anchor must be generated before shots") return nil, album.ErrAnchorRequired
} }
var jobIDs []string var jobIDs []string

View File

@ -4,3 +4,4 @@ export { useMediaGeneration, type GenerationStatus, type UseMediaGenerationConfi
export { useChat, type UseChatConfig, type UseChatResult, type ChatMessage } from './useChat'; export { useChat, type UseChatConfig, type UseChatResult, type ChatMessage } from './useChat';
export { useMediaUpload, type UploadProgress, type UploadResult, type UseMediaUploadConfig, type UseMediaUploadResult } from './useMediaUpload'; export { useMediaUpload, type UploadProgress, type UploadResult, type UseMediaUploadConfig, type UseMediaUploadResult } from './useMediaUpload';
export { useAlbumGeneration, type Album, type Shot, type ShotStatus, type UseAlbumGenerationConfig, type UseAlbumGenerationResult } from './useAlbumGeneration'; export { useAlbumGeneration, type Album, type Shot, type ShotStatus, type UseAlbumGenerationConfig, type UseAlbumGenerationResult } from './useAlbumGeneration';
export { usePersonaGeneration, type PersonaGenerationState, type UsePersonaGenerationConfig, type UsePersonaGenerationResult } from './usePersonaGeneration';

View File

@ -22,7 +22,17 @@ export type EventType =
| 'album_anchor_failed' | 'album_anchor_failed'
| 'album_shot_generating' | 'album_shot_generating'
| 'album_shot_complete' | 'album_shot_complete'
| 'album_shot_failed'; | 'album_shot_failed'
// Persona generation events (pkg/personagen)
| 'persona_spec_started'
| 'persona_spec_complete'
| 'persona_image_started'
| 'persona_image_progress'
| 'persona_image_complete'
| 'persona_video_started'
| 'persona_video_complete'
| 'persona_video_failed'
| 'persona_failed';
/** /**
* Chat message data payload. * Chat message data payload.

View File

@ -3,7 +3,6 @@ package album
import ( import (
"context" "context"
"fmt" "fmt"
"io"
"log/slog" "log/slog"
"net/http" "net/http"
"time" "time"
@ -76,13 +75,19 @@ func AnchorHandler(mg *mediagen.Manager, store storage.Store, pub realtime.Event
// Persist anchor to storage. // Persist anchor to storage.
anchorURL := img.URL anchorURL := img.URL
if store != nil && len(img.Data) > 0 { if store != nil {
storagePath := fmt.Sprintf("albums/%s/%s/anchor.png", userID, albumID) if len(img.Data) > 0 {
url, uploadErr := store.Upload(ctx, storagePath, img.Data, "image/png") storagePath := fmt.Sprintf("albums/%s/%s/anchor.png", userID, albumID)
if uploadErr != nil { url, uploadErr := store.Upload(ctx, storagePath, img.Data, "image/png")
logger.Warn("failed to persist anchor to storage, using inline URL", "error", uploadErr, "job_id", job.ID) if uploadErr != nil {
} else { logger.Warn("failed to persist anchor to storage, using inline URL", "error", uploadErr, "job_id", job.ID)
anchorURL = url } else {
anchorURL = url
}
}
// Save caption alongside the image regardless of whether bytes were uploaded.
// This ensures we always record what generated the image (URL-only providers included).
if anchorURL != "" {
captionPath := fmt.Sprintf("albums/%s/%s/anchor.caption", userID, albumID) captionPath := fmt.Sprintf("albums/%s/%s/anchor.caption", userID, albumID)
if _, captionErr := store.Upload(ctx, captionPath, []byte(subjectDesc), "text/plain"); captionErr != nil { if _, captionErr := store.Upload(ctx, captionPath, []byte(subjectDesc), "text/plain"); captionErr != nil {
logger.Warn("failed to persist anchor caption", "error", captionErr, "job_id", job.ID) logger.Warn("failed to persist anchor caption", "error", captionErr, "job_id", job.ID)
@ -150,10 +155,11 @@ func ShotHandler(mg *mediagen.Manager, store storage.Store, pub realtime.EventPu
}) })
// Fetch anchor image bytes from storage. // Fetch anchor image bytes from storage.
const anchorMaxBytes = 20 << 20 // 20 MB — anchor images should be small PNGs
var anchorBytes []byte var anchorBytes []byte
var anchorMime string var anchorMime string
if anchorURL != "" { if anchorURL != "" {
data, err := fetchBytes(ctx, anchorURL) data, err := storage.FetchURL(ctx, httpClient, anchorURL, anchorMaxBytes)
if err != nil { if err != nil {
logger.Warn("failed to fetch anchor image, proceeding without reference", logger.Warn("failed to fetch anchor image, proceeding without reference",
"error", err, "job_id", job.ID, "anchor_url", anchorURL) "error", err, "job_id", job.ID, "anchor_url", anchorURL)
@ -209,14 +215,19 @@ func ShotHandler(mg *mediagen.Manager, store storage.Store, pub realtime.EventPu
// Persist shot image to storage. // Persist shot image to storage.
imageURL := img.URL imageURL := img.URL
if store != nil && len(img.Data) > 0 { if store != nil {
storagePath := fmt.Sprintf("albums/%s/%s/shots/%d.png", userID, albumID, shotIndex) if len(img.Data) > 0 {
url, uploadErr := store.Upload(ctx, storagePath, img.Data, "image/png") storagePath := fmt.Sprintf("albums/%s/%s/shots/%d.png", userID, albumID, shotIndex)
if uploadErr != nil { url, uploadErr := store.Upload(ctx, storagePath, img.Data, "image/png")
logger.Warn("failed to persist shot to storage, using inline URL", if uploadErr != nil {
"error", uploadErr, "job_id", job.ID) logger.Warn("failed to persist shot to storage, using inline URL",
} else { "error", uploadErr, "job_id", job.ID)
imageURL = url } else {
imageURL = url
}
}
// Save caption alongside the image regardless of whether bytes were uploaded.
if imageURL != "" {
captionPath := fmt.Sprintf("albums/%s/%s/shots/%d.caption", userID, albumID, shotIndex) captionPath := fmt.Sprintf("albums/%s/%s/shots/%d.caption", userID, albumID, shotIndex)
if _, captionErr := store.Upload(ctx, captionPath, []byte(prompt), "text/plain"); captionErr != nil { if _, captionErr := store.Upload(ctx, captionPath, []byte(prompt), "text/plain"); captionErr != nil {
logger.Warn("failed to persist shot caption", "error", captionErr, "job_id", job.ID) logger.Warn("failed to persist shot caption", "error", captionErr, "job_id", job.ID)
@ -249,25 +260,3 @@ func ShotHandler(mg *mediagen.Manager, store storage.Store, pub realtime.EventPu
} }
} }
// fetchBytes downloads raw bytes from a URL.
// Used to load anchor images at shot-generation time.
func fetchBytes(ctx context.Context, url string) ([]byte, error) {
req, err := http.NewRequestWithContext(ctx, http.MethodGet, url, nil)
if err != nil {
return nil, fmt.Errorf("create request: %w", err)
}
resp, err := httpClient.Do(req)
if err != nil {
return nil, fmt.Errorf("fetch: %w", err)
}
defer resp.Body.Close()
if resp.StatusCode != http.StatusOK {
return nil, fmt.Errorf("fetch: HTTP %d", resp.StatusCode)
}
const maxSize = 20 << 20 // 20 MB limit for anchor images
data, err := io.ReadAll(io.LimitReader(resp.Body, maxSize))
if err != nil {
return nil, fmt.Errorf("read: %w", err)
}
return data, nil
}

View File

@ -13,9 +13,14 @@ package album
import ( import (
"context" "context"
"errors"
"time" "time"
) )
// ErrAnchorRequired is returned when shot generation is attempted before the anchor image exists.
// Handlers should map this to HTTP 422 Unprocessable Entity.
var ErrAnchorRequired = errors.New("anchor must be generated before shots")
// AlbumID is the unique identifier for an album. // AlbumID is the unique identifier for an album.
type AlbumID string type AlbumID string

View File

@ -6,7 +6,6 @@ import (
"context" "context"
"encoding/base64" "encoding/base64"
"fmt" "fmt"
"io"
"log/slog" "log/slog"
"net/http" "net/http"
"time" "time"
@ -179,6 +178,7 @@ func VideoHandler(mg *mediagen.Manager, store storage.Store, pub realtime.EventP
// Build videos array matching frontend VideoResult shape: // Build videos array matching frontend VideoResult shape:
// { videos: [{ data, isUrl, mimeType }], provider, latencyMs } // { videos: [{ data, isUrl, mimeType }], provider, latencyMs }
const videoMaxBytes = 500 << 20 // 500 MB — videos can be large
videos := make([]map[string]any, 0, len(resp.Videos)) videos := make([]map[string]any, 0, len(resp.Videos))
for i, vid := range resp.Videos { for i, vid := range resp.Videos {
videoURL := vid.URL videoURL := vid.URL
@ -193,7 +193,7 @@ func VideoHandler(mg *mediagen.Manager, store storage.Store, pub realtime.EventP
if len(vid.Data) > 0 { if len(vid.Data) > 0 {
videoData = vid.Data videoData = vid.Data
} else if vid.URL != "" { } else if vid.URL != "" {
downloaded, downloadErr := downloadURL(ctx, vid.URL) downloaded, downloadErr := storage.FetchURL(ctx, httpClient, vid.URL, videoMaxBytes)
if downloadErr != nil { if downloadErr != nil {
logger.Warn("failed to download video from provider", "error", downloadErr, "job_id", job.ID) logger.Warn("failed to download video from provider", "error", downloadErr, "job_id", job.ID)
} else { } else {
@ -209,6 +209,14 @@ func VideoHandler(mg *mediagen.Manager, store storage.Store, pub realtime.EventP
videoURL = persistedURL videoURL = persistedURL
} }
} }
// Save caption alongside the video regardless of where it's stored.
if videoURL != "" && prompt != "" {
captionPath := fmt.Sprintf("media/%s/videos/%s_%d.caption", userID, job.ID, i)
if _, captionErr := store.Upload(ctx, captionPath, []byte(prompt), "text/plain"); captionErr != nil {
logger.Warn("failed to persist video caption", "error", captionErr, "job_id", job.ID)
}
}
} }
videos = append(videos, map[string]any{ videos = append(videos, map[string]any{
@ -339,26 +347,3 @@ func ChatResponseHandler(tg *textgen.Manager, pub realtime.EventPublisher, logge
} }
} }
// downloadURL fetches content from a URL and returns the bytes.
// Used to download provider-hosted videos before persisting to storage.
func downloadURL(ctx context.Context, url string) ([]byte, error) {
req, err := http.NewRequestWithContext(ctx, http.MethodGet, url, nil)
if err != nil {
return nil, fmt.Errorf("create request: %w", err)
}
resp, err := httpClient.Do(req)
if err != nil {
return nil, fmt.Errorf("download: %w", err)
}
defer resp.Body.Close()
if resp.StatusCode != http.StatusOK {
return nil, fmt.Errorf("download: status %d", resp.StatusCode)
}
// Limit body to 500MB to prevent OOM from unexpected large responses.
const maxBodySize = 500 << 20
data, err := io.ReadAll(io.LimitReader(resp.Body, maxBodySize))
if err != nil {
return nil, fmt.Errorf("read body: %w", err)
}
return data, nil
}

View File

@ -0,0 +1,31 @@
package storage
import (
"context"
"fmt"
"io"
"net/http"
)
// FetchURL downloads content from a URL using the provided HTTP client.
// maxBytes caps the download to prevent OOM from unexpectedly large responses.
// Callers control the timeout via the http.Client they pass in.
func FetchURL(ctx context.Context, client *http.Client, url string, maxBytes int64) ([]byte, error) {
req, err := http.NewRequestWithContext(ctx, http.MethodGet, url, nil)
if err != nil {
return nil, fmt.Errorf("create request: %w", err)
}
resp, err := client.Do(req)
if err != nil {
return nil, fmt.Errorf("fetch: %w", err)
}
defer resp.Body.Close()
if resp.StatusCode != http.StatusOK {
return nil, fmt.Errorf("fetch: HTTP %d", resp.StatusCode)
}
data, err := io.ReadAll(io.LimitReader(resp.Body, maxBytes))
if err != nil {
return nil, fmt.Errorf("read: %w", err)
}
return data, nil
}