From 9be5c7d81b89197b84f0e1f0372f3b91ec8a86a1 Mon Sep 17 00:00:00 2001 From: jordan Date: Mon, 23 Feb 2026 03:01:37 -0700 Subject: [PATCH] fix: address code review issues in album and generation skeleton packages MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit - 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 --- .../internal/api/handlers/album.go.tmpl | 36 ++++++---- .../service/internal/service/album.go.tmpl | 2 +- .../skeleton/packages/realtime/src/index.ts | 1 + .../skeleton/packages/realtime/src/types.ts | 12 +++- .../skeleton/pkg/album/handler.go.tmpl | 67 ++++++++----------- .../skeleton/pkg/album/types.go.tmpl | 5 ++ .../skeleton/pkg/generation/handlers.go.tmpl | 35 +++------- .../skeleton/pkg/storage/fetch.go.tmpl | 31 +++++++++ 8 files changed, 110 insertions(+), 79 deletions(-) create mode 100644 internal/adapter/templates/templates/skeleton/pkg/storage/fetch.go.tmpl diff --git a/internal/adapter/templates/templates/components/service/internal/api/handlers/album.go.tmpl b/internal/adapter/templates/templates/components/service/internal/api/handlers/album.go.tmpl index 4f8be6e..863f8ca 100644 --- a/internal/adapter/templates/templates/components/service/internal/api/handlers/album.go.tmpl +++ b/internal/adapter/templates/templates/components/service/internal/api/handlers/album.go.tmpl @@ -1,8 +1,9 @@ package handlers import ( - "fmt" + "errors" "net/http" + "strconv" "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) 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.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") } - shotIndex := 0 - if idx := chi.URLParam(r, "index"); idx != "" { - if _, err := fmt.Sscanf(idx, "%d", &shotIndex); err != nil { - return httperror.BadRequest("invalid shot index") - } + shotIndex, err := parseShotIndex(chi.URLParam(r, "index")) + if err != nil { + return httperror.BadRequest("shot index must be a non-negative integer") } jobID, err := h.albums.GenerateShot(r.Context(), id, user.ID, shotIndex) 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.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") } - shotIndex := 0 - if idx := chi.URLParam(r, "index"); idx != "" { - if _, err := fmt.Sscanf(idx, "%d", &shotIndex); err != nil { - return httperror.BadRequest("invalid shot index") - } + shotIndex, err := parseShotIndex(chi.URLParam(r, "index")) + if err != nil { + return httperror.BadRequest("shot index must be a non-negative integer") } 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) 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 +} diff --git a/internal/adapter/templates/templates/components/service/internal/service/album.go.tmpl b/internal/adapter/templates/templates/components/service/internal/service/album.go.tmpl index 04f633a..c3752b6 100644 --- a/internal/adapter/templates/templates/components/service/internal/service/album.go.tmpl +++ b/internal/adapter/templates/templates/components/service/internal/service/album.go.tmpl @@ -122,7 +122,7 @@ func (s *AlbumService) GenerateAllShots(ctx context.Context, id album.AlbumID, u return nil, fmt.Errorf("album not found: %w", err) } if a.AnchorURL == "" { - return nil, fmt.Errorf("anchor must be generated before shots") + return nil, album.ErrAnchorRequired } var jobIDs []string diff --git a/internal/adapter/templates/templates/skeleton/packages/realtime/src/index.ts b/internal/adapter/templates/templates/skeleton/packages/realtime/src/index.ts index 780f0fd..10f448f 100644 --- a/internal/adapter/templates/templates/skeleton/packages/realtime/src/index.ts +++ b/internal/adapter/templates/templates/skeleton/packages/realtime/src/index.ts @@ -4,3 +4,4 @@ export { useMediaGeneration, type GenerationStatus, type UseMediaGenerationConfi export { useChat, type UseChatConfig, type UseChatResult, type ChatMessage } from './useChat'; 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 { usePersonaGeneration, type PersonaGenerationState, type UsePersonaGenerationConfig, type UsePersonaGenerationResult } from './usePersonaGeneration'; diff --git a/internal/adapter/templates/templates/skeleton/packages/realtime/src/types.ts b/internal/adapter/templates/templates/skeleton/packages/realtime/src/types.ts index 1d3dde4..9b65d19 100644 --- a/internal/adapter/templates/templates/skeleton/packages/realtime/src/types.ts +++ b/internal/adapter/templates/templates/skeleton/packages/realtime/src/types.ts @@ -22,7 +22,17 @@ export type EventType = | 'album_anchor_failed' | 'album_shot_generating' | '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. diff --git a/internal/adapter/templates/templates/skeleton/pkg/album/handler.go.tmpl b/internal/adapter/templates/templates/skeleton/pkg/album/handler.go.tmpl index 4bfa791..e6f8180 100644 --- a/internal/adapter/templates/templates/skeleton/pkg/album/handler.go.tmpl +++ b/internal/adapter/templates/templates/skeleton/pkg/album/handler.go.tmpl @@ -3,7 +3,6 @@ package album import ( "context" "fmt" - "io" "log/slog" "net/http" "time" @@ -76,13 +75,19 @@ func AnchorHandler(mg *mediagen.Manager, store storage.Store, pub realtime.Event // Persist anchor to storage. anchorURL := img.URL - if store != nil && len(img.Data) > 0 { - storagePath := fmt.Sprintf("albums/%s/%s/anchor.png", userID, albumID) - url, uploadErr := store.Upload(ctx, storagePath, img.Data, "image/png") - if uploadErr != nil { - logger.Warn("failed to persist anchor to storage, using inline URL", "error", uploadErr, "job_id", job.ID) - } else { - anchorURL = url + if store != nil { + if len(img.Data) > 0 { + storagePath := fmt.Sprintf("albums/%s/%s/anchor.png", userID, albumID) + url, uploadErr := store.Upload(ctx, storagePath, img.Data, "image/png") + if uploadErr != nil { + logger.Warn("failed to persist anchor to storage, using inline URL", "error", uploadErr, "job_id", job.ID) + } 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) 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) @@ -150,10 +155,11 @@ func ShotHandler(mg *mediagen.Manager, store storage.Store, pub realtime.EventPu }) // Fetch anchor image bytes from storage. + const anchorMaxBytes = 20 << 20 // 20 MB — anchor images should be small PNGs var anchorBytes []byte var anchorMime string if anchorURL != "" { - data, err := fetchBytes(ctx, anchorURL) + data, err := storage.FetchURL(ctx, httpClient, anchorURL, anchorMaxBytes) if err != nil { logger.Warn("failed to fetch anchor image, proceeding without reference", "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. imageURL := img.URL - if store != nil && len(img.Data) > 0 { - storagePath := fmt.Sprintf("albums/%s/%s/shots/%d.png", userID, albumID, shotIndex) - url, uploadErr := store.Upload(ctx, storagePath, img.Data, "image/png") - if uploadErr != nil { - logger.Warn("failed to persist shot to storage, using inline URL", - "error", uploadErr, "job_id", job.ID) - } else { - imageURL = url + if store != nil { + if len(img.Data) > 0 { + storagePath := fmt.Sprintf("albums/%s/%s/shots/%d.png", userID, albumID, shotIndex) + url, uploadErr := store.Upload(ctx, storagePath, img.Data, "image/png") + if uploadErr != nil { + logger.Warn("failed to persist shot to storage, using inline URL", + "error", uploadErr, "job_id", job.ID) + } 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) 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) @@ -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 -} diff --git a/internal/adapter/templates/templates/skeleton/pkg/album/types.go.tmpl b/internal/adapter/templates/templates/skeleton/pkg/album/types.go.tmpl index 1ca259e..bc8114b 100644 --- a/internal/adapter/templates/templates/skeleton/pkg/album/types.go.tmpl +++ b/internal/adapter/templates/templates/skeleton/pkg/album/types.go.tmpl @@ -13,9 +13,14 @@ package album import ( "context" + "errors" "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. type AlbumID string diff --git a/internal/adapter/templates/templates/skeleton/pkg/generation/handlers.go.tmpl b/internal/adapter/templates/templates/skeleton/pkg/generation/handlers.go.tmpl index e10fcde..a9bf780 100644 --- a/internal/adapter/templates/templates/skeleton/pkg/generation/handlers.go.tmpl +++ b/internal/adapter/templates/templates/skeleton/pkg/generation/handlers.go.tmpl @@ -6,7 +6,6 @@ import ( "context" "encoding/base64" "fmt" - "io" "log/slog" "net/http" "time" @@ -179,6 +178,7 @@ func VideoHandler(mg *mediagen.Manager, store storage.Store, pub realtime.EventP // Build videos array matching frontend VideoResult shape: // { 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)) for i, vid := range resp.Videos { videoURL := vid.URL @@ -193,7 +193,7 @@ func VideoHandler(mg *mediagen.Manager, store storage.Store, pub realtime.EventP if len(vid.Data) > 0 { videoData = vid.Data } else if vid.URL != "" { - downloaded, downloadErr := downloadURL(ctx, vid.URL) + downloaded, downloadErr := storage.FetchURL(ctx, httpClient, vid.URL, videoMaxBytes) if downloadErr != nil { logger.Warn("failed to download video from provider", "error", downloadErr, "job_id", job.ID) } else { @@ -209,6 +209,14 @@ func VideoHandler(mg *mediagen.Manager, store storage.Store, pub realtime.EventP 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{ @@ -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 -} diff --git a/internal/adapter/templates/templates/skeleton/pkg/storage/fetch.go.tmpl b/internal/adapter/templates/templates/skeleton/pkg/storage/fetch.go.tmpl new file mode 100644 index 0000000..866f485 --- /dev/null +++ b/internal/adapter/templates/templates/skeleton/pkg/storage/fetch.go.tmpl @@ -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 +}