Conversation
- Add imdb_id field to Movie model - Fetch IMDB ID from TMDB external_ids API when adding movies - Update frontend DTOs to include imdb_id - Add IMDB link button to movie overlay with yellow badge styling - Link opens in new tab when imdb_id is available
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
📝 WalkthroughWalkthroughAdds IMDb ID support end-to-end: new ImdbID field on Movie, DAO method to update it, list service fetches imdb_id from TMDB external_ids and sets/persists it, and frontend surfaces an IMDb link/badge when available. (47 words) Changes
Sequence Diagram(s)sequenceDiagram
participant UI as Frontend (MovieOverlay)
participant API as Backend API / List Service
participant TMDB as TMDB API
participant DB as Database / GORM
UI->>API: AddMediaToList(req with TMDB id, mediaType)
API->>TMDB: GET /{movie|tv}/{id} (details)
TMDB-->>API: media details (payload)
API->>TMDB: GET /{movie|tv}/{id}/external_ids
TMDB-->>API: { "imdb_id": "ttXXXX" }
Note over API: parse imdb_id → imdbID
alt new record
API->>DB: Create Movie (includes ImdbID)
DB-->>API: Created
else existing record lacks ImdbID
API->>DB: UpdateImdbID(movieID, imdbID)
DB-->>API: Updated / error
end
API-->>UI: Movie object (includes imdb_id)
UI->>UI: Render IMDb badge/link if imdb_id present
Estimated Code Review Effort🎯 3 (Moderate) | ⏱️ ~20 minutes Possibly Related PRs
Poem
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✨ Finishing touches
Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
backend/services/list_service.go (1)
322-350: Silent error handling is acceptable for this optional enhancement.The function gracefully returns
nilon HTTP errors or decode failures, which is appropriate since the IMDb ID is supplementary data. However, consider logging these failures at debug/info level for observability—silent failures can mask configuration issues (e.g., expired API token).🔎 Optional: Add debug logging for failures
if resp.StatusCode != http.StatusOK { + // Consider: log.Debug("fetchIMDBID: non-OK status", "status", resp.StatusCode, "mediaType", mediaType, "id", id) return nil, nil } var externalIDs tmdbExternalIDsResponse if err := json.NewDecoder(resp.Body).Decode(&externalIDs); err != nil { + // Consider: log.Debug("fetchIMDBID: decode error", "error", err, "mediaType", mediaType, "id", id) return nil, nil }
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
backend/models/movie.gobackend/services/list_service.gofrontend/src/components/movie/MovieOverlay.tsxfrontend/src/services/lists.ts
🧰 Additional context used
🧬 Code graph analysis (1)
frontend/src/components/movie/MovieOverlay.tsx (1)
frontend/src/lib/utils.ts (1)
cn(4-6)
🔇 Additional comments (7)
frontend/src/services/lists.ts (1)
118-118: LGTM!The
imdb_idfield addition toMovieDTOcorrectly mirrors the backend model's nullable string type and integrates cleanly with the existing interface structure.backend/models/movie.go (1)
16-16: LGTM!The field definition is appropriate—nullable pointer for optional IMDb IDs, and
size:20is sufficient for the IMDb ID format (tt+ digits).Ensure a database migration is applied to add the
imdb_idcolumn to themoviestable, or that GORM's auto-migrate is configured to handle schema changes.frontend/src/components/movie/MovieOverlay.tsx (2)
2-2: LGTM!Import correctly added for the
ExternalLinkicon used in the IMDb badge.
164-178: Clean implementation of the IMDb link.Good attention to detail:
- Security:
rel="noopener noreferrer"prevents tab-nabbing- Accessibility:
titleattribute provides context- UX: Opens in new tab to preserve app state
- Styling: Consistent badge pattern with existing media type indicator
backend/services/list_service.go (3)
267-269: LGTM!Clean response struct for the TMDB external IDs endpoint. Only capturing
imdb_idkeeps it minimal.
382-382: Note: Sequential API calls add latency.The IMDb ID fetch happens synchronously before processing the main response. For most use cases this is fine, but if add-movie latency becomes a concern, this could be done concurrently or as a background enrichment.
405-406: LGTM!IMDb ID correctly assigned to both movie and TV entries before persistence.
Also applies to: 440-441
- Check if existing movie has imdb_id when adding to list - If missing, fetch from TMDB and update database - Add UpdateImdbID method to MovieDAO - Ensures legacy movies get IMDB link gradually
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Fix all issues with AI Agents
In @backend/services/list_service.go:
- Around line 297-307: Replace the fmt.Println call and the ignored error from
s.fetchIMDBID with structured warning logs: capture the error returned by
s.fetchIMDBID (call signature s.fetchIMDBID(ctx, mediaID, mediaType)) and, if
non-nil, log a warning with context (mediaID, mediaType) rather than ignoring
it; when UpdateImdbID (s.movies.UpdateImdbID(movie.ID, imdbID)) fails, log that
failure as a warning or error via your app logger instead of fmt.Println,
including movie.ID and the updateErr; use the existing logger instance (e.g.,
s.logger or a logger retrieved from ctx) so logs are structured and leveled
consistently across the service.
🧹 Nitpick comments (1)
backend/services/list_service.go (1)
393-393: Consider using TMDB'sappend_to_responseparameter.The code currently makes two separate HTTP requests to TMDB: one for movie/TV details and another for external IDs (line 393). TMDB's API supports an
append_to_responseparameter that allows fetching external IDs in the same request, reducing latency and network overhead.Example implementation
Modify the URL construction:
func (s *listService) fetchAndStoreFromTMDB(ctx context.Context, id int64, mediaType string) (*models.Movie, error) { var url string if mediaType == "movie" { - url = fmt.Sprintf("https://api.themoviedb.org/3/movie/%d?language=pt-BR", id) + url = fmt.Sprintf("https://api.themoviedb.org/3/movie/%d?language=pt-BR&append_to_response=external_ids", id) } else { - url = fmt.Sprintf("https://api.themoviedb.org/3/tv/%d?language=pt-BR", id) + url = fmt.Sprintf("https://api.themoviedb.org/3/tv/%d?language=pt-BR&append_to_response=external_ids", id) }Then update the response structs to include the external_ids:
type tmdbMovieResponse struct { // ... existing fields ... + ExternalIDs *tmdbExternalIDsResponse `json:"external_ids"` } type tmdbTVResponse struct { // ... existing fields ... + ExternalIDs *tmdbExternalIDsResponse `json:"external_ids"` }And extract the IMDb ID from the response instead of making a separate call:
- imdbID, _ := s.fetchIMDBID(ctx, id, mediaType) - if mediaType == "movie" { var m tmdbMovieResponse if err := json.NewDecoder(resp.Body).Decode(&m); err != nil { return nil, err } + var imdbID *string + if m.ExternalIDs != nil { + imdbID = m.ExternalIDs.ImdbID + } // ... rest of movie handling
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
backend/daos/movie_dao.gobackend/services/list_service.go
🧰 Additional context used
🧬 Code graph analysis (1)
backend/daos/movie_dao.go (1)
backend/models/movie.go (2)
Movie(7-26)Movie(28-30)
🔇 Additional comments (4)
backend/daos/movie_dao.go (2)
13-13: LGTM: Clean interface extension.The new
UpdateImdbIDmethod follows existing patterns and uses appropriate types for nullable fields.
60-62: LGTM: Correct GORM implementation.The implementation properly uses GORM's
Model().Where().Update()pattern to persist the IMDb ID.backend/services/list_service.go (2)
267-269: LGTM: Clean struct definition.The
tmdbExternalIDsResponsestruct correctly defines the IMDb ID field as a nullable pointer with appropriate JSON tags.
416-416: LGTM: IMDb ID field assignments are correct.The fetched IMDb ID is properly assigned to the
Movie.ImdbIDfield for both movie and TV show types.Also applies to: 451-451
| // If movie exists but has no IMDB ID, fetch and update it | ||
| if movie.ImdbID == nil { | ||
| if imdbID, _ := s.fetchIMDBID(ctx, mediaID, mediaType); imdbID != nil { | ||
| movie.ImdbID = imdbID | ||
| // Update the movie in the database with the IMDB ID | ||
| if updateErr := s.movies.UpdateImdbID(movie.ID, imdbID); updateErr != nil { | ||
| // Log error but don't fail the operation | ||
| fmt.Println("Warning: failed to update IMDB ID for movie", movie.ID, ":", updateErr) | ||
| } | ||
| } | ||
| } |
There was a problem hiding this comment.
Replace fmt.Println with structured logging.
Line 304 uses fmt.Println for error logging, which is not suitable for production code. Consider using a proper logging framework (e.g., log/slog, zap, or logrus) to ensure logs are properly structured, leveled, and routed.
Additionally, the error from fetchIMDBID (line 299) is silently ignored. While this may be acceptable for non-critical supplementary data, consider logging a warning if the fetch fails.
🔎 Example refactor using structured logging
// If movie exists but has no IMDB ID, fetch and update it
if movie.ImdbID == nil {
- if imdbID, _ := s.fetchIMDBID(ctx, mediaID, mediaType); imdbID != nil {
+ if imdbID, fetchErr := s.fetchIMDBID(ctx, mediaID, mediaType); imdbID != nil {
movie.ImdbID = imdbID
// Update the movie in the database with the IMDB ID
if updateErr := s.movies.UpdateImdbID(movie.ID, imdbID); updateErr != nil {
// Log error but don't fail the operation
- fmt.Println("Warning: failed to update IMDB ID for movie", movie.ID, ":", updateErr)
+ logger.Warn("failed to update IMDB ID", "movie_id", movie.ID, "error", updateErr)
}
+ } else if fetchErr != nil {
+ logger.Debug("failed to fetch IMDB ID", "media_id", mediaID, "media_type", mediaType, "error", fetchErr)
}
}Note: Replace logger with your application's logging instance.
Committable suggestion skipped: line range outside the PR's diff.
🤖 Prompt for AI Agents
In @backend/services/list_service.go around lines 297 - 307, Replace the
fmt.Println call and the ignored error from s.fetchIMDBID with structured
warning logs: capture the error returned by s.fetchIMDBID (call signature
s.fetchIMDBID(ctx, mediaID, mediaType)) and, if non-nil, log a warning with
context (mediaID, mediaType) rather than ignoring it; when UpdateImdbID
(s.movies.UpdateImdbID(movie.ID, imdbID)) fails, log that failure as a warning
or error via your app logger instead of fmt.Println, including movie.ID and the
updateErr; use the existing logger instance (e.g., s.logger or a logger
retrieved from ctx) so logs are structured and leveled consistently across the
service.
| func (s *listService) fetchIMDBID(ctx context.Context, id int64, mediaType string) (*string, error) { | ||
| var url string | ||
| if mediaType == "movie" { | ||
| url = fmt.Sprintf("https://api.themoviedb.org/3/movie/%d/external_ids", id) | ||
| } else { | ||
| url = fmt.Sprintf("https://api.themoviedb.org/3/tv/%d/external_ids", id) | ||
| } | ||
| req, err := http.NewRequestWithContext(ctx, http.MethodGet, url, nil) | ||
| if err != nil { | ||
| return nil, err | ||
| } | ||
| req.Header.Set("Accept", "application/json") | ||
| if s.tmdbToken != "" { | ||
| req.Header.Set("Authorization", "Bearer "+s.tmdbToken) | ||
| } | ||
| resp, err := s.httpClient.Do(req) | ||
| if err != nil { | ||
| return nil, err | ||
| } | ||
| defer resp.Body.Close() | ||
| if resp.StatusCode != http.StatusOK { | ||
| return nil, nil | ||
| } | ||
| var externalIDs tmdbExternalIDsResponse | ||
| if err := json.NewDecoder(resp.Body).Decode(&externalIDs); err != nil { | ||
| return nil, nil | ||
| } | ||
| return externalIDs.ImdbID, nil | ||
| } |
There was a problem hiding this comment.
Inconsistent error handling loses diagnostic information.
The fetchIMDBID method returns nil, nil on non-200 HTTP responses (line 354) and JSON decode failures (line 358), while returning actual errors for request construction and network failures (lines 341-342, 349-350). This inconsistency makes it difficult to distinguish between "no IMDb ID available" versus "an error occurred."
This approach silently swallows legitimate errors like network timeouts, malformed JSON responses, or rate limiting (429 status), which could mask operational issues.
🔎 Recommended fix
Consider one of these approaches:
Option 1: Return errors for better diagnostics
if resp.StatusCode != http.StatusOK {
- return nil, nil
+ return nil, fmt.Errorf("TMDB external_ids returned status %d", resp.StatusCode)
}
var externalIDs tmdbExternalIDsResponse
if err := json.NewDecoder(resp.Body).Decode(&externalIDs); err != nil {
- return nil, nil
+ return nil, fmt.Errorf("failed to decode TMDB external_ids: %w", err)
}Option 2: Log errors while returning nil
if resp.StatusCode != http.StatusOK {
+ logger.Warn("TMDB external_ids non-OK status", "status", resp.StatusCode, "media_id", id, "media_type", mediaType)
return nil, nil
}
var externalIDs tmdbExternalIDsResponse
if err := json.NewDecoder(resp.Body).Decode(&externalIDs); err != nil {
+ logger.Warn("failed to decode TMDB external_ids", "error", err, "media_id", id, "media_type", mediaType)
return nil, nil
}2 participants
Summary by CodeRabbit
✏️ Tip: You can customize this high-level summary in your review settings.