feat: xay dung tinh nang tim kiem movies co filter va pagination#64
Merged
Conversation
Contributor
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (1)
✅ Files skipped from review due to trivial changes (1)
📝 WalkthroughWalkthroughAdds backend endpoints and service helpers for search and movies; introduces debounced search hooks and AppBar live-search UI; adds Search and Movies pages with pagination/year filter; adds auth-gated Favourite flow and RequireLoginModal; refactors profile backgrounds and adds technical docs. ChangesAdvanced Search, Movies Browse, Favourite Auth, and Profile Styling
Sequence Diagram(s)sequenceDiagram
participant User
participant AppBar
participant useSearch
participant API
participant SearchPage
participant MediaGrid
User->>AppBar: Type search query
AppBar->>useSearch: debouncedValue (after 300ms)
useSearch->>API: GET /medias/search?q={query}
API-->>useSearch: results (max 5)
useSearch-->>AppBar: data
AppBar-->>User: Display dropdown results
User->>AppBar: Click result or press Enter
AppBar->>SearchPage: Navigate /search?q={query}
SearchPage->>useSearch: fetch full results
useSearch->>API: GET /medias/search?q={query}
API-->>useSearch: all results
useSearch-->>SearchPage: data
SearchPage->>MediaGrid: Render all results
MediaGrid-->>User: Display full results grid
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 8
🧹 Nitpick comments (6)
server/src/services/media.service.js (1)
69-70: ⚡ Quick winParallelize the two TMDB fetches.
These run sequentially, doubling latency. The existing
tmdbFetchAllhelper already usesPromise.all; do the same here.♻️ Proposed change
- const movieData = await tmdbFetch(baseMovieUrl) - const tvData = await tmdbFetch(baseTvUrl) + const [movieData, tvData] = await Promise.all([ + tmdbFetch(baseMovieUrl), + tmdbFetch(baseTvUrl), + ])🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@server/src/services/media.service.js` around lines 69 - 70, The two sequential TMDB calls (tmdbFetch with baseMovieUrl and baseTvUrl producing movieData and tvData) should be run in parallel to reduce latency; replace the sequential awaits with a parallel Promise.all (or call the existing tmdbFetchAll helper) to run tmdbFetch(baseMovieUrl) and tmdbFetch(baseTvUrl) concurrently and then destructure the results into movieData and tvData so the rest of the function uses the same variables.client/src/components/common/Modals/RequireLoginModal.jsx (1)
8-29: ⚡ Quick winAdd dialog semantics for screen-reader/keyboard users.
The overlay lacks
role="dialog",aria-modal, and a label, so assistive tech won't announce it as a modal. Adding these (and ideally an Escape-to-close handler) improves accessibility with minimal effort.♿ Proposed accessibility attributes
- <div className="fixed inset-0 z-50 flex items-center justify-center p-4"> + <div + role="dialog" + aria-modal="true" + aria-labelledby="require-login-title" + className="fixed inset-0 z-50 flex items-center justify-center p-4" + >- <h3 className="text-white font-bold text-xl">Yêu cầu đăng nhập</h3> + <h3 id="require-login-title" className="text-white font-bold text-xl">Yêu cầu đăng nhập</h3>🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@client/src/components/common/Modals/RequireLoginModal.jsx` around lines 8 - 29, The modal markup is missing accessibility semantics; add role="dialog" and aria-modal="true" to the modal container (the div that currently has className "relative bg-[`#141414`] ...") and provide a label via aria-labelledby (pointing at the modal title element) or aria-label on that same div to ensure screen-readers announce it; also add a keyboard handler in the RequireLoginModal component to listen for Escape (e.g., on mount add keydown listener that calls onClose when key === 'Escape' and remove it on unmount) and ensure the overlay div retains its onClick close behavior while clicks inside the modal (onClick on the "relative ..." div) stop propagation.client/src/pages/Movies/Movies.jsx (1)
80-87: ⚡ Quick winClean up redundant movie property mapping.
Lines 83-84 set both
posterPathandposterto the same value, and line 86 hardcodeslikedtofalse, which will display incorrect favorite state even when the favorite feature is implemented.♻️ Proposed cleanup
<MovieCard movie={{ id: movie.id, title: movie.title || movie.name, - posterPath: movie.poster_path, poster: movie.poster_path, rating: movie.vote_average, - liked: false, }} onToggleLike={handleToggleLike} />If
MovieCardrequires thelikedprop and you don't have backend state yet, consider fetching the user's favorites or passingundefinedto indicate unknown state.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@client/src/pages/Movies/Movies.jsx` around lines 80 - 87, The movie prop mapping currently duplicates posterPath and poster and hardcodes liked to false; update the object passed (the movie prop) so you only include one poster field (either poster or posterPath) to match what MovieCard expects (e.g., use poster or posterPath consistently) and remove the redundant property, and stop hardcoding liked—either derive it from the user's favorites state (if available) or pass liked: undefined (or omit liked) so the real favorite state can be applied later; look for the object creation that sets id, title, poster_path, vote_average and adjust the poster and liked fields accordingly.client/src/components/layouts/AuthLayout.jsx (1)
10-13: ⚡ Quick winExtract the glassmorphism background glow pattern into a reusable component.
The glow effect pattern (two absolutely-positioned blurred divs with
bg-primary/20andbg-primary/10) is duplicated 10 times across AuthLayout, ProfileForm, ProfileManage, and ProfileSelection. This violates the DRY principle and makes future styling updates error-prone.♻️ Proposed refactor to extract shared background component
Create a new shared component:
// client/src/components/common/GlassmorphismBackground.jsx export default function GlassmorphismBackground({ children, className = "" }) { return ( <div className={`min-h-screen text-white flex flex-col font-sans bg-bg-default relative overflow-hidden ${className}`}> {/* Background glow effects */} <div className="absolute top-[-20%] left-[-10%] w-[50%] h-[50%] bg-primary/20 blur-[120px] rounded-full pointer-events-none"></div> <div className="absolute bottom-[-20%] right-[-10%] w-[50%] h-[50%] bg-primary/10 blur-[120px] rounded-full pointer-events-none"></div> {children} </div> ); }Then simplify AuthLayout:
+import GlassmorphismBackground from '../common/GlassmorphismBackground'; import Header from './Header'; import Footer from './Footer'; export default function AuthLayout({ children }) { return ( - <div className="min-h-screen text-white flex flex-col font-sans bg-bg-default relative overflow-hidden"> - {/* Background glow effects to match global glassmorphism style */} - <div className="absolute top-[-20%] left-[-10%] w-[50%] h-[50%] bg-primary/20 blur-[120px] rounded-full pointer-events-none"></div> - <div className="absolute bottom-[-20%] right-[-10%] w-[50%] h-[50%] bg-primary/10 blur-[120px] rounded-full pointer-events-none"></div> + <GlassmorphismBackground> <Header /> <main className="grow flex items-center justify-center p-4 pb-16"> {children} </main> <div className="mt-16"> <Footer /> </div> - </div> + </GlassmorphismBackground> ); }Apply the same pattern to ProfileForm, ProfileManage, and ProfileSelection.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@client/src/components/layouts/AuthLayout.jsx` around lines 10 - 13, Extract the duplicated glassmorphism background into a reusable component and replace the repeated markup in AuthLayout (and the other files mentioned) with that component: create a new component named GlassmorphismBackground (e.g., exported as default from GlassmorphismBackground.jsx) which renders the wrapper div with the two absolute blurred glow divs and accepts children and an optional className prop, then update AuthLayout.jsx to wrap its children with <GlassmorphismBackground> (and likewise swap the duplicated blocks in ProfileForm, ProfileManage, and ProfileSelection to use GlassmorphismBackground) so styling changes are centralized.client/src/pages/MyList/Content/MovieCard.jsx (1)
30-34: ⚡ Quick winHost the IMDb logo locally for improved reliability.
Loading the IMDb logo from an external CDN (wikimedia.org) introduces a runtime dependency that could fail if the CDN is down, blocked by network policies, or slow. For production apps, critical UI assets should be self-hosted.
♻️ Proposed fix to host the logo locally
- Download the SVG from the wikimedia URL
- Add it to your assets folder (e.g.,
client/src/assets/images/imdb-logo.svg)- Update the import and src:
+import imdbLogo from '../../../assets/images/imdb-logo.svg'; import { Heart } from 'lucide-react' export default function MovieCard({ movie, onToggleLike }) { return ( <div className="group"> {/* ... */} <p className="mt-1 text-xs text-gray-400 flex items-center gap-1.5"> <img - src="https://upload.wikimedia.org/wikipedia/commons/6/69/IMDB_Logo_2016.svg" + src={imdbLogo} alt="IMDb-logo" className="w-7" /> <span>{movie.rating ? `${movie.rating.toFixed(1)}/10` : 'N/A'}</span> </p> </div> ) }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@client/src/pages/MyList/Content/MovieCard.jsx` around lines 30 - 34, The IMDb logo is currently loaded from an external URL in the MovieCard component; download the SVG and add it to the project assets (e.g., client/src/assets/images/imdb-logo.svg), then update MovieCard.jsx to import the local asset and use that import as the img src (replace the external URL in the <img> element in MovieCard.jsx with the imported symbol) so the component reliably serves the logo from the app bundle.server/src/controllers/media.controller.js (1)
68-68: 💤 Low valueOptional: Use string default for consistency.
Query parameters are strings when present, but the default
1is a number. Consider using'1'for type consistency, sinceparseIntis applied regardless.♻️ Suggested change
- const { page = 1, year } = req.query + const { page = '1', year } = req.query🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@server/src/controllers/media.controller.js` at line 68, The default for page in the req.query destructuring is a number but query params are strings; change the destructuring from const { page = 1, year } = req.query to use a string default (page = '1') so type is consistent with other query values before you call parseInt or similar; update any subsequent usage of page (e.g., parseInt(page, 10) or Number(page)) to remain unchanged.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@client/src/components/common/AppBar/AppBar.jsx`:
- Around line 153-161: The img element in AppBar.jsx currently uses an external
placeholder URL when movie.poster_path is missing; replace that discontinued
fallback with a local asset or data-URI so missing posters render reliably.
Update the component to import or reference a bundled image (e.g.,
fallbackPoster) and use it in the src expression where movie.poster_path is
falsey (the same JSX that reads movie.poster_path), or embed a small base64
data-URI directly as the fallback; ensure alt and className remain unchanged and
no external placeholder URLs are referenced.
In `@client/src/hooks/useMovies.jsx`:
- Around line 1-16: The hook useMovies currently imports axios and builds
API_BASE_URL manually, bypassing the shared HTTP client; replace
axios/API_BASE_URL usage with the existing mediaClient from your axios client
module (the one that includes withCredentials and the 401 interceptor). In
useMovies, import mediaClient (instead of axios), construct a params object {
page } and conditionally add year when present, then call
mediaClient.get('/medias/movies', { params }) and return response.data; keep the
queryKey ['movies', page, year] and queryFn shape the same so behavior and
caching remain unchanged.
In `@README_TECHNICAL.md`:
- Line 96: The README testing step for Search is incorrect about the wait time;
update the sentence that currently says "đợi ~1.5s" to reflect the actual
debounce of 500ms (e.g., "đợi ~0.5s") so it matches the debounce implementation
and removes the mismatch between the README and the Search/Header debounce
behavior.
- Line 18: The README incorrectly documents the debounce delay as "1.5s - 3s";
update the documentation to match the actual implementation by changing the
description to "0.5s (500ms)" or "500ms" where it mentions the Custom Hook
useDebounce, since AppBar.jsx calls useDebounce(searchTerm, 500); alternatively,
if the intended behavior is 1.5–3s instead, change the call in
client/src/components/common/AppBar/AppBar.jsx from useDebounce(searchTerm, 500)
to the desired millisecond value and keep the README as-is—ensure consistency
between useDebounce usage and README text.
In `@server/src/controllers/media.controller.js`:
- Line 69: Validate incoming page and year before calling getMoviesList: parse
page and year using parseInt, ensure page is a positive integer (>=1) and year
is either null or an integer within 1888–2100; if validation fails, respond with
a 400 error or coerce to a safe default (e.g., page=1, year=null) rather than
passing invalid values to getMoviesList; update the controller code around the
parseInt(page) call and the year handling to use Number.isInteger checks and
explicit range checks and include clear error messages when rejecting
parameters.
- Line 69: Validate and sanitize page and year in getMovies
(media.controller.js) before calling getMoviesList: use parseInt(page, 10) and
parseInt(year, 10) with checks that parsed values are finite (not NaN), enforce
bounds (page >= 1; year within a sensible range, e.g., >= 1900 and <=
currentYear+1), and reject requests (return a 400) if validation fails; only
call getMoviesList(parsedPage, parsedYearOrNull) when values pass validation so
getMoviesList never receives NaN/invalid values.
- Around line 17-19: The controller currently only checks falsiness for
req.query.q before calling getMediasSearch; add input validation in the media
controller to ensure q is a string and within a reasonable max length (e.g., <=
200 chars) and return res.status(400).json({ error: 'Invalid query' }) for
invalid input rather than forwarding it to getMediasSearch; keep using
getMediasSearch(q) for valid queries and preserve existing behavior for empty
results.
In `@server/src/services/media.service.js`:
- Around line 75-83: The current merge in media.service.js (combinedData ->
sorted -> slice(0, pageSize)) discards half the fetched items but still reports
totalResults as the sum of movieData and tvData, causing inconsistent
pagination; update the return metadata to reflect the actual 20-item-per-page
contract by computing totalResults as the larger of movieData.total_results and
tvData.total_results (e.g., totalResults = Math.max(movieData.total_results ||
0, tvData.total_results || 0)) and derive totalPages = Math.ceil(totalResults /
pageSize), or implement the alternate approach from the review (fetch enough
items from each source to cover page*pageSize and then slice the combined
window) if you want true interleaved pagination — adjust the code around
combinedData, sorted, slice, and the returned object accordingly.
---
Nitpick comments:
In `@client/src/components/common/Modals/RequireLoginModal.jsx`:
- Around line 8-29: The modal markup is missing accessibility semantics; add
role="dialog" and aria-modal="true" to the modal container (the div that
currently has className "relative bg-[`#141414`] ...") and provide a label via
aria-labelledby (pointing at the modal title element) or aria-label on that same
div to ensure screen-readers announce it; also add a keyboard handler in the
RequireLoginModal component to listen for Escape (e.g., on mount add keydown
listener that calls onClose when key === 'Escape' and remove it on unmount) and
ensure the overlay div retains its onClick close behavior while clicks inside
the modal (onClick on the "relative ..." div) stop propagation.
In `@client/src/components/layouts/AuthLayout.jsx`:
- Around line 10-13: Extract the duplicated glassmorphism background into a
reusable component and replace the repeated markup in AuthLayout (and the other
files mentioned) with that component: create a new component named
GlassmorphismBackground (e.g., exported as default from
GlassmorphismBackground.jsx) which renders the wrapper div with the two absolute
blurred glow divs and accepts children and an optional className prop, then
update AuthLayout.jsx to wrap its children with <GlassmorphismBackground> (and
likewise swap the duplicated blocks in ProfileForm, ProfileManage, and
ProfileSelection to use GlassmorphismBackground) so styling changes are
centralized.
In `@client/src/pages/Movies/Movies.jsx`:
- Around line 80-87: The movie prop mapping currently duplicates posterPath and
poster and hardcodes liked to false; update the object passed (the movie prop)
so you only include one poster field (either poster or posterPath) to match what
MovieCard expects (e.g., use poster or posterPath consistently) and remove the
redundant property, and stop hardcoding liked—either derive it from the user's
favorites state (if available) or pass liked: undefined (or omit liked) so the
real favorite state can be applied later; look for the object creation that sets
id, title, poster_path, vote_average and adjust the poster and liked fields
accordingly.
In `@client/src/pages/MyList/Content/MovieCard.jsx`:
- Around line 30-34: The IMDb logo is currently loaded from an external URL in
the MovieCard component; download the SVG and add it to the project assets
(e.g., client/src/assets/images/imdb-logo.svg), then update MovieCard.jsx to
import the local asset and use that import as the img src (replace the external
URL in the <img> element in MovieCard.jsx with the imported symbol) so the
component reliably serves the logo from the app bundle.
In `@server/src/controllers/media.controller.js`:
- Line 68: The default for page in the req.query destructuring is a number but
query params are strings; change the destructuring from const { page = 1, year }
= req.query to use a string default (page = '1') so type is consistent with
other query values before you call parseInt or similar; update any subsequent
usage of page (e.g., parseInt(page, 10) or Number(page)) to remain unchanged.
In `@server/src/services/media.service.js`:
- Around line 69-70: The two sequential TMDB calls (tmdbFetch with baseMovieUrl
and baseTvUrl producing movieData and tvData) should be run in parallel to
reduce latency; replace the sequential awaits with a parallel Promise.all (or
call the existing tmdbFetchAll helper) to run tmdbFetch(baseMovieUrl) and
tmdbFetch(baseTvUrl) concurrently and then destructure the results into
movieData and tvData so the rest of the function uses the same variables.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: 520a9579-07a3-4971-b355-d32158d30a9a
📒 Files selected for processing (30)
README_TECHNICAL.mdclient/src/App.jsxclient/src/components/common/ActionButton/FavouriteButton/FavouriteButton.jsxclient/src/components/common/AppBar/AppBar.jsxclient/src/components/common/Filters/YearFilter.jsxclient/src/components/common/Filters/index.jsclient/src/components/common/Modals/RequireLoginModal.jsxclient/src/components/common/Pagination/Pagination.jsxclient/src/components/common/Pagination/index.jsclient/src/components/common/Skeletons/MovieCardSkeleton.jsxclient/src/components/common/Skeletons/index.jsclient/src/components/layouts/AuthLayout.jsxclient/src/hooks/useDebounce.jsxclient/src/hooks/useMovies.jsxclient/src/hooks/useSearch.jsxclient/src/pages/Movies/Movies.jsxclient/src/pages/Movies/index.jsclient/src/pages/MyList/Content/MovieCard.jsxclient/src/pages/Profile/ProfileForm.jsxclient/src/pages/Profile/ProfileManage.jsxclient/src/pages/Profile/ProfileSelection.jsxclient/src/pages/Profile/index.jsclient/src/pages/Search/Search.jsxclient/src/profile/ProfileManage/ProfileManage.jsxclient/src/profile/ProfileManage/index.jsclient/src/profile/ProfileSelection/ProfileSelection.jsxclient/src/profile/ProfileSelection/index.jsserver/src/controllers/media.controller.jsserver/src/routes/media.route.jsserver/src/services/media.service.js
💤 Files with no reviewable changes (4)
- client/src/profile/ProfileSelection/ProfileSelection.jsx
- client/src/profile/ProfileSelection/index.js
- client/src/profile/ProfileManage/ProfileManage.jsx
- client/src/profile/ProfileManage/index.js
Comment on lines
+153
to
+161
| <img | ||
| src={ | ||
| movie.poster_path | ||
| ? `https://image.tmdb.org/t/p/w92${movie.poster_path}` | ||
| : 'https://via.placeholder.com/92x138?text=No+Image' | ||
| } | ||
| alt="poster" | ||
| className="w-12 h-16 object-cover rounded-md shrink-0" | ||
| /> |
Contributor
There was a problem hiding this comment.
🧩 Analysis chain
🌐 Web query:
Is via.placeholder.com placeholder image service still online 2025
💡 Result:
The via.placeholder.com service is no longer online as a placeholder image provider [1][2]. The domain was acquired around 2024 and subsequently ceased providing image generation services, resulting in connection-refused errors for requests targeting it [1][2][3]. Developers who previously relied on via.placeholder.com are advised to migrate to alternative services, such as placehold.co, which is commonly used as a replacement for generating placeholder images [1][3].
Citations:
- 1: Angular AI Tutor: via.placeholder.com does no longer work angular/angular-cli#32908
- 2: fix: via.placeholder.com shut down in 2024 (now connection-refused) vercel/satori#751
- 3: Placeholder.com no longer provides placeholder images faker-js/faker#2477
Replace discontinued poster fallback
In client/src/components/common/AppBar/AppBar.jsx (lines 153-161), the fallback URL https://via.placeholder.com/92x138?text=No+Image points to a placeholder service that is no longer online, so images will fail to load when movie.poster_path is missing. Use a local asset (or inline/data-URI) as the fallback instead.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@client/src/components/common/AppBar/AppBar.jsx` around lines 153 - 161, The
img element in AppBar.jsx currently uses an external placeholder URL when
movie.poster_path is missing; replace that discontinued fallback with a local
asset or data-URI so missing posters render reliably. Update the component to
import or reference a bundled image (e.g., fallbackPoster) and use it in the src
expression where movie.poster_path is falsey (the same JSX that reads
movie.poster_path), or embed a small base64 data-URI directly as the fallback;
ensure alt and className remain unchanged and no external placeholder URLs are
referenced.
Comment on lines
+1
to
+16
| import { useQuery } from '@tanstack/react-query' | ||
| import axios from 'axios' | ||
|
|
||
| const API_BASE_URL = import.meta.env.VITE_API_BASE_URL || 'http://localhost:3000/api' | ||
|
|
||
| export const useMovies = (page = 1, year = null) => { | ||
| return useQuery({ | ||
| queryKey: ['movies', page, year], | ||
| queryFn: async () => { | ||
| const params = new URLSearchParams() | ||
| params.append('page', page) | ||
| if (year) params.append('year', year) | ||
|
|
||
| const response = await axios.get(`${API_BASE_URL}/medias/movies?${params}`) | ||
| return response.data | ||
| }, |
Contributor
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
rg -nP -C3 '\bmediaClient\b' --type=js -g '*axiosClient*'Repository: LoiTang1710/MovieApp
Length of output: 414
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Show authClient creation / baseURL in axiosClient.js
echo "=== client/src/api/axiosClient.js (around authClient) ==="
sed -n '1,200p' client/src/api/axiosClient.js | nl -ba | sed -n '1,200p'
# Show useMovies file fully (it's likely short) to confirm imports/axios usage
echo "=== client/src/hooks/useMovies.jsx ==="
sed -n '1,200p' client/src/hooks/useMovies.jsx | nl -ba | sed -n '1,200p'
# Find sibling useSearch to confirm it uses mediaClient
echo "=== Find useSearch usage of mediaClient ==="
rg -n "useSearch|mediaClient" client/src/hooks -S || trueRepository: LoiTang1710/MovieApp
Length of output: 183
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "=== client/src/api/axiosClient.js (first 200 lines) ==="
sed -n '1,200p' client/src/api/axiosClient.js | cat -n
echo "=== client/src/hooks/useMovies.jsx (full file) ==="
sed -n '1,200p' client/src/hooks/useMovies.jsx | cat -n
echo "=== useSearch hook(s) and mediaClient usage in client/src/hooks ==="
rg -n "useSearch|mediaClient" client/src/hooks -SRepository: LoiTang1710/MovieApp
Length of output: 3638
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "=== client/src/utils/env.js (resolveServerUrl) ==="
rg -n "function resolveServerUrl|resolveServerUrl\s*=" client/src/utils/env.js
sed -n '1,200p' client/src/utils/env.js | cat -nRepository: LoiTang1710/MovieApp
Length of output: 1000
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Find Vite config files
ls -la | head
fd -a "vite.config.*" . || true
fd -a "vite.*config.*" . || true
# Search for "proxy" in Vite config
rg -n "proxy" -S -g "vite.config.*" -g "*vite*config*" . || trueRepository: LoiTang1710/MovieApp
Length of output: 1040
🏁 Script executed:
#!/bin/bash
set -euo pipefail
sed -n '1,200p' client/vite.config.js | cat -nRepository: LoiTang1710/MovieApp
Length of output: 1315
Use mediaClient in useMovies to match the shared HTTP client
useMovies imports raw axios and builds its own base URL via import.meta.env.VITE_API_BASE_URL, bypassing the shared mediaClient/authClient setup in client/src/api/axiosClient.js (notably withCredentials: true and the 401 response interceptor). Other hooks (including useSearch) already use mediaClient with /medias/* paths.
♻️ Proposed refactor
-import { useQuery } from '`@tanstack/react-query`'
-import axios from 'axios'
-
-const API_BASE_URL = import.meta.env.VITE_API_BASE_URL || 'http://localhost:3000/api'
+import { useQuery } from '`@tanstack/react-query`'
+import { mediaClient } from '../api/axiosClient'
export const useMovies = (page = 1, year = null) => {
return useQuery({
queryKey: ['movies', page, year],
queryFn: async () => {
- const params = new URLSearchParams()
- params.append('page', page)
- if (year) params.append('year', year)
-
- const response = await axios.get(`${API_BASE_URL}/medias/movies?${params}`)
+ const response = await mediaClient.get('/medias/movies', {
+ params: { page, ...(year ? { year } : {}) },
+ })
return response.data
},🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@client/src/hooks/useMovies.jsx` around lines 1 - 16, The hook useMovies
currently imports axios and builds API_BASE_URL manually, bypassing the shared
HTTP client; replace axios/API_BASE_URL usage with the existing mediaClient from
your axios client module (the one that includes withCredentials and the 401
interceptor). In useMovies, import mediaClient (instead of axios), construct a
params object { page } and conditionally add year when present, then call
mediaClient.get('/medias/movies', { params }) and return response.data; keep the
queryKey ['movies', page, year] and queryFn shape the same so behavior and
caching remain unchanged.
Comment on lines
+17
to
+19
| const { q } = req.query | ||
| if (!q) return res.status(StatusCodes.OK).json([]) | ||
| const data = await getMediasSearch(q) |
Contributor
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Description: Verify if getMediasSearch validates or sanitizes the query parameter
# Check the service implementation
ast-grep --pattern 'export const getMediasSearch = $$$'
# Also check for any validation in the service layer
rg -A 10 'getMediasSearch' server/src/services/media.service.jsRepository: LoiTang1710/MovieApp
Length of output: 960
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# 1) Inspect controller around getSearch
echo "---- controller: server/src/controllers/media.controller.js (lines ~1-60) ----"
nl -ba server/src/controllers/media.controller.js | sed -n '1,120p'
# 2) Locate tmdbFetchAll implementation
echo "---- locating tmdbFetchAll ----"
rg -n "tmdbFetchAll" server/src -S
echo "---- showing tmdbFetchAll implementation ----"
# pick the first match and show its surrounding lines (if multiple, show up to ~80 lines each)
matches=$(rg -n "tmdbFetchAll" server/src -S | cut -d: -f1 | sort -u)
for f in $matches; do
echo "=== $f ==="
nl -ba "$f" | sed -n '1,220p'
doneRepository: LoiTang1710/MovieApp
Length of output: 204
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "---- controller: server/src/controllers/media.controller.js (first 160 lines) ----"
cat -n server/src/controllers/media.controller.js | sed -n '1,160p'
echo "---- locating tmdbFetchAll ----"
rg -n "tmdbFetchAll" server/src -S || true
echo "---- showing tmdbFetchAll implementation (up to 220 lines each match file) ----"
files=$(rg -l "tmdbFetchAll" server/src -S || true)
for f in $files; do
echo "=== $f ==="
cat -n "$f" | sed -n '1,220p'
doneRepository: LoiTang1710/MovieApp
Length of output: 7114
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "---- locate tmdbFetch ----"
rg -n "export const tmdbFetch|const tmdbFetch" server/src/utils -S || true
echo "---- show tmdbFetch implementation ----"
fd -a "tmdbFetch.js" server/src/utils 2>/dev/null || true
# Prefer known path from earlier import
if [ -f server/src/utils/tmdbFetch.js ]; then
cat -n server/src/utils/tmdbFetch.js
else
# fallback: search and print the first matching file
f=$(rg -l "tmdbFetch" server/src/utils -S | head -n 1 || true)
if [ -n "${f:-}" ]; then cat -n "$f"; fi
fiRepository: LoiTang1710/MovieApp
Length of output: 926
Validate q before TMDB search (type + length).
getSearch forwards q with only a falsy check; while getMediasSearch applies encodeURIComponent, there’s still no input validation at the controller boundary (e.g., non-string/very long q can trigger heavy/failed upstream requests). Add type/maximum length checks and return 400 for invalid input.
🛡️ Proposed validation
export const getSearch = catchAsync(async (req, res) => {
const { q } = req.query
if (!q) return res.status(StatusCodes.OK).json([])
+ if (typeof q !== 'string' || q.length > 200) {
+ return res.status(StatusCodes.BAD_REQUEST).json({ error: 'Invalid search query' })
+ }
const data = await getMediasSearch(q)
res.status(StatusCodes.OK).json(data)
})📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| const { q } = req.query | |
| if (!q) return res.status(StatusCodes.OK).json([]) | |
| const data = await getMediasSearch(q) | |
| export const getSearch = catchAsync(async (req, res) => { | |
| const { q } = req.query | |
| if (!q) return res.status(StatusCodes.OK).json([]) | |
| if (typeof q !== 'string' || q.length > 200) { | |
| return res.status(StatusCodes.BAD_REQUEST).json({ error: 'Invalid search query' }) | |
| } | |
| const data = await getMediasSearch(q) | |
| res.status(StatusCodes.OK).json(data) | |
| }) |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@server/src/controllers/media.controller.js` around lines 17 - 19, The
controller currently only checks falsiness for req.query.q before calling
getMediasSearch; add input validation in the media controller to ensure q is a
string and within a reasonable max length (e.g., <= 200 chars) and return
res.status(400).json({ error: 'Invalid query' }) for invalid input rather than
forwarding it to getMediasSearch; keep using getMediasSearch(q) for valid
queries and preserve existing behavior for empty results.
|
|
||
| export const getMovies = catchAsync(async (req, res) => { | ||
| const { page = 1, year } = req.query | ||
| const data = await getMoviesList(parseInt(page), year ? parseInt(year) : null) |
Contributor
There was a problem hiding this comment.
Add bounds validation for page and year parameters.
The controller does not validate that page and year are within reasonable ranges:
pageshould be positive (≥ 1)yearshould be within a realistic movie year range (e.g., 1888-2100)
Invalid values could cause poor UX, performance issues, or errors in the TMDB API.
🛡️ Proposed bounds validation
export const getMovies = catchAsync(async (req, res) => {
const { page = 1, year } = req.query
const parsedPage = parseInt(page, 10)
const parsedYear = year ? parseInt(year, 10) : null
if (isNaN(parsedPage)) {
return res.status(StatusCodes.BAD_REQUEST).json({ error: 'Invalid page number' })
}
+ if (parsedPage < 1) {
+ return res.status(StatusCodes.BAD_REQUEST).json({ error: 'Page must be at least 1' })
+ }
+
if (parsedYear !== null && isNaN(parsedYear)) {
return res.status(StatusCodes.BAD_REQUEST).json({ error: 'Invalid year' })
}
+ if (parsedYear !== null && (parsedYear < 1888 || parsedYear > 2100)) {
+ return res.status(StatusCodes.BAD_REQUEST).json({ error: 'Year must be between 1888 and 2100' })
+ }
const data = await getMoviesList(parsedPage, parsedYear)
res.status(StatusCodes.OK).json(data)
})🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@server/src/controllers/media.controller.js` at line 69, Validate incoming
page and year before calling getMoviesList: parse page and year using parseInt,
ensure page is a positive integer (>=1) and year is either null or an integer
within 1888–2100; if validation fails, respond with a 400 error or coerce to a
safe default (e.g., page=1, year=null) rather than passing invalid values to
getMoviesList; update the controller code around the parseInt(page) call and the
year handling to use Number.isInteger checks and explicit range checks and
include clear error messages when rejecting parameters.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Description: Check if getMoviesList validates page and year parameters
# Find the service function implementation
ast-grep --pattern 'export const getMoviesList = $$$'
# Check for validation logic
rg -A 15 'getMoviesList' server/src/services/media.service.jsRepository: LoiTang1710/MovieApp
Length of output: 2958
🏁 Script executed:
#!/bin/bash
# Inspect controller around the getMovies handler and imports (StatusCodes etc.)
set -euo pipefail
sed -n '1,120p' server/src/controllers/media.controller.js | nl -ba
echo "----"
sed -n '120,240p' server/src/controllers/media.controller.js | nl -ba
echo "----"
# Also inspect getMoviesList implementation region for how it uses page/year
sed -n '1,140p' server/src/services/media.service.js | nl -baRepository: LoiTang1710/MovieApp
Length of output: 107
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== controller: server/src/controllers/media.controller.js (lines ~1-120) =="
sed -n '1,140p' server/src/controllers/media.controller.js | cat -n
echo "----"
echo "== controller: server/src/controllers/media.controller.js (search for getMovies handler) =="
rg -n "getMovies" server/src/controllers/media.controller.js || true
echo "----"
echo "== controller: around parseInt usage =="
rg -n "parseInt\\(page\\)|parseInt\\(year\\)" server/src/controllers/media.controller.js || true
echo "----"
echo "== service: server/src/services/media.service.js (lines ~1-130) =="
sed -n '1,140p' server/src/services/media.service.js | cat -nRepository: LoiTang1710/MovieApp
Length of output: 6989
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== tmdbFetch implementation =="
sed -n '1,220p' server/src/utils/tmdbFetch.js | cat -n
echo "----"
echo "== catchAsync usage (to see how errors are surfaced) =="
sed -n '1,200p' server/src/utils/catchAsync.js | cat -nRepository: LoiTang1710/MovieApp
Length of output: 936
Add numeric parsing validation for page/year before calling getMoviesList.
getMovies currently calls getMoviesList(parseInt(page), year ? parseInt(year) : null), and getMoviesList interpolates page/year directly into TMDB discover URLs with no validation. Non-numeric inputs (e.g., page=abc, year=invalid) become NaN in the URL, and tmdbFetch throws on non-OK responses—causing request failures.
- Critical: reject non-finite/NaN
pageandyearbefore reaching the service layer. - Major: add basic bounds checks (e.g.,
page >= 1andyearwithin an expected range) before building TMDB queries. - Minor: use
parseInt(..., 10)for clarity.
🐛 Proposed fix with validation
export const getMovies = catchAsync(async (req, res) => {
const { page = 1, year } = req.query
- const data = await getMoviesList(parseInt(page), year ? parseInt(year) : null)
+ const parsedPage = parseInt(page, 10)
+ const parsedYear = year ? parseInt(year, 10) : null
+
+ if (isNaN(parsedPage)) {
+ return res.status(StatusCodes.BAD_REQUEST).json({ error: 'Invalid page number' })
+ }
+ if (parsedYear !== null && isNaN(parsedYear)) {
+ return res.status(StatusCodes.BAD_REQUEST).json({ error: 'Invalid year' })
+ }
+
+ const data = await getMoviesList(parsedPage, parsedYear)
res.status(StatusCodes.OK).json(data)
})📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| const data = await getMoviesList(parseInt(page), year ? parseInt(year) : null) | |
| const parsedPage = parseInt(page, 10) | |
| const parsedYear = year ? parseInt(year, 10) : null | |
| if (isNaN(parsedPage)) { | |
| return res.status(StatusCodes.BAD_REQUEST).json({ error: 'Invalid page number' }) | |
| } | |
| if (parsedYear !== null && isNaN(parsedYear)) { | |
| return res.status(StatusCodes.BAD_REQUEST).json({ error: 'Invalid year' }) | |
| } | |
| const data = await getMoviesList(parsedPage, parsedYear) |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@server/src/controllers/media.controller.js` at line 69, Validate and sanitize
page and year in getMovies (media.controller.js) before calling getMoviesList:
use parseInt(page, 10) and parseInt(year, 10) with checks that parsed values are
finite (not NaN), enforce bounds (page >= 1; year within a sensible range, e.g.,
>= 1900 and <= currentYear+1), and reject requests (return a 400) if validation
fails; only call getMoviesList(parsedPage, parsedYearOrNull) when values pass
validation so getMoviesList never receives NaN/invalid values.
Comment on lines
+75
to
+83
| const combinedData = [...movies, ...tvs] | ||
| const sorted = combinedData.sort((a, b) => b.popularity - a.popularity) | ||
|
|
||
| return { | ||
| results: sorted.slice(0, pageSize), | ||
| page, | ||
| totalPages: Math.max(movieData.total_pages || 1, tvData.total_pages || 1), | ||
| totalResults: (movieData.total_results || 0) + (tvData.total_results || 0), | ||
| } |
Contributor
There was a problem hiding this comment.
Pagination silently drops half of every page's results.
You fetch page N of movies (20 items) and page N of TV (20 items), merge into 40, sort by popularity, then slice(0, 20). The 20 lower-popularity items are discarded and never surface on any page — the next request fetches page N+1 of each source, so that dropped data is lost permanently. This also makes the metadata inconsistent: totalResults is the sum of both sources and totalPages is the max, but each page only emits 20 of the 40 fetched items, so the reported counts don't match what the client can actually page through.
Consider one of:
- Treat movies and TV as a single interleaved feed but page over the combined offset (e.g. fetch enough from each source to cover
page*pageSizeand slice the correct window), or - Keep a single TMDB source per page, or expose movie/TV pagination separately so counts stay consistent.
At minimum, totalPages/totalResults must reflect the 20-per-page contract actually returned.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@server/src/services/media.service.js` around lines 75 - 83, The current merge
in media.service.js (combinedData -> sorted -> slice(0, pageSize)) discards half
the fetched items but still reports totalResults as the sum of movieData and
tvData, causing inconsistent pagination; update the return metadata to reflect
the actual 20-item-per-page contract by computing totalResults as the larger of
movieData.total_results and tvData.total_results (e.g., totalResults =
Math.max(movieData.total_results || 0, tvData.total_results || 0)) and derive
totalPages = Math.ceil(totalResults / pageSize), or implement the alternate
approach from the review (fetch enough items from each source to cover
page*pageSize and then slice the combined window) if you want true interleaved
pagination — adjust the code around combinedData, sorted, slice, and the
returned object accordingly.
Co-authored-by: coderabbitai[bot] <136622811+coderabbitai[bot]@users.noreply.github.com>
Co-authored-by: coderabbitai[bot] <136622811+coderabbitai[bot]@users.noreply.github.com>
This was referenced Jun 2, 2026
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary by CodeRabbit
New Features
Components
UI
Documentation
Removed