[2단계 - 상세 정보 & UI/UX 개선하기] 찰리 미션 제출합니다. #292
[2단계 - 상세 정보 & UI/UX 개선하기] 찰리 미션 제출합니다. #292yuncic wants to merge 58 commits intowoowacourse:yuncicfrom
Conversation
Co-authored-by: yuncic <ksy151020@gmail.com>
Co-authored-by: yuncic <ksy151020@gmail.com>
Co-authored-by: yuncic <ksy151020@gmail.com>
Co-authored-by: yuncic <ksy151020@gmail.com>
Co-authored-by: yuncic <ksy151020@gmail.com>
Co-authored-by: yuncic <ksy151020@gmail.com>
Co-authored-by: yuncic <ksy151020@gmail.com>
Co-authored-by: yuncic <ksy151020@gmail.com>
Co-authored-by: yuncic <ksy151020@gmail.com>
Co-authored-by: yuncic <ksy151020@gmail.com>
Co-authored-by: yuncic <ksy151020@gmail.com>
Co-authored-by: yuncic <ksy151020@gmail.com>
Co-authored-by: yuncic <ksy151020@gmail.com>
Co-authored-by: yuncic <ksy151020@gmail.com>
Co-authored-by: yuncic <ksy151020@gmail.com>
Co-authored-by: yuncic <ksy151020@gmail.com>
Co-authored-by: yuncic <ksy151020@gmail.com>
Co-authored-by: yuncic <ksy151020@gmail.com>
Co-authored-by: yuncic <ksy151020@gmail.com>
Co-authored-by: yuncic <ksy151020@gmail.com>
Co-authored-by: yuncic <ksy151020@gmail.com>
Co-authored-by: yuncic <ksy151020@gmail.com>
Co-authored-by: yuncic <ksy151020@gmail.com>
Co-authored-by: yuncic <ksy151020@gmail.com>
Co-authored-by: yuncic <ksy151020@gmail.com>
Co-authored-by: yuncic <ksy151020@gmail.com>
- refactor: fetchMovieList 공통 함수, baseURL 상수, callMovieList로 호출 통합
- 영화 포스터 - 제목 - 평균 별점 - 줄거리
|
Important Review skippedAuto incremental reviews are disabled on this repository. Please check the settings in the CodeRabbit UI or the ⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Run ID: You can disable this status message by setting the Use the checkbox below for a quick retry:
Walkthrough이 PR은 영화 리뷰 애플리케이션을 구현합니다. TMDB API를 호출하는 TypeScript 서비스 모듈, DOM 렌더링 유틸리티, 검색/스크롤/포스터 클릭 이벤트 바인딩 로직을 추가합니다. HTML 구조를 동적 모달과 검색 UI로 확장하고, 반응형 미디어 쿼리와 스켈레톤 로딩 스타일을 포함한 광범위한 CSS 업데이트를 적용합니다. 5개의 Cypress E2E 테스트 스위트와 4개의 JSON 테스트 픽스처를 추가하여 포스터 클릭, 검색, 모달 동작, 무한 스크롤, 별점 기능을 검증합니다. Estimated code review effort🎯 4 (Complex) | ⏱️ ~50 minutes 🚥 Pre-merge checks | ✅ 3✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
There was a problem hiding this comment.
Actionable comments posted: 16
Note
Due to the large number of review comments, Critical, Major severity comments were prioritized as inline comments.
🟡 Minor comments (7)
README.md-29-29 (1)
29-29:⚠️ Potential issue | 🟡 Minor문서 오타만 맞춰 두면 좋겠습니다.
Line 29의
최악이예요는최악이에요가 자연스럽습니다. 구현 쪽 텍스트와도 맞춰 두면 문서 신뢰도가 더 좋아집니다.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@README.md` at line 29, Update the README text "최악이예요" to the correct and consistent phrasing "최악이에요" on line containing that checklist item so it matches the implementation wording and improves document consistency; search for the exact string "최악이예요" and replace it with "최악이에요" wherever it appears in the README.cypress/e2e/clickTest.cy.ts-24-45 (1)
24-45:⚠️ Potential issue | 🟡 Minor시나리오 이름과 선택 인덱스가 맞지 않습니다.
Line 27의
eq(6)은 일곱 번째 항목이고, Line 44의eq(12)는 열세 번째 항목입니다. 설명과 실제 선택 대상이 어긋나 있으면 실패 로그를 읽을 때 혼동이 커지니 둘 중 하나를 맞춰 주세요.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@cypress/e2e/clickTest.cy.ts` around lines 24 - 45, The test names ("다섯 번째" and "열한 번째") do not match the selected indices (eq(6) and eq(12))—update either the scenario descriptions or the eq(...) calls to match: for the first test, change eq(6) to eq(4) or rename the it(...) title to "일곱 번째" to match eq(6); for the second test, change eq(12) to eq(10) or rename the it(...) title to "열세 번째" to match eq(12). Ensure you update the it(...) strings and any related uses of .eq(...) in the same test blocks (the tests containing the .eq(...) calls and the it(...) descriptions) so the human-readable scenario matches the actual selected index.templates/modal.html-41-43 (1)
41-43:⚠️ Potential issue | 🟡 Minor빈
img src는 그대로 두면 안 됩니다.Line 42의
src=""는 브라우저에 따라 현재 문서를 다시 요청해서 불필요한 네트워크 호출이나 콘솔 에러를 만들 수 있습니다. 실제 이미지가 없으면 태그를 빼거나, 값이 있을 때만 렌더링되도록 정리해 주세요.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@templates/modal.html` around lines 41 - 43, The empty img src inside the .search-error-container should be removed or rendered conditionally to avoid needless network requests/console errors; update the template so the <img> element is either deleted from the static markup or only included when a valid image source exists (e.g., guard the rendering with the same condition that provides the image URL) and ensure the remaining markup still provides accessible fallback text via the .search-error-text element (preserve the class names search-error-container and search-error-text when making this change).index.html-29-30 (1)
29-30:⚠️ Potential issue | 🟡 Minor
img의 빈src를 사용하지 않도록 수정해주세요.두 위치 모두
src=""상태라 HTML 유효성 오류가 발생합니다. 초기 렌더 시 이미지가 필요 없다면 조건부 렌더링으로 분리하는 편이 안전합니다.Also applies to: 54-54
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@index.html` around lines 29 - 30, The empty img elements inside the search-error-container (and the other occurrence) must not have src=""—remove the empty src attribute or replace it with a valid placeholder or a data-src, and render the <img> only when an actual URL exists (e.g., conditionally render based on image availability or set the element hidden until you assign a real src). Locate the <div class="search-error-container"> and the second identical img occurrence and update those img tags to avoid an empty src, using conditional rendering or a non-empty valid src to fix HTML validity.templates/index.html-42-44 (1)
42-44:⚠️ Potential issue | 🟡 Minor빈
img src를 제거하거나 유효한 값으로 초기화해주세요.
src=""는 HTML 유효성 문제를 만들고 불필요한 요청을 유발할 수 있습니다. 초기 표시가 필요 없다면<img>자체를 렌더링하지 않거나, 기본 플레이스홀더 경로를 지정해 주세요.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@templates/index.html` around lines 42 - 44, The empty img element inside the search-error block (the element with class "search-error-container") uses src="" which is invalid and can trigger unnecessary network requests; either remove the <img> entirely from the "search-error-container" markup or initialize its src to a valid placeholder image path and set an appropriate alt, or render the <img> conditionally only when an actual image URL is available (keeping the accompanying element with class "search-error-text" intact).src/view/movieListView.ts-31-31 (1)
31-31:⚠️ Potential issue | 🟡 Minor반복문 내 중복
id속성 문제
id="title"이forEach루프 안에서 사용되어 DOM에 동일한 ID가 여러 개 생성됩니다. HTML 명세상 ID는 문서 내에서 고유해야 합니다.ID 대신 어떤 선택자를 사용하면 좋을까요?
💡 class로 변경
- <strong id="title">${movie.title}</strong> + <strong class="title">${movie.title}</strong>🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/view/movieListView.ts` at line 31, The template in movieListView.ts uses a non-unique id on each item (<strong id="title">), which creates duplicate IDs; change the id to a class or data-attribute (e.g., class="title" or data-title) inside the rendering loop where <strong id="title">${movie.title}</strong> appears, and update any downstream DOM access (e.g., calls using getElementById('title')) to use document.querySelectorAll('.title') or querySelectorAll('[data-title]') and iterate/select by index or dataset accordingly. Ensure all references to the old id in event handlers or tests are updated to the new selector.src/events/bindMovieEvent.ts-86-98 (1)
86-98:⚠️ Potential issue | 🟡 Minor비동기 함수 호출 시
await누락
displayMovieBySearch는 Promise를 반환하는 async 함수인데, 이벤트 핸들러에서await없이 호출하고 있습니다.에러가 발생하면 어떻게 될까요? unhandled promise rejection으로 콘솔에만 에러가 표시되고 사용자에게는 피드백이 없을 수 있습니다.
💡 await 추가
searchBar.addEventListener('keydown', async (event) => { if (event.isComposing) return if (event.key === 'Enter') { - displayMovieBySearch(movieDisplay, state) + await displayMovieBySearch(movieDisplay, state) } }) // 검색 버튼 '클릭' const searchBtn = document.querySelector('.search-btn') searchBtn?.addEventListener('click', async () => { - displayMovieBySearch(movieDisplay, state) + await displayMovieBySearch(movieDisplay, state) })🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/events/bindMovieEvent.ts` around lines 86 - 98, The event handlers calling the async function displayMovieBySearch currently omit awaiting its Promise; update both the keydown handler (where event.key === 'Enter') and the click handler attached to '.search-btn' to await displayMovieBySearch and wrap the await in a try/catch so any errors are handled (log via process/UI logger and provide user feedback) instead of causing unhandled promise rejections; reference displayMovieBySearch in your changes and ensure the handlers are marked async if not already.
🧹 Nitpick comments (13)
cypress/fixtures/infoModal.json (1)
1-76: 모달 fixture도 핵심 검증 필드 중심으로 슬림화하면 유지보수가 쉬워집니다.Line 1-76에서 실제 assertion에 쓰지 않는 필드가 많다면,
테스트 목적(제목/개봉일/평점/런타임 등) 기준으로 최소화해서 실패 원인을 더 빠르게 좁혀보는 방향을 추천드립니다.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@cypress/fixtures/infoModal.json` around lines 1 - 76, The infoModal.json fixture contains many unused fields; reduce it to only the properties your tests assert (e.g., "title", "release_date", "vote_average", "runtime", and any UI-relevant keys like "poster_path" or "overview" if referenced) so the fixture is minimal and easier to maintain—open the fixture (infoModal.json) and remove unrelated sections such as "belongs_to_collection", "production_companies", "production_countries", "spoken_languages", "budget", "revenue", etc., leaving only the keys used by the tests' assertions.src/view/getElementView.ts (1)
1-17: DOM 조회 유틸의 중복을 공통화해보면 좋겠습니다.Line 1-17은 “조회 → 타입검사 → 에러” 흐름이 동일합니다.
지금 구조를 유지하되,selector와 기대 타입만 바꿔 주입하는 공통 헬퍼로 묶을 수 있을지 먼저 점검해보실래요?
그렇게 하면 새 엘리먼트 타입이 늘어날 때 함수 복제가 줄고 에러 메시지 포맷도 일관되게 유지됩니다.As per coding guidelines "Promote modular thinking—breaking problems into reusable components."
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/view/getElementView.ts` around lines 1 - 17, The three functions getElement, getUListElement, and getInputElement duplicate the same "query → type check → throw" flow; create a single generic helper (e.g., queryElementOrThrow or getElementOfType) that accepts the selector and a type guard or expected class (HTMLInputElement/HTMLUListElement/HTMLElement) and centralizes the error message/throw logic, then refactor getElement/getUListElement/getInputElement to call this helper with the appropriate type guard to preserve behavior and unify error formatting.cypress/fixtures/searchMovies.json (1)
1-229: 검색 목킹 데이터는 “테스트에 필요한 필드만” 남기는 방향을 고려해보세요.Line 1-229 fixture가 상당히 커서, 테스트 의도와 무관한 필드 변경에도 테스트 유지보수 비용이 커질 수 있습니다.
어떤 단언이 실제로id/title/poster_path등 최소 필드만 쓰는지 먼저 확인해보고, 필요한 필드만 남겨 fixture를 가볍게 가져가면 좋겠습니다.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@cypress/fixtures/searchMovies.json` around lines 1 - 229, The fixture searchMovies.json contains many unused fields; reduce it to the minimal shape your tests rely on (keep the top-level "page" and for each item only the properties actually asserted, e.g., "id", "title", "poster_path" and any other used fields like "release_date"), verify which assertions reference fields in tests (search for usages of this fixture in your Cypress specs) and remove all other keys (backdrop_path, overview, popularity, vote_*, genre_ids, etc.) so the fixture is compact and less brittle.cypress/e2e/moreBtnTest.cy.ts (1)
49-71: 주석 처리된 예전 시나리오는 정리하는 편이 좋겠습니다.현재 구현은 무한 스크롤인데, 파일 하단에는 예전 "더 보기 버튼" 테스트가 통째로 남아 있습니다. 이런 블록은 나중에 실패 원인이나 현재 요구사항을 읽을 때 노이즈가 되기 쉬우니, 정말 참고가 필요하면 문서로 옮기고 테스트 파일에서는 제거하는 쪽이 깔끔합니다.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@cypress/e2e/moreBtnTest.cy.ts` around lines 49 - 71, Remove the large commented-out legacy "more btn test" block in the Cypress spec (the describe block labeled 'more btn test' containing tests that reference selectors '.display-more-btn' and '.item'), since the app now uses infinite scroll; if you want to keep the old scenarios for reference, move them to documentation or a separate migration note file and delete the commented code from the test file to reduce noise.templates/modal.html (1)
16-18: 검색 입력창에 명시적인 접근성 이름이 필요합니다.Line 17은 placeholder만 있고
label이나aria-label이 없습니다. 이 상태에서는 스크린리더 사용자가 입력창의 용도를 안정적으로 파악하기 어려우니, 검색 필드라는 이름을 명시해 두는 편이 좋겠습니다.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@templates/modal.html` around lines 16 - 18, The input with class "search-bar" lacks an accessible name; add a proper label or aria attribute so screen readers can identify it. Give the input an id (e.g., id="search-input") and either add a visually associated <label> with the Korean text "검색어" linked via for="search-input" (use a visually-hidden/sr-only style if you don't want it visible) or add aria-label="검색어" / aria-labelledby pointing to a visible label; update any related code that queries "search-bar" if it uses the id instead.templates/styles/modal.css (1)
123-140:.modal-background중복 선언을 축소하는 리팩터링을 권장합니다.기본 스타일과 태블릿/모바일 미디어쿼리에 동일한 속성이 반복되어 유지보수 비용이 큽니다. 공통 속성은 기본 블록으로 올리고, 브레이크포인트에서는 변경되는 속성(예:
align-items)만 남기는 구조가 더 안전합니다.Also applies to: 200-217
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@templates/styles/modal.css` around lines 123 - 140, The .modal-background rules are duplicated across base and media-query blocks; refactor by extracting shared properties into the primary .modal-background selector and leaving only the breakpoint-specific overrides (e.g., align-items) inside the tablet/mobile media queries—update the selectors referenced as .modal-background in templates/styles/modal.css so common props (position, top, left, width, height, background-color, backdrop-filter, display, justify-content, z-index, visibility, opacity, transition) live only once and media queries modify only the differing properties.templates/styles/main.css (1)
277-330: 동일한 모바일 미디어쿼리 블록이 중복되어 있습니다.완전히 같은 규칙이 두 번 선언되어 있어 수정 시 누락 위험이 큽니다. 하나로 합쳐 단일 소스로 관리하는 것을 권장합니다.
Also applies to: 333-386
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@templates/styles/main.css` around lines 277 - 330, There are duplicate mobile media-query blocks starting with the `@media` only screen and (480px <= width <= 800px) rule; locate both media-query blocks and merge them into a single block so each selector (body, nav, nav img, .search-container, .top-rated-container, .top-rated-movie, .top-rated-movie .title, .page-title, footer) appears only once, preserving all unique declarations and consolidating any overlapping properties (prefer the more specific/updated values), then remove the redundant block to ensure a single source of truth.src/events/bindMovieEvent.ts (5)
132-142:let대신const사용 권장
renderMovieList와 동일하게movieList를const로 선언할 수 있습니다.♻️ 개선 제안
export const movieViewFlow = async (state: State, movieDisplay: HTMLUListElement) => { state.pageNum++ addMovieSkeletonUIList(movieDisplay) - let movieList - movieList = await callMovieList(state.pageNum, state.searchBarText) + const movieList = await callMovieList(state.pageNum, state.searchBarText) removeMovieSkeletonUIList(movieDisplay) addMovieList(movieDisplay, movieList) }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/events/bindMovieEvent.ts` around lines 132 - 142, The variable movieList inside the movieViewFlow function is never reassigned after assignment so replace its mutable declaration with an immutable one: change let movieList to const movieList where movieList is assigned from await callMovieList(state.pageNum, state.searchBarText); this is in movieViewFlow alongside calls to addMovieSkeletonUIList, removeMovieSkeletonUIList and addMovieList.
117-129: 주석 처리된 코드 제거 권장더 이상 사용하지 않는 코드는 버전 관리 시스템에서 추적 가능하므로 제거하는 것이 좋습니다.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/events/bindMovieEvent.ts` around lines 117 - 129, Remove the dead commented block that handles the "display more" button (the lines referencing displayMoreBtn, addMovieSkeletonUIList, callMovieList, removeMovieSkeletonUIList, addMovieList and state.pageNum/state.searchBarText with movieDisplay) since it's no longer used; delete the entire commented section so the file doesn't contain obsolete code and rely on VCS history if it needs restoring.
27-37:let대신const사용 권장
movieList변수가 재할당되지 않으므로const로 선언하는 것이 더 적절합니다.♻️ 개선 제안
const renderMovieList = async (movieDisplay: HTMLUListElement, state: State): Promise<Movie[]> => { - let movieList movieDisplay.replaceChildren() addMovieSkeletonUIList(movieDisplay) - movieList = await callMovieList(state.pageNum, state.searchBarText) + const movieList = await callMovieList(state.pageNum, state.searchBarText) removeMovieSkeletonUIList(movieDisplay) return movieList }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/events/bindMovieEvent.ts` around lines 27 - 37, In renderMovieList, movieList is never reassigned so change its declaration from "let movieList" to "const movieList" (assigning the awaited call directly, e.g. const movieList = await callMovieList(...)) to use an immutable binding; update the declaration in the renderMovieList function accordingly.
182-203:closest()반환값 null 체크 개선Line 184에서
closest('li')가 null을 반환할 수 있는데as HTMLElement로 강제 캐스팅하고 있습니다. Line 185의 옵셔널 체이닝이 있지만, 타입 단언을 피하는 것이 더 안전합니다.♻️ 개선 제안
thumbnailBox.addEventListener('click', async (event: MouseEvent) => { const target = event.target as HTMLElement - const item = target.closest('li') as HTMLElement - if (!item?.dataset.id) return + const item = target.closest('li') + if (!item || !item.dataset.id) return🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/events/bindMovieEvent.ts` around lines 182 - 203, The click handler attached in thumbnailBox.addEventListener uses target.closest('li') and force-casts it to HTMLElement; instead, stop using a blanket type assertion and properly null-check and type-guard the result of target.closest — e.g., get the result as const item = target.closest('li') or const maybeItem = target.closest('li') as HTMLElement | null and immediately return if it's null or not an HTMLElement before accessing item.dataset.id and calling fetchMovieDetail, so that the handler safely exits when closest() returns null and no runtime errors occur.
95-98: 요소 쿼리 방식 불일치다른 곳에서는
getElement,getInputElement헬퍼를 사용하는데, 여기서는document.querySelector를 직접 사용하고 있습니다.왜 다르게 처리했는지, 그리고
searchBtn이 null일 때의 동작이 의도된 것인지 확인해보세요.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/events/bindMovieEvent.ts` around lines 95 - 98, The event binding uses document.querySelector directly for searchBtn instead of the project's element helpers, creating an inconsistency and potential null-handling mismatch; replace the direct query with the helper (e.g., getElement or getInputElement) to obtain the search button, ensure the helper's return is handled (throw or assert if null) or guard before adding the listener, and keep the listener calling displayMovieBySearch(movieDisplay, state) unchanged so behavior remains consistent with other bindings.src/view/movieListView.ts (1)
59-75:backdrop_path와title에 대한 동일한 보안 고려 필요
addMovieList와 동일하게movie.backdrop_path와movie.title이 innerHTML에 직접 삽입되고 있습니다. 일관성 있게 처리하는 것이 좋습니다.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/view/movieListView.ts` around lines 59 - 75, The showBackgroundMovieInfo function directly injects movie.backdrop_path and movie.title into background.innerHTML causing the same XSS risk as noted; replace the innerHTML construction with DOM creation and safe assignment: create elements for the overlay IMG, top-rated container, rate, title etc., set img.src using a validated/whitelisted or sanitized backdrop path (or prepend the fixed base URL and validate that backdrop_path contains only safe characters), set text nodes via element.textContent for movie.title and rate value, and append the elements to the element returned by getElement('.background-container') instead of using background.innerHTML to ensure safe rendering.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In @.env:
- Line 1: The .env file contains a real secret value
(VITE_API_KEY=293805c00692398c046c66e6b7b51d4d) and must be remediated:
immediately revoke/rotate the exposed API key with the provider, remove the
secret from the repository by replacing the value in .env with a non-secret
placeholder (e.g., VITE_API_KEY=REPLACE_ME), add .env to .gitignore, commit a
safe .env.example that documents the placeholder and required vars, and consider
purging the secret from git history (git filter-repo / BFG) or opening a
follow-up task to do so; ensure any deployment/CI secrets are updated to use the
new rotated key.
In `@cypress/e2e/clickTest.cy.ts`:
- Line 8: The test is visiting a hardcoded deployment URL via cy.visit(...) in
clickTest.cy.ts which can cause tests to target different deployments; replace
those hardcoded visits (the cy.visit calls) to use the configured test target
instead — e.g. call cy.visit('/') or build the URL from
Cypress.config('baseUrl') / Cypress.env('BASE_URL') so all specs use the shared
baseUrl from cypress config rather than an explicit Vercel URL.
In `@cypress/e2e/modalTest.cy.ts`:
- Line 9: Tests currently hardcode the deployed Vercel URL
(cy.visit('https://...')) so PR/CI/local runs always hit production; change all
Cypress test files (including modalTest.cy.ts) to use relative routes (e.g.,
cy.visit('/') or cy.visit('/path')) instead of the full URL, and set a global
baseUrl in your Cypress config (cypress.config.ts) wired to an environment
variable (e.g., process.env.CYPRESS_BASE_URL or CYPRESS_ENV) so CI, local, and
deployment verification can target different hosts; update any cy.visit(...)
occurrences across the five test files and ensure the CI pipeline sets the
proper env var for the desired target.
In `@cypress/e2e/searchTest.cy.ts`:
- Around line 54-60: The alert listener is registered after cy.visit(), so it
can miss alerts fired during page load; move the cy.on('window:alert', ...) call
to before cy.visit(...) (and keep cy.intercept('GET', '**/movie/popular**', {
forceNetworkError: true }) before visit as well) so that the window:alert
handler is attached prior to navigation; apply the same change for the other
test block that uses cy.on('window:alert') around the failing network intercept.
In `@index.html`:
- Around line 49-50: 모달 닫기 버튼(<button> 요소, id="closeModal",
class="close-modal")에 시멘틱 접근성 레이블이 부족합니다; 버튼에 aria-label="닫기" 속성을 추가하여 스크린리더가 버튼
목적을 명확히 알 수 있도록 수정하세요 (이미지 대신 접근성 이름을 제공하되 시각적 디자인은 변경하지 않도록 aria-label만 추가).
- Around line 18-19: The search input and button lack accessible labels; add
descriptive accessibility labels for screen readers by adding either a visible
<label> tied to the input's id or ARIA attributes: give the input with class
"search-bar" an id and a corresponding <label> or an aria-label (e.g., "검색어
입력"), and add an appropriate aria-label (e.g., "검색" or "검색 버튼") to the button
with class "search-btn" so assistive technologies can announce their purpose.
In `@src/events/bindMovieEvent.ts`:
- Around line 101-115: The bindMoreMovieEvents implementation must guard against
null targets and allow the observer to be reset after renderMovieList/
displayMovieBySearch replaces DOM: change bindMoreMovieEvents to only create and
start observing when firstTarget (querySelector('.thumbnail-list >
li:last-child')) is non-null, and inside the IntersectionObserver callback check
that entry.target and the newly queried target are non-null before calling
observer.observe/unobserve; additionally expose a way to reset or disconnect the
observer (either return the observer from bindMoreMovieEvents or store it on the
shared State) and ensure renderMovieList or displayMovieBySearch calls
observer.disconnect() (or the returned observer is re-created by calling
bindMoreMovieEvents again) so the observer is not left observing removed nodes
and is re-initialized for the new DOM; refer to bindMoreMovieEvents,
movieViewFlow, getUListElement, renderMovieList and displayMovieBySearch when
making these changes.
In `@src/main.ts`:
- Around line 27-35: The initialization assumes movieList[0] and always
registers infinite-scroll observers, which breaks when the first response is an
empty array; change the startup flow in the block around
callMovieList/removeMovieSkeletonUIList/addMovieList so you first check if
movieList.length > 0 before calling showBackgroundMovieInfo(movieList[0]) and
before calling bindMoreMovieEvents(state), otherwise skip those calls (but still
call bindSearchEvents and render the empty list). Alternatively ensure
bindMoreMovieEvents or the observer in bindMovieEvent gracefully no-ops when the
observed element is null — update the code paths involving
showBackgroundMovieInfo and bindMoreMovieEvents to guard against undefined/null
movieList[0] and null observers.
- Around line 18-20: 현재 전역 state 객체(state)의 pageNum과 searchBarText를
bindSearchEvents()와 bindMoreMovieEvents()가 비동기 요청 도중 즉시 변경해 경쟁 상태를 만들고 있으니, 각
이벤트 핸들러에서 비동기 요청 전에는 로컬 복사본(localPage, localQuery)을 사용하거나 isLoading 같은 플래그로 동시
실행을 막고, 요청이 성공적으로 완료된 후에만 전역 state.pageNum과 state.searchBarText를 갱신하도록 바꾸세요;
구체적으로 bindSearchEvents()는 검색 시작 시 localQuery와 localPage(=1)로 요청을 보내고 응답 후 state를
덮어쓰기, bindMoreMovieEvents()는 더보기 클릭 시 현재 state 값을 바로 증가시키지 말고 요청 성공 시에만
state.pageNum을 증가시키거나 두 핸들러가 동시에 갱신하지 않도록 단일 업데이트 경로(락 또는 큐)를 도입하세요.
In `@src/service/movieApi.ts`:
- Around line 31-35: The fetchSearchMovieList function is double-encoding the
search string by calling encodeURIComponent before passing it into request which
likely uses URLSearchParams; remove encodeURIComponent and pass the raw
searchBarText as the query parameter so URLSearchParams can perform encoding
itself (update the call in fetchSearchMovieList that constructs the params for
SEARCH_PATH to use searchBarText directly).
In `@src/view/movieListView.ts`:
- Around line 19-25: The img alt attribute in the li.innerHTML interpolation is
missing quotes and directly injects movie.title, which breaks attributes with
spaces and opens an XSS vector; update the code to stop injecting raw HTML by
creating the <img> element via DOM methods (e.g.,
document.createElement('img')), set its src from movie.poster_path and set its
alt via the element's alt property (img.alt = sanitizedTitle) or assign text via
textContent, and ensure movie.title is sanitized/escaped (use a sanitizer like
DOMPurify or a safe-encoding function) before assigning; change references:
li.innerHTML -> createElement path, movie.title -> sanitizedTitle, and
poster_path -> assigned to img.src.
- Around line 3-12: The Movie interface currently conflates two different API
shapes causing type mismatch for list endpoints; update types and usage by
creating two distinct interfaces (e.g., MovieDetail with genres:
{id:number;name:string}[] used by fetchMovieDetail, and MovieListItem with
genre_ids: number[] used by fetchDefaultMovieList and fetchSearchMovieList) or
make Movie.genres optional and explicitly map genre_ids to genres where needed
(e.g., in fetchDefaultMovieList/fetchSearchMovieList convert each item’s
genre_ids into a safe genres array or keep genre_ids on the list type); then
update function signatures (fetchMovieDetail, fetchDefaultMovieList,
fetchSearchMovieList) and any consumers to use the correct interface so the type
matches actual API responses.
In `@templates/index.html`:
- Around line 17-18: The search input and button lack accessible names; update
the elements with accessible labels by adding either a <label> tied to the
input's id or appropriate aria attributes: give the input with class
"search-bar" a unique id and a visible or screen-reader-only <label for="...">
or an aria-label (e.g., aria-label="검색어 입력"), and add an accessible name to the
button with class "search-btn" (e.g., aria-label="검색" or aria-labelledby
pointing to the label). Ensure the id/for pairing (or aria-labelledby) matches
exactly so assistive tech can associate the controls.
In `@templates/modal.html`:
- Around line 47-49: The "더 보기" button in templates/modal.html is dead UI
because src/events/bindMovieEvent.ts only sets up an IntersectionObserver (lines
~102-115) and no click handler exists for .display-more-btn; either remove the
button markup to rely solely on infinite scroll, or add a click listener for the
.display-more-btn that calls the same load/more-fetch function used by the
IntersectionObserver (or invokes the same handler wrapper in bindMovieEvent.ts)
so clicking triggers the identical loading flow; update any related DOM queries
in bindMovieEvent.ts to select .display-more-btn and wire the click to the
existing loadMore/handleIntersection function.
In `@templates/styles/main.css`:
- Around line 249-261: Remove the vendor-prefixed at-rule `@-webkit-keyframes` for
the skeleton-gradient animation and keep only the standard `@keyframes`
skeleton-gradient (or rely on your build/autoprefixer to add vendor prefixes);
update any references to the animation name if needed so they point to the
standard `@keyframes` and delete the `@-webkit-keyframes` block to satisfy the
at-rule-no-vendor-prefix stylelint rule.
In `@templates/styles/thumbnail.css`:
- Around line 58-63: 현재 `@media` only screen and (max-width: 800px) 블록에서
.thumbnail-list에 grid-template-columns: repeat(3, 200px)를 지정해 480px 미만에서 가로
오버플로우가 발생하므로, 480px 미만 전용 규칙을 별도로 추가하거나 덮어써서 모바일 레이아웃을 조정하세요; 예를 들어 `@media`
(max-width: 480px)를 추가하여 .thumbnail-list의 grid-template-columns를 단일 열(예: 1fr 또는
repeat(1, 100%))이나 더 작은 칼럼 수(예: repeat(2, 1fr))로 재정의하고 이 규칙을 기존 `@media`
(max-width: 800px) 블록보다 뒤에 위치시켜 우선 적용되도록 하며, 동일한 수정은 파일 내 다른 유사
블록(.thumbnail-list 규칙이 반복된 66-71 범위)에도 적용하세요.
---
Minor comments:
In `@cypress/e2e/clickTest.cy.ts`:
- Around line 24-45: The test names ("다섯 번째" and "열한 번째") do not match the
selected indices (eq(6) and eq(12))—update either the scenario descriptions or
the eq(...) calls to match: for the first test, change eq(6) to eq(4) or rename
the it(...) title to "일곱 번째" to match eq(6); for the second test, change eq(12)
to eq(10) or rename the it(...) title to "열세 번째" to match eq(12). Ensure you
update the it(...) strings and any related uses of .eq(...) in the same test
blocks (the tests containing the .eq(...) calls and the it(...) descriptions) so
the human-readable scenario matches the actual selected index.
In `@index.html`:
- Around line 29-30: The empty img elements inside the search-error-container
(and the other occurrence) must not have src=""—remove the empty src attribute
or replace it with a valid placeholder or a data-src, and render the <img> only
when an actual URL exists (e.g., conditionally render based on image
availability or set the element hidden until you assign a real src). Locate the
<div class="search-error-container"> and the second identical img occurrence and
update those img tags to avoid an empty src, using conditional rendering or a
non-empty valid src to fix HTML validity.
In `@README.md`:
- Line 29: Update the README text "최악이예요" to the correct and consistent phrasing
"최악이에요" on line containing that checklist item so it matches the implementation
wording and improves document consistency; search for the exact string "최악이예요"
and replace it with "최악이에요" wherever it appears in the README.
In `@src/events/bindMovieEvent.ts`:
- Around line 86-98: The event handlers calling the async function
displayMovieBySearch currently omit awaiting its Promise; update both the
keydown handler (where event.key === 'Enter') and the click handler attached to
'.search-btn' to await displayMovieBySearch and wrap the await in a try/catch so
any errors are handled (log via process/UI logger and provide user feedback)
instead of causing unhandled promise rejections; reference displayMovieBySearch
in your changes and ensure the handlers are marked async if not already.
In `@src/view/movieListView.ts`:
- Line 31: The template in movieListView.ts uses a non-unique id on each item
(<strong id="title">), which creates duplicate IDs; change the id to a class or
data-attribute (e.g., class="title" or data-title) inside the rendering loop
where <strong id="title">${movie.title}</strong> appears, and update any
downstream DOM access (e.g., calls using getElementById('title')) to use
document.querySelectorAll('.title') or querySelectorAll('[data-title]') and
iterate/select by index or dataset accordingly. Ensure all references to the old
id in event handlers or tests are updated to the new selector.
In `@templates/index.html`:
- Around line 42-44: The empty img element inside the search-error block (the
element with class "search-error-container") uses src="" which is invalid and
can trigger unnecessary network requests; either remove the <img> entirely from
the "search-error-container" markup or initialize its src to a valid placeholder
image path and set an appropriate alt, or render the <img> conditionally only
when an actual image URL is available (keeping the accompanying element with
class "search-error-text" intact).
In `@templates/modal.html`:
- Around line 41-43: The empty img src inside the .search-error-container should
be removed or rendered conditionally to avoid needless network requests/console
errors; update the template so the <img> element is either deleted from the
static markup or only included when a valid image source exists (e.g., guard the
rendering with the same condition that provides the image URL) and ensure the
remaining markup still provides accessible fallback text via the
.search-error-text element (preserve the class names search-error-container and
search-error-text when making this change).
---
Nitpick comments:
In `@cypress/e2e/moreBtnTest.cy.ts`:
- Around line 49-71: Remove the large commented-out legacy "more btn test" block
in the Cypress spec (the describe block labeled 'more btn test' containing tests
that reference selectors '.display-more-btn' and '.item'), since the app now
uses infinite scroll; if you want to keep the old scenarios for reference, move
them to documentation or a separate migration note file and delete the commented
code from the test file to reduce noise.
In `@cypress/fixtures/infoModal.json`:
- Around line 1-76: The infoModal.json fixture contains many unused fields;
reduce it to only the properties your tests assert (e.g., "title",
"release_date", "vote_average", "runtime", and any UI-relevant keys like
"poster_path" or "overview" if referenced) so the fixture is minimal and easier
to maintain—open the fixture (infoModal.json) and remove unrelated sections such
as "belongs_to_collection", "production_companies", "production_countries",
"spoken_languages", "budget", "revenue", etc., leaving only the keys used by the
tests' assertions.
In `@cypress/fixtures/searchMovies.json`:
- Around line 1-229: The fixture searchMovies.json contains many unused fields;
reduce it to the minimal shape your tests rely on (keep the top-level "page" and
for each item only the properties actually asserted, e.g., "id", "title",
"poster_path" and any other used fields like "release_date"), verify which
assertions reference fields in tests (search for usages of this fixture in your
Cypress specs) and remove all other keys (backdrop_path, overview, popularity,
vote_*, genre_ids, etc.) so the fixture is compact and less brittle.
In `@src/events/bindMovieEvent.ts`:
- Around line 132-142: The variable movieList inside the movieViewFlow function
is never reassigned after assignment so replace its mutable declaration with an
immutable one: change let movieList to const movieList where movieList is
assigned from await callMovieList(state.pageNum, state.searchBarText); this is
in movieViewFlow alongside calls to addMovieSkeletonUIList,
removeMovieSkeletonUIList and addMovieList.
- Around line 117-129: Remove the dead commented block that handles the "display
more" button (the lines referencing displayMoreBtn, addMovieSkeletonUIList,
callMovieList, removeMovieSkeletonUIList, addMovieList and
state.pageNum/state.searchBarText with movieDisplay) since it's no longer used;
delete the entire commented section so the file doesn't contain obsolete code
and rely on VCS history if it needs restoring.
- Around line 27-37: In renderMovieList, movieList is never reassigned so change
its declaration from "let movieList" to "const movieList" (assigning the awaited
call directly, e.g. const movieList = await callMovieList(...)) to use an
immutable binding; update the declaration in the renderMovieList function
accordingly.
- Around line 182-203: The click handler attached in
thumbnailBox.addEventListener uses target.closest('li') and force-casts it to
HTMLElement; instead, stop using a blanket type assertion and properly
null-check and type-guard the result of target.closest — e.g., get the result as
const item = target.closest('li') or const maybeItem = target.closest('li') as
HTMLElement | null and immediately return if it's null or not an HTMLElement
before accessing item.dataset.id and calling fetchMovieDetail, so that the
handler safely exits when closest() returns null and no runtime errors occur.
- Around line 95-98: The event binding uses document.querySelector directly for
searchBtn instead of the project's element helpers, creating an inconsistency
and potential null-handling mismatch; replace the direct query with the helper
(e.g., getElement or getInputElement) to obtain the search button, ensure the
helper's return is handled (throw or assert if null) or guard before adding the
listener, and keep the listener calling displayMovieBySearch(movieDisplay,
state) unchanged so behavior remains consistent with other bindings.
In `@src/view/getElementView.ts`:
- Around line 1-17: The three functions getElement, getUListElement, and
getInputElement duplicate the same "query → type check → throw" flow; create a
single generic helper (e.g., queryElementOrThrow or getElementOfType) that
accepts the selector and a type guard or expected class
(HTMLInputElement/HTMLUListElement/HTMLElement) and centralizes the error
message/throw logic, then refactor getElement/getUListElement/getInputElement to
call this helper with the appropriate type guard to preserve behavior and unify
error formatting.
In `@src/view/movieListView.ts`:
- Around line 59-75: The showBackgroundMovieInfo function directly injects
movie.backdrop_path and movie.title into background.innerHTML causing the same
XSS risk as noted; replace the innerHTML construction with DOM creation and safe
assignment: create elements for the overlay IMG, top-rated container, rate,
title etc., set img.src using a validated/whitelisted or sanitized backdrop path
(or prepend the fixed base URL and validate that backdrop_path contains only
safe characters), set text nodes via element.textContent for movie.title and
rate value, and append the elements to the element returned by
getElement('.background-container') instead of using background.innerHTML to
ensure safe rendering.
In `@templates/modal.html`:
- Around line 16-18: The input with class "search-bar" lacks an accessible name;
add a proper label or aria attribute so screen readers can identify it. Give the
input an id (e.g., id="search-input") and either add a visually associated
<label> with the Korean text "검색어" linked via for="search-input" (use a
visually-hidden/sr-only style if you don't want it visible) or add
aria-label="검색어" / aria-labelledby pointing to a visible label; update any
related code that queries "search-bar" if it uses the id instead.
In `@templates/styles/main.css`:
- Around line 277-330: There are duplicate mobile media-query blocks starting
with the `@media` only screen and (480px <= width <= 800px) rule; locate both
media-query blocks and merge them into a single block so each selector (body,
nav, nav img, .search-container, .top-rated-container, .top-rated-movie,
.top-rated-movie .title, .page-title, footer) appears only once, preserving all
unique declarations and consolidating any overlapping properties (prefer the
more specific/updated values), then remove the redundant block to ensure a
single source of truth.
In `@templates/styles/modal.css`:
- Around line 123-140: The .modal-background rules are duplicated across base
and media-query blocks; refactor by extracting shared properties into the
primary .modal-background selector and leaving only the breakpoint-specific
overrides (e.g., align-items) inside the tablet/mobile media queries—update the
selectors referenced as .modal-background in templates/styles/modal.css so
common props (position, top, left, width, height, background-color,
backdrop-filter, display, justify-content, z-index, visibility, opacity,
transition) live only once and media queries modify only the differing
properties.
🪄 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: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 287a70d0-b3c1-4807-887d-942a76741b2e
📒 Files selected for processing (27)
.env.prettierrcREADME.mdcypress/e2e/clickTest.cy.tscypress/e2e/modalTest.cy.tscypress/e2e/moreBtnTest.cy.tscypress/e2e/searchTest.cy.tscypress/e2e/spec.cy.tscypress/e2e/starRatingTest.cy.tscypress/fixtures/infoModal.jsoncypress/fixtures/popularMovies.jsoncypress/fixtures/searchInfoModal.jsoncypress/fixtures/searchMovies.jsoncypress/tsconfig.jsonindex.htmlsrc/events/bindMovieEvent.tssrc/main.tssrc/service/movieApi.tssrc/view/getElementView.tssrc/view/movieListView.tssrc/vite-env.d.tstemplates/index.htmltemplates/modal.htmltemplates/styles/index.csstemplates/styles/main.csstemplates/styles/modal.csstemplates/styles/thumbnail.css
💤 Files with no reviewable changes (1)
- cypress/e2e/spec.cy.ts
| @@ -0,0 +1 @@ | |||
| VITE_API_KEY=293805c00692398c046c66e6b7b51d4d No newline at end of file | |||
There was a problem hiding this comment.
실제 API 키가 저장소에 노출되어 즉시 조치가 필요합니다.
Line 1에 비밀값이 하드코딩되어 커밋되었습니다. 키 유출 상태이므로 즉시 키 폐기(rotate) 후 재발급하고, 저장소에는 placeholder만 남기는 방식으로 바꿔주세요.
보안 조치 예시 (placeholder로 교체)
-VITE_API_KEY=293805c00692398c046c66e6b7b51d4d
+VITE_API_KEY=YOUR_TMDB_API_KEY원하시면 .env.example 기준으로 팀 온보딩 흐름(로컬 설정 문서 포함)까지 같이 정리해드릴게요.
📝 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.
| VITE_API_KEY=293805c00692398c046c66e6b7b51d4d | |
| VITE_API_KEY=YOUR_TMDB_API_KEY |
🧰 Tools
🪛 dotenv-linter (4.0.0)
[warning] 1-1: [EndingBlankLine] No blank line at the end of the file
(EndingBlankLine)
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In @.env at line 1, The .env file contains a real secret value
(VITE_API_KEY=293805c00692398c046c66e6b7b51d4d) and must be remediated:
immediately revoke/rotate the exposed API key with the provider, remove the
secret from the repository by replacing the value in .env with a non-secret
placeholder (e.g., VITE_API_KEY=REPLACE_ME), add .env to .gitignore, commit a
safe .env.example that documents the placeholder and required vars, and consider
purging the secret from git history (git filter-repo / BFG) or opening a
follow-up task to do so; ensure any deployment/CI secrets are updated to use the
new rotated key.
cypress/e2e/searchTest.cy.ts
Outdated
| it('네트워크 오류 시 알림을 띄운다', () => { | ||
| cy.intercept('GET', '**/movie/popular**', { forceNetworkError: true }) | ||
| cy.visit('https://javascript-movie-review-dvlk-a6xn5spuo-yun-cics-projects.vercel.app/') | ||
| cy.on('window:alert', (text) => { | ||
| expect(text).to.equal('네트워크 오류가 발생하였습니다.') | ||
| }) | ||
| }) |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# alert 리스너/visit 순서 점검
rg -n -C2 "cy\\.visit\\(|cy\\.on\\('window:alert'" cypress/e2e/searchTest.cy.ts -SRepository: woowacourse/javascript-movie-review
Length of output: 1530
Alert 리스너가 page load 이후에 등록되어 이벤트를 놓칠 수 있습니다.
현재 코드 순서를 살펴보세요: cy.intercept() → cy.visit() → cy.on('window:alert')
페이지가 초기 로드되는 동안 alert가 발생할 수 있는데, 리스너가 그 이후에 등록되면 alert를 감지하지 못합니다.
순서를 어떻게 변경해야 할까요? 이벤트 리스너가 언제 등록되어야 alert 이벤트를 확실히 받을 수 있을지 생각해봐주세요. Cypress 문서의 cy.on() 부분을 참고하면 도움이 될 겁니다.
Also applies to: 62-68
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@cypress/e2e/searchTest.cy.ts` around lines 54 - 60, The alert listener is
registered after cy.visit(), so it can miss alerts fired during page load; move
the cy.on('window:alert', ...) call to before cy.visit(...) (and keep
cy.intercept('GET', '**/movie/popular**', { forceNetworkError: true }) before
visit as well) so that the window:alert handler is attached prior to navigation;
apply the same change for the other test block that uses cy.on('window:alert')
around the failing network intercept.
| <input class="search-bar" placeholder="검색어를 입력하세요" /> | ||
| <button class="search-btn">🔍</button> |
There was a problem hiding this comment.
검색 입력/버튼에 접근성 라벨을 추가해주세요.
placeholder/아이콘만으로는 보조기기에서 의미 전달이 부족합니다. label 또는 aria-label을 명시해 주세요.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@index.html` around lines 18 - 19, The search input and button lack accessible
labels; add descriptive accessibility labels for screen readers by adding either
a visible <label> tied to the input's id or ARIA attributes: give the input with
class "search-bar" an id and a corresponding <label> or an aria-label (e.g.,
"검색어 입력"), and add an appropriate aria-label (e.g., "검색" or "검색 버튼") to the
button with class "search-btn" so assistive technologies can announce their
purpose.
src/view/movieListView.ts
Outdated
| li.innerHTML = /*html*/ ` | ||
| <div class="item" > | ||
| <img | ||
| class="thumbnail" | ||
| src="https://image.tmdb.org/t/p/w500${movie.poster_path}" | ||
| alt=${movie.title} | ||
| /> |
There was a problem hiding this comment.
HTML alt 속성에 따옴표 누락 및 XSS 취약점 고려 필요
두 가지 문제점이 있습니다:
alt=${movie.title}- 따옴표가 없어서 공백이 있는 제목은 alt 속성이 깨집니다movie.title이 악의적인 문자열을 포함할 경우 XSS 공격에 노출될 수 있습니다
이 데이터가 어디서 오는지, 그리고 어떻게 sanitize할 수 있을지 고민해보세요. 외부 API 데이터를 신뢰할 수 있는지가 핵심 질문입니다.
💡 힌트: alt 속성 따옴표 수정
- alt=${movie.title}
+ alt="${movie.title}"XSS 방어를 위해서는 textContent 사용이나 DOMPurify 같은 라이브러리 도입을 검토해보세요.
📝 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.
| li.innerHTML = /*html*/ ` | |
| <div class="item" > | |
| <img | |
| class="thumbnail" | |
| src="https://image.tmdb.org/t/p/w500${movie.poster_path}" | |
| alt=${movie.title} | |
| /> | |
| li.innerHTML = /*html*/ ` | |
| <div class="item" > | |
| <img | |
| class="thumbnail" | |
| src="https://image.tmdb.org/t/p/w500${movie.poster_path}" | |
| alt="${movie.title}" | |
| /> |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/view/movieListView.ts` around lines 19 - 25, The img alt attribute in the
li.innerHTML interpolation is missing quotes and directly injects movie.title,
which breaks attributes with spaces and opens an XSS vector; update the code to
stop injecting raw HTML by creating the <img> element via DOM methods (e.g.,
document.createElement('img')), set its src from movie.poster_path and set its
alt via the element's alt property (img.alt = sanitizedTitle) or assign text via
textContent, and ensure movie.title is sanitized/escaped (use a sanitizer like
DOMPurify or a safe-encoding function) before assigning; change references:
li.innerHTML -> createElement path, movie.title -> sanitizedTitle, and
poster_path -> assigned to img.src.
| <input class="search-bar" placeholder="검색어를 입력하세요" /> | ||
| <button class="search-btn">🔍</button> |
There was a problem hiding this comment.
검색 입력 요소에 접근 가능한 이름을 추가해주세요.
현재 검색창/버튼은 시각적 사용자 중심으로만 구성되어 있어 보조기기 사용자가 용도를 파악하기 어렵습니다. label 또는 aria-label을 지정해 접근성을 보강해 주세요.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@templates/index.html` around lines 17 - 18, The search input and button lack
accessible names; update the elements with accessible labels by adding either a
<label> tied to the input's id or appropriate aria attributes: give the input
with class "search-bar" a unique id and a visible or screen-reader-only <label
for="..."> or an aria-label (e.g., aria-label="검색어 입력"), and add an accessible
name to the button with class "search-btn" (e.g., aria-label="검색" or
aria-labelledby pointing to the label). Ensure the id/for pairing (or
aria-labelledby) matches exactly so assistive tech can associate the controls.
| <div class="btn-container"> | ||
| <button type="button" class="display-more-btn">더 보기</button> | ||
| </div> |
There was a problem hiding this comment.
더 보기 버튼은 지금 기준으로는 죽은 UI입니다.
제공된 src/events/bindMovieEvent.ts:102-115에는 IntersectionObserver만 연결되어 있고 .display-more-btn 클릭 처리 로직은 없습니다. 버튼이 화면에 보이면 사용자는 눌러도 아무 일도 일어나지 않으니, 무한 스크롤만 남길지 아니면 같은 로딩 흐름에 버튼도 연결할지 한쪽으로 정리해야 합니다.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@templates/modal.html` around lines 47 - 49, The "더 보기" button in
templates/modal.html is dead UI because src/events/bindMovieEvent.ts only sets
up an IntersectionObserver (lines ~102-115) and no click handler exists for
.display-more-btn; either remove the button markup to rely solely on infinite
scroll, or add a click listener for the .display-more-btn that calls the same
load/more-fetch function used by the IntersectionObserver (or invokes the same
handler wrapper in bindMovieEvent.ts) so clicking triggers the identical loading
flow; update any related DOM queries in bindMovieEvent.ts to select
.display-more-btn and wire the click to the existing loadMore/handleIntersection
function.
| @-webkit-keyframes skeleton-gradient { | ||
| 0% { | ||
| background-color: rgba(165, 165, 165, 0.1); | ||
| } | ||
|
|
||
| 50% { | ||
| background-color: rgba(165, 165, 165, 0.3); | ||
| } | ||
|
|
||
| 100% { | ||
| background-color: rgba(165, 165, 165, 0.1); | ||
| } | ||
| } |
There was a problem hiding this comment.
Stylelint 오류가 발생하는 vendor prefix at-rule을 제거해주세요.
@-webkit-keyframes는 현재 규칙(at-rule-no-vendor-prefix)에 의해 실패합니다. 표준 @keyframes만 유지하거나 빌드 단계의 자동 prefix 전략으로 일원화해 주세요.
🧰 Tools
🪛 Stylelint (17.6.0)
[error] 249-249: Unexpected vendor-prefixed at-rule "@-webkit-keyframes" (at-rule-no-vendor-prefix)
(at-rule-no-vendor-prefix)
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@templates/styles/main.css` around lines 249 - 261, Remove the vendor-prefixed
at-rule `@-webkit-keyframes` for the skeleton-gradient animation and keep only the
standard `@keyframes` skeleton-gradient (or rely on your build/autoprefixer to add
vendor prefixes); update any references to the animation name if needed so they
point to the standard `@keyframes` and delete the `@-webkit-keyframes` block to
satisfy the at-rule-no-vendor-prefix stylelint rule.
| @media only screen and (max-width: 800px) { | ||
| .thumbnail-list { | ||
| margin: 0 auto 56px; | ||
| display: grid; | ||
| grid-template-columns: repeat(3, 200px); | ||
| } |
There was a problem hiding this comment.
모바일(480px 미만)에서 3열 레이아웃이 강제됩니다.
현재 미디어쿼리 조합 때문에 480px 미만 뷰포트는 repeat(3, 200px)가 적용되어 가로 오버플로우가 발생합니다. 모바일 최소 구간을 별도로 분리하거나 max-width: 800px 규칙과 충돌하지 않게 조건을 재정의해 주세요.
Also applies to: 66-71
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@templates/styles/thumbnail.css` around lines 58 - 63, 현재 `@media` only screen
and (max-width: 800px) 블록에서 .thumbnail-list에 grid-template-columns: repeat(3,
200px)를 지정해 480px 미만에서 가로 오버플로우가 발생하므로, 480px 미만 전용 규칙을 별도로 추가하거나 덮어써서 모바일 레이아웃을
조정하세요; 예를 들어 `@media` (max-width: 480px)를 추가하여 .thumbnail-list의
grid-template-columns를 단일 열(예: 1fr 또는 repeat(1, 100%))이나 더 작은 칼럼 수(예: repeat(2,
1fr))로 재정의하고 이 규칙을 기존 `@media` (max-width: 800px) 블록보다 뒤에 위치시켜 우선 적용되도록 하며, 동일한
수정은 파일 내 다른 유사 블록(.thumbnail-list 규칙이 반복된 66-71 범위)에도 적용하세요.
There was a problem hiding this comment.
안녕하세요 찰리!
고생 많으셨어요! 테스트코드와 로컬스토리지 관련해서 코멘트 남겨 두었으니 한번 살펴봐주시면 감사하겠습니다!
질문에 대해 답변 드릴게요!
1. 기본기에 대한 두려움
코드를 이해했다고 생각했지만, 진짜로 내 것이 된 것인지 확신이 없는 상황! 너무 공감돼요!
저 또한 비슷한 경험이 있었는데요!
사실 한번에 완벽하게 이해하면 좋겠지만 그렇지 않은 경우가 더 많아요. 반대로 이해한 것 같지만 실제로 잘못 이해하고 있었던 경우도 많죠.
그렇기 때문에 반복적으로 다양한 경험을 쌓는 것이 중요한 것 같아요.
영화리뷰 미션을 여러번 만들어보라는 것은 아니고, 계속 비슷한 서비스 또는 기능들을 만들다 보면 자연스레 손에 익고 머리속에 정리되는 순간들이 와요!
시간을 쓰면 성장하는 것은 사실이지만 쓴 만큼 비례해서 성장하진 않더라구요. 저의 경우 계단식으로 방황하다 어느순간 퍼즐이 맞춰진 것처럼 확 이해가 되고 자신감이 붙어 공부하는 시기가 있고, 그 이후 또 성장하는 것 같은 느낌이 들지 않아 방황하는 시기가 반복되었어요.
그렇기에 포기하지 마시고 반복적으로 해보시는 것을 추천드려요.
추가로 저의 경우 잘 이해했는지 확인하기 위해 누군가에게 제가 알고 있는 개념과 코드 구조를 설명하는 방식을 많이 활용했던 것 같아요. 말할 대상이 없다면 문서화 하고, 문서만 읽고도 모두 이해할 수 있게 하거나요.
2. 구멍이 쌓일 것 같다는 두려움
이 부분 또한 너무 자연스러운 고민이고 1번에 대한 답변과 비슷하다고 생각해요.
사람마다 본인에게 맞는 학습법이 있고 지금의 성장 과정에는 개념을 알고 익히는 것도 있지만 내게 알맞는 학습방법을 찾아가는 과정도 포함되어있다고 생각해요.
저는 주변 사람들과 비교하며 성장하는 것을 큰 동기부여 요소로 삼고 도움을 많이 받았지만 사실 그 과정이 항상 행복하진 않았어요. 뒤쳐지는 것 같고, 다들 쉽게 따라가는 것 같은데 저는 그렇지 못할 떄가 많았거든요. 근데 시간이 지나고 돌아보니 생각보다 그 당시 차이가 크지 않았던 것 같다는 생각을 할 때가 많아요. 멀리서 보면 별거 아닌데 그 태풍 속에 내가 있다보니 크게 느껴졌던 것이죠.
결과적으로 각자 성장하는 시기, 속도는 다르지만 포기하지 않는다면 다 원하는 성장을 이뤄내는 것 같아요. 그렇기에 포기하지 마시고 본인의 페이스대로 완주하는 것이 정말 중요하다고 생각합니다.
저는 모르는 것 또는 개념을 발견했을 때 중요도, 상황에 따라 다르게 처리해요.
당장 이 개념을 반드시 알아야 하는 상황이 있고, 상황에 따라선 몰라도 쓸 수 있는 상황들이 있어요.
지금 내가 하려고 하는 것에 이게 어느정도 중요한 것인지? 꼭 알아야 하는 개념인지 고민해본 후 중요한 부분이면 지금 중요한 만큼만 접근해서 공부하는 편이에요. 이후 시간이 지난 후 더 자세히 공부하기도 하고, 애매하게 알게 된 개념은 계속 다른 공부를 하다보면 이제 키워드는 알기 때문에 계속 눈에 보이더라구요..! 그렇게 반복적으로 접하게 된다면 그때 더 깊이있게 공부하기도 해요.
화이팅이에요!!
| @@ -4,7 +4,7 @@ describe('search test', () => { | |||
| cy.intercept('GET', '**/search/movie**', { fixture: 'searchMovies.json' }) | |||
| }) | |||
| it('검색어를 입력한 뒤 검색 버튼을 누르면 필터링된 영화 목록을 보여준다.', () => { | |||
There was a problem hiding this comment.
우선 테스트 시나리오를 유저 행동 기반으로 작성한 점은 정말 좋다고 생각해요.
다만 현재 해당 테스트 시나리오는 한번에 2개를 검증하고 있는데 이렇게 하신 의도가 있을까요?
따로따로 나눠서 해도 되지 않을까 해서요!
There was a problem hiding this comment.
검색을 하지 않았을때 뽑은 타이틀 텍스트와 비교하기 위해 그랬던 것인데, fixture 데이터를 정의했을 시점에 수정했어야하는데 놓친 것 같습니다..!
해당 부분 수정했습니다!
cypress/e2e/starRatingTest.cy.ts
Outdated
| }) | ||
| }) | ||
|
|
||
| it('별점 클릭 시 localStorage에 별점이 저장된다.', () => { |
There was a problem hiding this comment.
P2;
E2E 테스트에선 유저 행동 기반으로 작성하는게 좋아요.
현재 로컬 스토리지에 저장하는지 여부는 유저 입장에서 굳이 알 필요가 없기도 해요.
유저 관점으로 테스트 시나리오를 작성한다면 어떻게 해볼 수 있을까요?
There was a problem hiding this comment.
인터렉션 관점에서 생각했어야하는데 구현 세부사항으로 빠져버렸네요 ㅠ
중요한건 "별점을 매기고 모달을 껏다 켜도 남아있는가" 이니까
해당 부분 테스트만 남기고 내부 동작 검증 테스트는 지웠습니다!
yuncic
left a comment
There was a problem hiding this comment.
꼼꼼한 리뷰와 질문 답변 감사합니다!
너무 답답하고 걱정되는 고민이었는데 덕분에 마음이 한결 편해진 것 같아요 ㅠ
남겨주신 코멘트들 피드백 반영해놨습니다!
추가 질문은 index.html에서 별점 부분 관련 질문입니다.
<div class="stars-rate-container"> <div class="stars"> <img class="star-icon" src="./templates/images/star_empty.png" data-value="2" /> <img class="star-icon" src="./templates/images/star_empty.png" data-value="4" /> <img class="star-icon" src="./templates/images/star_empty.png" data-value="6" /> <img class="star-icon" src="./templates/images/star_empty.png" data-value="8" /> <img class="star-icon" src="./templates/images/star_empty.png" data-value="10" /> </div> <p class="my-rate-text"></p> </div>
단순히 빈별 이미지 -> 꽉찬 별 이미지 로 바꾸는 작업을 떠올리면서 div -> img 태그를 썼는데,
생각해보니 시멘틱하지 않은 것 같다는 생각이 들었습니다.
이 상태로 놔둬도 괜찮을지 아니면 input 이나 button태그를 써야할지 조언 부탁드립니다!
| @@ -4,7 +4,7 @@ describe('search test', () => { | |||
| cy.intercept('GET', '**/search/movie**', { fixture: 'searchMovies.json' }) | |||
| }) | |||
| it('검색어를 입력한 뒤 검색 버튼을 누르면 필터링된 영화 목록을 보여준다.', () => { | |||
There was a problem hiding this comment.
검색을 하지 않았을때 뽑은 타이틀 텍스트와 비교하기 위해 그랬던 것인데, fixture 데이터를 정의했을 시점에 수정했어야하는데 놓친 것 같습니다..!
해당 부분 수정했습니다!
cypress/e2e/starRatingTest.cy.ts
Outdated
| }) | ||
| }) | ||
|
|
||
| it('별점 클릭 시 localStorage에 별점이 저장된다.', () => { |
There was a problem hiding this comment.
인터렉션 관점에서 생각했어야하는데 구현 세부사항으로 빠져버렸네요 ㅠ
중요한건 "별점을 매기고 모달을 껏다 켜도 남아있는가" 이니까
해당 부분 테스트만 남기고 내부 동작 검증 테스트는 지웠습니다!
- 페이지 크기를 fixture에서 읽어 하드코딩 제거 - 스크롤 횟수를 루프로 관리 - 절대 개수 대신 "이전보다 증가했는지"로 검증해 API 응답 크기 변동에 유연하게 대응
eastroots92
left a comment
There was a problem hiding this comment.
안녕하세요 찰리.
빠르게 대응해주셔서 감사해요.
책임 분리, 관심사 분리 측면에서 코멘트 남겼으니 확인 부탁드려요!
질문에 대한 답변
별점 이미지는 button, input등을 안써도 괜찮을까?
단순 img는 스크린리더, 검색엔진 등 단순 이미지로 해석해요.
지금은 클릭하면 상태가 변경되어야 하다보니 시멘틱 측면에서 동작이 어긋나게 돼요.
- button을 쓰는 방식
- input radio를 쓰는 방식
현재 상황에서 위 두개의 방식이 좋다고 생각되는데 각각 장단이 있어요.
1은 구현 난이도가 낮고, 2의 경우 구현 난이도는 있지만 조금 더 정석적인 방법이에요.
|
|
||
| it('인기순 영화 페이지에서 두 번째 포스터를 클릭하면 백그라운드에 해당 영화 정보가 띄워진다.', () => { | ||
| cy.visit('https://javascript-movie-review-dvlk.vercel.app/') | ||
| cy.visit('http://localhost:5173'); |
There was a problem hiding this comment.
P1;
URL은 상수로 관리해도 좋을 것 같은데 어떻게 생각하시나요!?
| it('모달을 닫았다가 다시 열면 이전에 저장한 별점이 복원된다.', () => { | ||
| cy.get('.star-icon[data-value="6"]').click(); |
There was a problem hiding this comment.
P3.
새로고침 후에도 별점이 잘 유지되는지 등도 고려해볼 수 있을것 같아요!
| beforeEach(() => { | ||
| cy.intercept('GET', '**/movie/*', { fixture: 'infoModal.json' }); | ||
| cy.intercept('GET', '**/movie/popular**', { fixture: 'popularMovies.json' }); | ||
| cy.clearLocalStorage(); |
There was a problem hiding this comment.
P4;
현재 별점 매기기에만 clearLocalStorage를 하고 있는데 이를 모든 테스트에서 항상 초기화 한다면 어떻게 할 수 있을까요?
| for (let i = 0; i < scrollCount; i++) { | ||
| cy.get('.thumbnail-list li').then(($items) => { | ||
| const beforeCount = $items.length; | ||
|
|
||
| cy.get('.thumbnail-list li').last().scrollIntoView(); | ||
|
|
||
| cy.get('.thumbnail-list li').should('have.length.greaterThan', beforeCount); | ||
| }); | ||
| } |
There was a problem hiding this comment.
P2;
실무에서는 스크롤 카운트로 loop를 도는 것이 아니라 api의 값을 통해 페이지네이션의 끝을 알게 되어요!
무한 스크롤, 페이지네이션 등에서 어떻게 API를 구성하고 프론트에선 어떻게 적용하면 좋을지 한번 찾아보고 개선해볼 수 있을까요?
| @@ -0,0 +1,11 @@ | |||
| const KEY_PREFIX = 'rating_'; | |||
|
|
|||
| export const starRatingStorage = { | |||
There was a problem hiding this comment.
p2;
아래 케이스를 대응해볼 수 있을까요?
-
현재 별점에서만 사용할 수 있는데 최근 검색어, 최근 클릭한 영화 등을 구현할 때마다 매번 비슷한 파일을 만들게 될 것 같아요. 최소한의 공수로 여러 스토리지를 대응한다면 어떻게 할 수 있을까요?
-
상황에따라 갑작스레 세션스토리지, 쿠키, 또는 서버 스토리지(Redis, KV 기반의 스토리지) 등으로 자유자재 바꿔야 하는 상황이 생긴다면? 어떻게 설계 했을 때 쉽게 대응 할 수 있을까?
| } from '../view/movieListView.ts' | ||
| updateMyStarRate, | ||
| } from '../view/movieListView.ts'; | ||
| import { starRatingStorage } from '../storage/StarRatingStorage.ts'; |
There was a problem hiding this comment.
P3;
bindMovieEvent는 "별점을 어딘가에 저장/조회한다"만 알면 충분한데, 지금은 특정 구현체(싱글톤 객체) 를 직접 참조해요. 나중에 "IndexedDB로 바꿀래요", "서버 API로 올릴래요", "테스트에선 in-memory로 바꾸고 싶어요"가 오면 bindMovieEvent를 고쳐야 할 것 같은데 어떻게 하면 쉽게 바꿀 수 있을까요?
| }) | ||
| } | ||
| // 포스터 클릭 이벤트 | ||
| export const bindClickPosterEvent = () => { |
There was a problem hiding this comment.
P1. bindClickPosterEvent가 너무 많은 일을 하고 있어요.
한 함수에 여덟 가지 책임이 뭉쳐 있어요.
이렇게 되면
- 테스트가 어려워져요 (함수 하나 부르면 DOM 전체와 API가 엮여요)
- 한 가지를 고치려다 다른 걸 깨기 쉬워요
- 이름(bindClickPosterEvent)과 실제 내용이 안 맞아요. (모달 전체 생애주기를 다 다뤄요)
역할별로 책임을 나누어 보면 좋을 것 같아요.
한 번에 다 쪼개기는 부담이니, 우선 모달 열기/닫기 부분과 DOM에 영화 정보 채우는 부분만이라도 분리해 주세요.
🎯 미션 소개
🕵️ 셀프 리뷰(Self-Review)
제출 전 체크 리스트
리뷰 요청 & 논의하고 싶은 내용
1) 이번 단계에서 가장 많이 고민했던 문제와 해결 과정에서 배운 점
무한 스크롤을 이벤트 리스너로 스크롤을 기준삼아 구현하려고 했습니다.
스크롤 이벤트 기반으로 구현 시 scrollTop + clientHeight >= scrollHeight 조건을 매 스크롤마다 체크하는 구조가 됩니다.
스크롤 이벤트는 1초에 수십 번 발생하기 때문에 불필요한 연산이 과도하게 일어나는 문제가 있었습니다.
이를 해결하기 위해
IntersectionObserverAPI를 적용했습니다.리스트 최하단 엘리먼트를 정의하고, 해당 엘리먼트가 뷰포트와 교차하는 시점에만 콜백이 실행되도록 했습니다.
필요한 시점에만 pageNum을 증가시켜 다음 페이지를 요청할 수 있었습니다.
반응형 구현 과정에서 CSS 상대 단위들의 기준점 차이를 명확히 학습했습니다.
%는 부모 엘리먼트의 크기를 기준으로 계산됩니다.rem은 루트 엘리먼트(html)의 font-size를 기준으로 하며, 일반적으로 브라우저 기본값인 16px이 1rem이 됩니다.vw/vh는 각각 뷰포트 너비와 높이의 1%를 의미하며 부모 엘리먼트와 무관하게 항상 뷰포트 기준으로 계산됩니다.단순히 "상대값"이라는 개념만 알고 있던 상태에서, 각 단위가 어떤 컨텍스트를 기준으로 삼는지 구분하게 됐습니다.
2) 이번 리뷰를 통해 논의하고 싶은 부분
현재 구조 설계는 스스로 시도해보지만, 로직 구현 단계에서 막히면 AI에 의존한 뒤 복기로 이해하는 방식으로 학습하고 있습니다.
이 방식이 맞는 방향인지 확신이 서지 않아 불안한 상태입니다.
불안한 이유는 크게 두 가지입니다.
첫째는 기본기에 대한 불안입니다.
로또, 영화리뷰 미션을 처음부터 다시 구현하려 하면 못할 것 같다는 느낌이 있습니다.
코드를 이해했다고 생각했지만, 진짜로 내 것이 된 건지 확신이 없습니다.
둘째는 구멍이 쌓일 것 같다는 두려움입니다.
바닐라 JS도 아직 부족한 상태에서 레벨 2 React로 넘어가면, 메꿔지지 않은 빈틈 위에 계속 쌓아올리기만 하는 것이 아닐까 하는 걱정이 있습니다.
그럼에도 학습에 AI를 사용하게 되는 이유는, 레벨 속도를 맞춰야 한다는 압박과 함께 레벨 2, 3, 4를 거치면서 자연스럽게 채워지지 않을까 하는 기대가 있기 때문입니다.
지금 단계에서 사소한 불안 때문에 속도를 늦추는 것이 더 손해일 수 있다는 생각도 동시에 있습니다.
또, 막히거나 방향자체를 모르겠을 때 스트레스가 극에 달하면 홧김에 AI를 쓰는 경우도 있는 것 같습니다 ㅋㅋ ... ㅠ
결국 "해결책을 먼저 알고 나서 학습하면 되지 않나"와 "그 고민의 과정이 진짜 성장인데 너무 쉽게 포기하는 건 아닌가" 사이에서 결론을 내리지 못하고 있는 상태입니다.
레벨 2 전까지는 로또, 영화리뷰 미션 코드를 읽으며 막혔던 부분을 학습하고 따라 쳐보는 것과 React 공식문서로 기본기를 다지는 것을 병행할 계획입니다.
이 방향이 적절한지, 혹시 더 효율적인 학습 방법이 있다면 조언해 주시면 감사하겠습니다.
✅ 리뷰어 체크 포인트