Spec 02 because you liked#5
Conversation
… the current profile
…e/dislike functionality
…ndations based on provided movie IDs
…based on user preferences
…e recommendations
…ations in BecauseYouLiked component
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
5019fa8 to
ce2d8bd
Compare
devferx
left a comment
There was a problem hiding this comment.
Automated code review (high effort, 8-angle recall-biased pass, verified against specs/02-because-you-liked.md). 2 confirmed correctness bugs, 1 plausible race condition, 4 cleanup/simplification notes. See inline comments.
| export const BecauseYouLiked = () => { | ||
| const getCurrentLikedMovies = useStore( | ||
| useUserMoviePreferences, | ||
| (store) => store.getCurrentLikedMovies, |
There was a problem hiding this comment.
Bug (confirmed): selector never triggers a re-render
This selector returns the uninvoked getCurrentLikedMovies function reference instead of calling it. Since that function reference is never reassigned by any set() call, it is Object.is-stable across every store update, so useSyncExternalStore never signals a change to this subscriber.
Repro: user has 0 liked movies at page load (section renders null). User likes their first movie via MovieCard/MovieHero/MovieModal elsewhere on the page without a full reload. The section stays hidden for the rest of the session.
Note the spec itself (specs/02-because-you-liked.md:123) specifies (s) => s.getCurrentLikedMovies() — invoked — so this looks like a shipped deviation from the intended selector, not intended behavior.
| (store) => store.getCurrentLikedMovies, | |
| const likedMovies = useStore(useUserMoviePreferences, (store) => | |
| store.getCurrentLikedMovies(), | |
| ) |
|
|
||
| const sourceMovies = getRandomItems(likedMovies, MAX_SOURCE_MOVIES) | ||
|
|
||
| getMoviesRecommendations(sourceMovies.map((movie) => movie.id)).then( |
There was a problem hiding this comment.
Bug (confirmed): unhandled rejection permanently freezes the loading state
getMoviesRecommendations runs up to MAX_SOURCE_MOVIES (5) concurrent TMDB calls via Promise.all with no try/catch, and the underlying axios instance (movie-api.ts) uses the default validateStatus, so any TMDB 429/5xx throws. This .then() has no .catch(), so a rejection is unhandled and setRows is never called.
Worse: hasFetchedRef.current = true (line 49) is set synchronously before the await, so the effect's own guard (if (hasFetchedRef.current) return) blocks any retry — BecauseYouLikedSkeleton renders indefinitely for the rest of the session with no recovery path short of a full reload.
Suggest wrapping in try/catch and resetting hasFetchedRef.current = false on failure (or storing an error state) so a future render/navigation can retry.
|
|
||
| const onClickDislike = (event: React.MouseEvent) => { | ||
| event.stopPropagation() | ||
| if (isMovieLiked) { |
There was a problem hiding this comment.
Plausible: narrow race on rapid Like → Dislike clicks
isMovieLiked comes from the SSR-safe useStore wrapper (src/store/use-store.tsx), which syncs its returned value one render/effect cycle behind the live selector result. If a user clicks Like then immediately clicks Dislike before that effect flushes and re-renders, this closure can still see the stale isMovieLiked === false and skip updateLikedMovies(movie), leaving the movie simultaneously liked and disliked in persisted state — which contradicts the documented "dislike always clears an existing like" invariant (specs/02-because-you-liked.md:18).
Low likelihood (needs two clicks within the same paint cycle), but worth a look given it undermines an explicit spec invariant.
| updateUserMovies(movie) | ||
| } | ||
|
|
||
| const onClickLike = (event: React.MouseEvent) => { |
There was a problem hiding this comment.
Cleanup: duplicated Like/Dislike handler logic across 3 files
This isMovieLiked read + onClickLike/onClickDislike pair is duplicated near-verbatim in movie-hero.tsx and movie-modal.tsx. A shared useMovieLikeActions(movie) hook returning { isMovieLiked, onClickLike, onClickDislike } would collapse all three call sites and keep the toggle semantics (e.g. the deferred "like clears dislike" behavior mentioned in the spec) in one place instead of three.
| @@ -1,30 +1,54 @@ | |||
| 'use client' | |||
There was a problem hiding this comment.
Altitude: whole component client-ified for 2 buttons
MovieHero was previously server-rendered (title, backdrop image, overview, Play/Info links are all static). Adding 'use client' here to wire the Like/Dislike buttons pulls the entire hero — image, links, animation classes — into the client bundle. Consider isolating just the interactive Like/Dislike controls into a small client subcomponent composed into an otherwise-server MovieHero.
| (set, get) => ({ | ||
| likedMoviesByProfileId: {}, | ||
| dislikedMoviesByProfileId: {}, | ||
| getCurrentLikedMovies: function (): Movie[] { |
There was a problem hiding this comment.
Reuse: duplicates getCurrentUserMovies's pattern
This is a near line-for-line copy of getCurrentUserMovies in user-movies-store.ts (guard on missing profileId, then Object.values(get().map[profileId] || {})). A shared generic accessor/factory for "profile-scoped map values" would avoid a second (soon third?) copy of this logic drifting apart over time.
| <section className="grid gap-4"> | ||
| <div className="mx-16 h-6 w-64 animate-pulse rounded bg-white/10" /> | ||
| <div className="mx-16 flex gap-4"> | ||
| {Array.from({ length: 5 }).map((_, index) => ( |
There was a problem hiding this comment.
Simplification: magic number instead of the constant defined above
Array.from({ length: 5 }) should reuse MAX_SOURCE_MOVIES (defined at line 14) so the skeleton's placeholder count can't silently drift out of sync with the real row count.
No description provided.