[2단계 - 상세 정보 & UI/UX 개선하기] 클라우디 미션 제출합니다.#314
Open
bel1c10ud wants to merge 55 commits intowoowacourse:bel1c10udfrom
Open
[2단계 - 상세 정보 & UI/UX 개선하기] 클라우디 미션 제출합니다.#314bel1c10ud wants to merge 55 commits intowoowacourse:bel1c10udfrom
bel1c10ud wants to merge 55 commits intowoowacourse:bel1c10udfrom
Conversation
Co-authored-by: bel1c10ud <bel1c10ud@users.noreply.github.com>
Co-authored-by: bel1c10ud <bel1c10ud@users.noreply.github.com>
Co-authored-by: bel1c10ud <bel1c10ud@users.noreply.github.com>
Co-authored-by: bel1c10ud <bel1c10ud@users.noreply.github.com>
Co-authored-by: bel1c10ud <bel1c10ud@users.noreply.github.com>
Co-authored-by: bel1c10ud <bel1c10ud@users.noreply.github.com>
Co-authored-by: bel1c10ud <bel1c10ud@users.noreply.github.com>
Co-authored-by: Untae Lee <bigcloud07@users.noreply.github.com>
Co-authored-by: Untae Lee <bigcloud07@users.noreply.github.com>
Co-authored-by: Untae Lee <bigcloud07@users.noreply.github.com>
Co-authored-by: Untae Lee <bigcloud07@users.noreply.github.com>
Co-authored-by: Untae Lee <bigcloud07@users.noreply.github.com>
Co-authored-by: Untae Lee <bigcloud07@users.noreply.github.com>
Co-authored-by: Untae Lee <bigcloud07@users.noreply.github.com>
Co-authored-by: Untae Lee <bigcloud07@users.noreply.github.com>
Co-authored-by: Untae Lee <bigcloud07@users.noreply.github.com>
Co-authored-by: Untae Lee <bigcloud07@users.noreply.github.com>
Co-authored-by: Untae Lee <bigcloud07@users.noreply.github.com>
Co-authored-by: Untae Lee <bigcloud07@users.noreply.github.com>
Co-authored-by: Untae Lee <bigcloud07@users.noreply.github.com>
Co-authored-by: Untae Lee <bigcloud07@users.noreply.github.com>
Co-authored-by: Untae Lee <bigcloud07@users.noreply.github.com>
span(인라인 요소) 안에 div(블록 요소)가 있던 문제 해결
- constans.ts -> constants.ts - getPramFromURL -> getParamFromURL - SHOW_MORE_TROTTLE_MS -> SHOW_MORE_THROTTLE_MS
- APIError, UnknownError 커스텀 에러 클래스 추가 - 에러 핸들링을 위한 handleError 함수 추가 - showMoreButton 콜백에 에러 핸들링 추가
- APIError에 response 필드 추가 - fetcher 내부로 timeout 이동 - fetchMovies의 불필요한 try/catch 제거, 에러를 그대로 전파하도록 단순화
- 뷰포인트안에 들어오는 요소의 페이지와 아이디를 URL에 반영 - 페이지 정보가 URL에 포함된 경우 해당 페이지만 먼저 fetch, 이후 위, 아래 페이지를 추가 로드 - 아이디가 URL에 포함된 경우 해당 위치로 스크롤 이동
|
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영화 검색 및 평가 애플리케이션을 위한 전체 프론트엔드 구현을 추가했습니다. TMDB API 통합을 통해 인기 영화 목록 조회, 검색 기능, 영화 상세 정보 모달 표시, 별점 평가를 구현합니다. 위/아래 무한 스크롤링, 스크롤 위치 복원, 로컬스토리지 기반 평가 저장, API 타임아웃 및 실패 처리, 토스트 알림 시스템을 포함합니다. 두 개의 HTML 페이지(인기 영화, 검색 결과), 다양한 CSS 스타일시트, API 페칭/에러 처리 모듈, 렌더링 및 이벤트 핸들링 함수들, 옵저버 기반 무한 스크롤 로직, 포괄적인 Cypress E2E 테스트 스위트를 추가합니다. Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes 🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 12
Note
Due to the large number of review comments, Critical, Major severity comments were prioritized as inline comments.
🟡 Minor comments (10)
README.md-106-106 (1)
106-106:⚠️ Potential issue | 🟡 Minor오타 수정이 필요합니다.
다음 두 가지 오타가 있습니다:
- "localStorag" → "localStorage" (철자 오류)
- "구현하돼" → "구현하되" (문법 오류)
📝 제안하는 수정
- - [x] localStorag로 구현하돼 서버 API로 쉽고 안전하게 갈아끼울 수 있는 구조로 개발하여야한다. + - [x] localStorage로 구현하되 서버 API로 쉽고 안전하게 갈아끼울 수 있는 구조로 개발하여야한다.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@README.md` at line 106, Fix the typos in the README sentence: replace "localStorag" with "localStorage" and change "구현하돼" to "구현하되" so the line reads correctly; locate the sentence in README.md containing "localStorag로 구현하돼 서버 API로 쉽고 안전하게 갈아끼울 수 있는 구조로 개발하여야한다." and apply those two replacements.src/page/popular.ts-9-11 (1)
9-11:⚠️ Potential issue | 🟡 Minor에러 처리 중 2차 예외 발생 시 정리 로직이 건너뛰어질 수 있습니다.
Line 10에서
handleError가 예외를 다시 던지면 Line 11removeSkeletonItems()가 실행되지 않아 로딩 UI가 남을 수 있습니다. 정리 로직은finally경로에서 보장 실행되는 구조인지 한 번 점검해 주세요.다음으로 현재 구현이 안전한지 빠르게 확인할 수 있습니다.
#!/bin/bash # 1) handleError 내부에 자체 예외 보호(try/catch)가 있는지 확인 fd -i "error.ts" --exec sed -n '1,240p' {} # 2) popular 페이지의 catch 정리 경로 확인 fd -i "popular.ts" --exec sed -n '1,140p' {}기대 결과:
handleError내부에서 파싱/메시지 생성 중 예외를 흡수하지 않는다면,
removeSkeletonItems()는 finally 성격으로 분리하는 편이 안전합니다.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/page/popular.ts` around lines 9 - 11, The catch block currently calls handleError(error) then removeSkeletonItems(), which can be skipped if handleError throws; update the error handling so cleanup always runs: either wrap handleError(error) in its own try/catch or move removeSkeletonItems() into a finally block that runs regardless of handleError outcome; also inspect the handleError implementation to confirm it already swallows internal parsing/formatting errors (check the handleError function) and if not, add internal try/catch there to prevent rethrows.src/event/bindSelectRatingEvent.ts-9-9 (1)
9-9:⚠️ Potential issue | 🟡 Minor타입 단언이 실제 요소 타입과 맞지 않습니다.
.modal-movie-my-rating-selector는div요소인데HTMLFormElement로 단언하고 있습니다.HTMLElement또는HTMLDivElement가 더 정확합니다.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/event/bindSelectRatingEvent.ts` at line 9, The type assertion on formElement is incorrect: change the cast of the result of event.target.closest(".modal-movie-my-rating-selector") from HTMLFormElement to a more accurate type (e.g., HTMLDivElement or HTMLElement) and update any downstream assumptions in bindSelectRatingEvent that rely on form-specific APIs (replace form-only methods or guard them with appropriate checks) so interactions with formElement use properties/methods valid for a div/HTMLElement.src/render/renderSearchListEmptyAlert.ts-1-1 (1)
1-1:⚠️ Potential issue | 🟡 Minor함수 이름에 오타가 있습니다:
createSearchListrEmptyAlertTemplate.
Listr→List로 수정이 필요합니다.✏️ 오타 수정
-function createSearchListrEmptyAlertTemplate() { +function createSearchListEmptyAlertTemplate() {🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/render/renderSearchListEmptyAlert.ts` at line 1, Rename the misspelled function createSearchListrEmptyAlertTemplate to createSearchListEmptyAlertTemplate, update its declaration and every reference (calls, exports, imports, tests) to the new name, and ensure any exported symbol or default export that used the old name is updated to match.src/styles/tab.css-8-20 (1)
8-20:⚠️ Potential issue | 🟡 MinorStylelint: 선언문 앞 빈 줄 오류.
정적 분석 도구에서 Lines 12, 18의 빈 줄을 지적하고 있습니다. 프로젝트 스타일 가이드에 맞게 수정이 필요합니다.
🔧 빈 줄 제거 제안
.tab-item { cursor: pointer; user-select: none; - display: flex; align-items: center; justify-content: center; width: 120px; height: 40px; - border-radius: 40px; background: linear-gradient(90deg, `#14191f` 0%, `#0a0d12` 100%); }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/styles/tab.css` around lines 8 - 20, 스타일 규칙 위반은 .tab-item 블록 내부의 선언 앞에 불필요한 빈 줄이 있는 것입니다; .tab-item 선택자 내에서 display, align-items, justify-content 등 선언들 사이의 빈 줄(현재 Lines 12, 18에 해당)을 제거해 선언이 연속되도록 정렬하고 프로젝트 스타일 가이드에 맞춰 공백을 없애세요.src/styles/thumbnail.css-77-87 (1)
77-87:⚠️ Potential issue | 🟡 Minorgap과 margin-bottom 중복 가능성
.item-desc에gap: 4px(Line 80)이 설정되어 있고,.item-desc>*:not(:last-child)에margin-bottom: 4px(Line 85)도 설정되어 있습니다. flexbox의 gap과 margin이 함께 적용되어 의도치 않은 8px 간격이 발생할 수 있습니다.둘 중 하나만 사용하는 것이 명확합니다.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/styles/thumbnail.css` around lines 77 - 87, The CSS currently applies both gap: 4px on .item-desc and margin-bottom: 4px on .item-desc>*:not(:last-child), causing doubled spacing; choose one approach and remove the other to avoid the duplicate 8px spacing—either delete margin-bottom from .item-desc>*:not(:last-child) and rely on .item-desc { gap: 4px; } for consistent flex gaps, or remove gap and keep margin-bottom for manual control; update the rule(s) for .item-desc and .item-desc>*:not(:last-child) accordingly.src/styles/main.css-54-57 (1)
54-57:⚠️ Potential issue | 🟡 Minor
min-width: 1440px과 반응형 디자인 충돌 가능성
#wrap에min-width: 1440px이 설정되어 있어, 1440px 미만의 화면에서는 가로 스크롤이 발생합니다. 이는 Line 99-108의 반응형 미디어 쿼리(width < 1280px)와 충돌할 수 있습니다.모바일/태블릿 지원이 필요하다면
min-width를 제거하거나 미디어 쿼리 내에서 조정하는 것을 검토해 주세요.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/styles/main.css` around lines 54 - 57, `#wrap에` 고정된 min-width: 1440px가 반응형 미디어 쿼리와 충돌해 작은 화면에서 가로 스크롤을 유발합니다; 이 문제를 해결하려면 `#wrap의` min-width 속성을 제거하거나 미디어 쿼리(현재 작은 화면 처리를 담당하는 규칙) 내부에서만 적용하도록 옮기고, 대신 필요하면 max-width 또는 width: 100%를 사용해 컨테이너가 뷰포트 이하로 줄어들 수 있게 하며 모바일/태블릿 크기에서 레이아웃을 깨지 않도록 해당 미디어 쿼리에서 min-width를 재정의하거나 제거하세요.src/styles/modal.css-148-155 (1)
148-155:⚠️ Potential issue | 🟡 Minor중복 선택자 오류
data-rating="10"규칙에서button:nth-of-type(1)이 두 번 선언되어 있습니다 (Line 148, 149). 이는 복사-붙여넣기 오류로 보입니다..modal-movie-my-rating-selector[data-rating="10"]>button:nth-of-type(1), -.modal-movie-my-rating-selector[data-rating="10"]>button:nth-of-type(1), .modal-movie-my-rating-selector[data-rating="10"]>button:nth-of-type(2), .modal-movie-my-rating-selector[data-rating="10"]>button:nth-of-type(3), .modal-movie-my-rating-selector[data-rating="10"]>button:nth-of-type(4), .modal-movie-my-rating-selector[data-rating="10"]>button:nth-of-type(5) {🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/styles/modal.css` around lines 148 - 155, The selector list for .modal-movie-my-rating-selector[data-rating="10"] contains a duplicated entry button:nth-of-type(1); remove the duplicate selector (or, if the intent was to target a different button, replace the duplicate with the correct nth-of-type index) so the rules only include each button selector once (refer to .modal-movie-my-rating-selector and the data-rating="10" selector to locate the block).src/render/renderPopularMoviePage.ts-33-33 (1)
33-33:⚠️ Potential issue | 🟡 Minor빈 결과 배열에 대한 방어 코드 필요
response.results가 빈 배열일 경우response.results[0]은undefined가 됩니다.renderTopRatedMovie함수는Movie타입의 필수 매개변수를 요구하며, 함수 내에서movie.id,movie.title,movie.vote_average,movie.backdrop_path등의 속성에 직접 접근하기 때문에undefined를 전달하면 런타임 에러가 발생합니다.다음과 같이 결과 배열이 비어있지 않음을 확인한 후 함수를 호출하시기 바랍니다:
- isTopPage && renderTopRatedMovie(response.results[0]); + isTopPage && response.results[0] && renderTopRatedMovie(response.results[0]);🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/render/renderPopularMoviePage.ts` at line 33, 검증 누락으로 response.results가 빈 배열일 때 response.results[0]이 undefined가 되어 renderTopRatedMovie에 잘못 전달됩니다; 호출부에서 isTopPage && renderTopRatedMovie(response.results[0]); 대신 response.results가 배열인지와 길이 > 0인지를 확인한 뒤 renderTopRatedMovie를 호출하도록 수정하세요 (예: check Array.isArray(response.results) && response.results.length > 0), 대상 식별자: response.results, renderTopRatedMovie, isTopPage, Movie.src/error.ts-70-77 (1)
70-77:⚠️ Potential issue | 🟡 Minor
APIError처리 시 두 가지 잠재적 문제
JSON 파싱 실패 미처리:
content-type이application/json이지만 body가 유효한 JSON이 아닌 경우error.response.json()이 예외를 던집니다.handleError내에서 다시 에러가 발생하면 사용자에게 토스트가 표시되지 않을 수 있습니다.객체를 문자열로 변환 시
[object Object]: Line 73에서body가 JSON 객체일 때String(body)는"[object Object]"가 됩니다.이 두 가지 엣지 케이스를 어떻게 처리할 수 있을지 고민해보시겠어요?
💡 힌트: 안전한 body 처리
if (error instanceof APIError) { const isJson = error.response.headers.get("content-type")?.includes("application/json"); - const body = isJson ? await error.response.json() : await error.response.text(); - const bodyStr = String(body); + let bodyStr: string; + try { + const body = isJson ? await error.response.json() : await error.response.text(); + bodyStr = typeof body === "object" ? JSON.stringify(body) : String(body); + } catch { + bodyStr = "응답 본문을 읽을 수 없습니다."; + } const message = bodyStr.length ? bodyStr : "응답이 없습니다.";🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/error.ts` around lines 70 - 77, When handling APIError in src/error.ts (the block using APIError, error.response.json(), body and bodyStr), make JSON parsing robust by wrapping the call to error.response.json() in a try/catch and falling back to error.response.text() if json parsing fails or throws; normalize the resulting body value so that if it's an object you convert it via JSON.stringify (with a try/catch and a sensible small-length fallback like "<<non-serializable body>>") and if it’s not a string ensure you coerce to a readable string; then build message from that safe body string (with a max length if desired) before calling showErrorToast so the toast always receives a human-readable message instead of "[object Object]" or blowing up on invalid JSON.
🧹 Nitpick comments (18)
src/styles/skeleton.css (1)
1-6: skeleton 애니메이션에 모션 감소 환경 대응을 추가해 주세요.현재 shimmer가 항상 동작하므로,
prefers-reduced-motion사용자를 위한 비활성화/완화 규칙을 두면 접근성이 더 좋아집니다.Also applies to: 38-46
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/styles/skeleton.css` around lines 1 - 6, Add a prefers-reduced-motion media query to disable or simplify the .skeleton animation for users who request reduced motion: detect the user's preference with `@media` (prefers-reduced-motion: reduce) and inside it remove the animation on the .skeleton selector (or replace it with a non-animating static background) and ensure any `@keyframes` shimmer is not applied; update both the .skeleton rule and any related rules (e.g., the `@keyframes` shimmer block) so the shimmer is suppressed when prefers-reduced-motion is set.src/styles/colors.css (1)
1-13: 중복된 색상 변수 정리 고려
--color-white와--color-ffffff가 동일한 값(#ffffff)이고,--color-bluegray-100과--color-242a32도 동일한 값(#242a32)입니다.의도적인 설계라면 무시해도 되지만, 유지보수를 위해 하나로 통일하거나 한쪽이 다른 쪽을 참조하도록 변경하는 것을 고려해 보세요:
--color-ffffff: var(--color-white); --color-242a32: var(--color-bluegray-100);🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/styles/colors.css` around lines 1 - 13, Two color variables are duplicated: --color-white and --color-ffffff both equal `#ffffff`, and --color-bluegray-100 and --color-242a32 both equal `#242a32`; consolidate by removing one of each pair or make one an alias to the other (e.g., set --color-ffffff to reference var(--color-white) and/or set --color-242a32 to reference var(--color-bluegray-100)) so code uses a single source of truth for those values and avoids redundant literals.src/render/restoreScrollPosition.ts (1)
6-10:setTimeout(..., 0)타이밍 관련 검토 필요
setTimeout(..., 0)은 현재 이벤트 루프 틱 이후로 실행을 지연시키지만,renderSearchMoviePage나renderPopularMoviePage가 비동기로 영화 아이템을 렌더링하는 경우, 해당 요소가 아직 DOM에 존재하지 않을 수 있습니다.
restoreScrollPosition()이 호출되는 시점을 확인해 보시기 바랍니다. 컨텍스트 스니펫에 따르면renderMovieItems이후에 호출되므로 대부분의 경우 정상 동작할 것으로 보이지만, 만약 스크롤 복원이 간헐적으로 실패한다면 이 부분을 확인해 보세요.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/render/restoreScrollPosition.ts` around lines 6 - 10, restoreScrollPosition currently uses setTimeout(...,0) which can run before async rendering (e.g., renderSearchMoviePage, renderPopularMoviePage, or renderMovieItems) finishes and the target li[data-movie-id] exists; change restoreScrollPosition to wait for the element instead of a zero-timeout: either make renderMovieItems (or the callers renderSearchMoviePage/renderPopularMoviePage) return/await a Promise and call await restoreScrollPosition after rendering completes, or implement a short-element-wait inside restoreScrollPosition (poll with requestAnimationFrame or use a MutationObserver watching the container until document.querySelector(`li[data-movie-id="${targetMovieId}"]`) is present) and then call scrollIntoView on the found element.src/styles/tab.css (1)
19-27:colors.css를 import하지만 하드코딩된 색상을 사용하고 있습니다.
colors.css를 import하는데, 색상값들이 직접 하드코딩되어 있습니다.colors.css에 정의된 CSS 변수가 있다면 활용하는 것이 유지보수에 좋습니다.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/styles/tab.css` around lines 19 - 27, The gradients in tab.css are using hardcoded hex colors for the .tab-item.selected and .tab-item:hover (and the base background rule) instead of the CSS variables from colors.css; update these rules to reference the appropriate CSS custom properties (e.g., var(--background, ...), var(--accent, ...), var(--hover, ...)) defined in colors.css, replacing the literal hex values so the gradients use those variables and fallback values if needed, ensuring selectors .tab-item.selected and .tab-item:hover are updated accordingly.src/render/renderSearchListEmptyAlert.ts (1)
15-21: 빈 목록일 때 중복 alert 생성 가능성이 있습니다.
renderSearchListEmptyAlert()가 연속으로 호출되면 alert가 중복 생성될 수 있습니다. alert를 추가하기 전에 기존 alert를 제거하거나, 이미 존재하는지 확인하는 로직을 추가하면 어떨까요?♻️ 중복 방지 로직 추가 제안
export default function renderSearchListEmptyAlert() { const listEl = document.querySelector(".thumbnail-list"); if (!listEl) return; if (listEl.children.length === 0) { + // 기존 alert가 있으면 추가하지 않음 + if (document.querySelector('.empty-list-alert')) return; + listEl.insertAdjacentHTML( "afterend", createSearchListrEmptyAlertTemplate(), ); } else { document.querySelector('.empty-list-alert')?.remove(); } }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/render/renderSearchListEmptyAlert.ts` around lines 15 - 21, renderSearchListEmptyAlert may insert duplicate alerts when called repeatedly; before inserting the template from createSearchListrEmptyAlertTemplate(), check for an existing element with the '.empty-list-alert' selector (e.g., via document.querySelector('.empty-list-alert')) and remove it or early-return if present, or alternatively remove any existing alert unconditionally before calling listEl.insertAdjacentHTML; update the logic around listEl.children.length and the else branch to ensure a single alert element is maintained.src/constants.ts (1)
11-19: 타입 안전성 향상을 위해as const사용을 고려해보세요.
RATING_OPTIONS와RATING_MESSAGES에as const를 추가하면 타입 추론이 더 정확해집니다. 예를 들어RATING_OPTIONS는number[]대신readonly [2, 4, 6, 8, 10]으로 추론됩니다.💡 타입 안전성 개선 제안
-export const RATING_OPTIONS = [2, 4, 6, 8, 10]; +export const RATING_OPTIONS = [2, 4, 6, 8, 10] as const; export const RATING_MESSAGES = { -} +} as const;🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/constants.ts` around lines 11 - 19, Update the constants to be readonly literal types by appending "as const" to both RATING_OPTIONS and RATING_MESSAGES so TypeScript infers them as tuple/readonly literal mappings rather than loose number[] and string index types; modify the declarations for RATING_OPTIONS and RATING_MESSAGES accordingly (refer to the RATING_OPTIONS and RATING_MESSAGES symbols) so downstream code benefits from stronger type safety.src/event/bindClickMovieEvent.ts (1)
11-13:movieId유효성 검사에서NaN체크가 더 명확할 수 있습니다.
!movieId조건은NaN을 잡아내지만, 만약 movie ID가0인 경우도 에러로 처리됩니다 (TMDB에서 ID가 0인 경우는 없겠지만).Number.isNaN()사용을 고려해 보시는 건 어떨까요?💡 더 명확한 유효성 검사 제안
const movieIdStr = (targetElement as HTMLElement).dataset.movieId; const movieId = Number(movieIdStr); - if (!movieId) throw new Error("영화 ID를 찾을 수 없습니다.") + if (Number.isNaN(movieId)) throw new Error("영화 ID를 찾을 수 없습니다.")🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/event/bindClickMovieEvent.ts` around lines 11 - 13, The current check uses if (!movieId) which treats 0 as invalid and can be ambiguous; in bindClickMovieEvent replace that falsy check with an explicit NaN check using Number.isNaN(movieId) (and optionally also guard an empty movieIdStr if needed) so only truly non-numeric values cause the throw in the block that references movieId and movieIdStr.src/render/renderMyRatingSelector.ts (1)
5-7: 변수명이 실제 타입과 맞지 않습니다.
formElement라는 이름이지만 실제로는div요소입니다.HTMLFormElement로 타입 단언되어 사용되는 곳도 있어 혼란을 줄 수 있습니다.♻️ 명확한 변수명 제안
-function createMyRateSelectorElement(currentRating?: number) { - const formElement = document.createElement("div"); - formElement.classList.add("modal-movie-my-rating-selector") +function createMyRateSelectorElement(currentRating?: number) { + const selectorElement = document.createElement("div"); + selectorElement.classList.add("modal-movie-my-rating-selector")🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/render/renderMyRatingSelector.ts` around lines 5 - 7, The variable name formElement in createMyRateSelectorElement is misleading because it holds a div; rename it (e.g., to divElement or wrapperElement) and update all usages inside that function to match, and remove any incorrect HTMLFormElement type assertions or casts so the element is treated as an HTMLDivElement where needed; ensure event listeners and DOM manipulations reference the new variable name and correct element type (createMyRateSelectorElement, formElement).src/styles/toast.css (1)
40-52: 닫기 버튼에 키보드 포커스 표시도 같이 두는 편이 좋겠습니다.
.toast-close는 hover 상태만 정의되어 있어서 키보드로 이동했을 때 현재 포커스를 확인하기 어렵습니다.:focus-visible스타일을 하나 추가해 두면 토스트 닫기 접근성이 훨씬 안정적입니다.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/styles/toast.css` around lines 40 - 52, .toast-close currently only styles hover, so keyboard users can't see focus; add a :focus-visible rule for .toast-close to mirror hover (e.g., set opacity: 1 and visible focus ring/outlines) and ensure it doesn't conflict with existing styles; update the .toast-close:focus-visible selector to provide a clear visible indicator (outline or box-shadow) and preserve accessibility for keyboard navigation.src/render/renderSearchMoviePage.ts (2)
51-58: 세미콜론 누락 (일관성)Line 58에서 함수 호출 후 세미콜론이 누락되어 있습니다. 다른 코드와의 일관성을 위해 추가하는 것이 좋습니다.
- }) + });🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/render/renderSearchMoviePage.ts` around lines 51 - 58, The call to bindBottomInfiniteScrollObserver(...) is missing a trailing semicolon which breaks consistency; update the statement that starts with bindBottomInfiniteScrollObserver(hasNextPage, async () => { ... }) to end with a semicolon so the call is terminated properly (locate the call in src/render/renderSearchMoviePage.ts near the existing async callback and add the semicolon after the closing parenthesis).
38-38: 불필요한 null 체크Line 16-22에서
firstLoadedPage가 항상 설정되므로, Line 38의firstLoadedPage !== null체크는 항상 true입니다. 간결하게firstLoadedPage > 1로 작성할 수 있습니다.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/render/renderSearchMoviePage.ts` at line 38, The hasPrevPage assignment contains an unnecessary null check; simplify the expression by removing the redundant "firstLoadedPage !== null" and set hasPrevPage using only "firstLoadedPage > 1" (update the variable defined as hasPrevPage that references firstLoadedPage accordingly).src/styles/modal.css (1)
157-159: 동일 클래스 중복 정의
.modal-movie-my-rating-message가 Line 157-159와 Line 181-183에서 두 번 정의되어 있습니다. 하나로 통합하면 유지보수가 더 쉬워집니다.Also applies to: 181-183
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/styles/modal.css` around lines 157 - 159, The selector .modal-movie-my-rating-message is defined twice; consolidate them into a single declaration by removing the duplicate and merging any differing properties so all styles for .modal-movie-my-rating-message live in one place; update the remaining rule to include any properties from the removed rule (preserve intent/ordering) and run a quick style check to ensure no visual regressions.src/url.ts (2)
28-33: 정수 검증 로직 가독성 개선 제안
pageNum % 1을 사용한 정수 체크는 동작하지만,Number.isInteger()를 사용하면 의도가 더 명확해집니다.- const page = isNaN(pageNum) || pageNum % 1 || pageNum < 1 ? 1 : pageNum; + const page = isNaN(pageNum) || !Number.isInteger(pageNum) || pageNum < 1 ? 1 : pageNum;🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/url.ts` around lines 28 - 33, The integer validation in getPage currently uses the expression pageNum % 1 which works but is unclear; update getPage to use Number.isInteger(pageNum) when deciding whether to fall back to 1 so the intent is explicit (keep the existing getURLSearchParam("page", "1") and NaN check but replace the pageNum % 1 condition with !Number.isInteger(pageNum) in the ternary that computes page).
1-4: 제네릭 타입T의 반환 타입 정확성
getURLSearchParam<T>의 반환 타입이 명확하지 않습니다.urlParams.get(name)은string | null을 반환하므로, 실제 반환 타입은string | T가 됩니다. 하지만 함수 시그니처에서 이를 명시하지 않아 타입 안전성이 떨어질 수 있습니다.현재 사용처(
getPage,getQuery)에서는 문제가 없지만, 다른 타입의 defaultValue를 전달할 경우 혼란을 줄 수 있습니다.-export function getURLSearchParam<T>(name: string, defaultValue: T) { +export function getURLSearchParam<T extends string>(name: string, defaultValue: T): string {또는 더 명확하게:
export function getURLSearchParam(name: string, defaultValue: string): string {🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/url.ts` around lines 1 - 4, getURLSearchParam's signature wrongly implies it returns T but urlParams.get(name) yields string | null, so update the function signature and return type to reflect actual values (e.g. export function getURLSearchParam<T>(name: string, defaultValue: T): string | T) or simplify by constraining defaultValue to string and returning string only; modify call sites like getPage and getQuery if needed to match the chosen contract and ensure callers handle string | T correctly.src/render/renderPopularMoviePage.ts (1)
14-60: renderSearchMoviePage와의 코드 중복
renderPopularMoviePage와renderSearchMoviePage의 구조가 거의 동일합니다 (페이지 상태 관리, 스켈레톤 렌더링, 옵저버 바인딩 등). 공통 로직을 추상화하여 재사용성을 높이는 것을 고려해 볼 수 있습니다.다만, PR 설명에서 언급하신 것처럼 패턴 도입 여부와 시간 제약을 고려하여 현재 구조를 유지하는 것도 합리적인 선택입니다.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/render/renderPopularMoviePage.ts` around lines 14 - 60, renderPopularMoviePage and renderSearchMoviePage duplicate page-state management, skeleton rendering, observer binding and error handling; refactor by extracting shared behavior into a reusable helper (e.g., a new function like renderPaginatedMovies or managePaginatedRender) that accepts the endpoint/loader callback, page number, direction and a top-item renderer callback, then update renderPopularMoviePage to call that helper (preserving unique bits such as renderTopRatedMovie and fetchMovies call); ensure the helper encapsulates firstLoadedPage/lastLoadedPage updates, renderSkeletonItemsToList/removeSkeletonItems, restoreScrollPosition, and bindTopInfiniteScrollObserver/bindBottomInfiniteScrollObserver wiring so both renderPopularMoviePage and renderSearchMoviePage delegate common flow while keeping their specific differences.src/observer/bindBottomInfiniteScrollObserver.ts (1)
3-15:throttle함수 중복 검토 필요
src/observer/bindBottomInfiniteScrollObserver.ts와src/observer/bindTopInfiniteScrollObserver.ts에서 동일한throttle함수가 정의되어 있습니다.이 중복이 코드 유지보수와 일관성 관리에 어떤 영향을 미칠 수 있을지, 그리고 공통 유틸리티로 분리했을 때 어떤 이점이 있을지 생각해보세요.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/observer/bindBottomInfiniteScrollObserver.ts` around lines 3 - 15, There is a duplicated throttle implementation used by bindBottomInfiniteScrollObserver and bindTopInfiniteScrollObserver; extract the shared function into a single utility module (e.g., create a util file exporting throttle) and update both bindBottomInfiniteScrollObserver and bindTopInfiniteScrollObserver to import and use that exported throttle implementation; ensure the exported function preserves the same signature (throttle<T extends (...args: any[]) => void>(callback: T, ms: number)) and behavior (single timer, immediate call, reset after ms).src/error.ts (1)
30-48: ParseError에 긴 raw 데이터가 포함될 경우 고려
raw값이 매우 긴 문자열(예: 대용량 잘못된 JSON 응답)일 경우, 에러 메시지가 과도하게 길어질 수 있습니다. 프로덕션 환경에서 로깅이나 모니터링 시 문제가 될 수 있는지 검토해보세요.필요하다면
raw를 일정 길이로 잘라서 메시지에 포함하는 방법도 고려해볼 수 있습니다.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/error.ts` around lines 30 - 48, The ParseError constructor currently embeds the full context.raw into the error message which can be extremely large; update the constructor in class ParseError to truncate context.raw to a safe length (e.g., 200 chars) or replace it with a summarized snippet/placeholder before calling super, e.g., create a local shortRaw from context.raw (handling null/undefined) and use that in the super message and/or as an additional property so the full raw is not in log messages by default; keep the original raw in the public readonly raw field unchanged if you want to preserve full data but avoid printing it in the error message.src/api.ts (1)
65-88:RATING_OPTIONS에 없는 rating 전달 시 동작이 명확하지 않음Line 83에서
RATING_OPTIONS.includes(rating)이 false면parsed[movieId]가 업데이트되지 않지만,localStorage.setItem은 여전히 호출됩니다. 유효하지 않은 rating이 전달되었을 때 조용히 무시하는 것이 의도된 동작인가요?만약 의도된 것이라면, 함수 상단에 조기 반환(early return)을 추가하거나 JSDoc으로 이 동작을 문서화하는 것이 유지보수에 도움이 될 수 있습니다.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/api.ts` around lines 65 - 88, The function updateMyRating currently proceeds to call localStorage.setItem even when RATING_OPTIONS.includes(rating) is false, causing a no-op write; change updateMyRating to early-return when the provided rating is not in RATING_OPTIONS (e.g., check RATING_OPTIONS.includes(rating) near the top and return immediately) so you don't perform unnecessary storage writes, and update the JSDoc for updateMyRating to state that invalid ratings are ignored (or alternatively throw a ValidationError instead of returning if you prefer strict behavior).
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@cypress/e2e/spec.cy.ts`:
- Around line 19-20: The cy.visit calls in this spec use URLs without a protocol
(e.g., cy.visit("localhost:5173")), which are treated as relative because
cypress.config.ts has no baseUrl; either add baseUrl: "http://localhost:5173" to
cypress.config.ts and change calls like cy.visit("localhost:5173") to
cy.visit("/") or cy.visit("/?page=2"), or update every cy.visit(...) in this
file (including the occurrences paired with cy.wait("@popularMovies") and other
visits at the lines cited) to use absolute URLs with protocol (e.g.,
"http://localhost:5173"); choose one approach and apply it consistently across
all cy.visit calls in this file.
In `@search.html`:
- Around line 22-27: In search.html replace hard-coded absolute asset paths for
the logo and search icon (the <img class="logo" src="/images/logo.png" ...> and
the <img src="/svg/search.svg" ...> used inside the .search-form/.search-submit)
so they respect the app base URL; use the existing %BASE_URL% prefix (e.g.,
%BASE_URL%images/logo.png and %BASE_URL%svg/search.svg) or convert them to
relative paths instead, ensuring both src attributes are updated so assets
resolve when deployed to a subdirectory.
- Line 49: Update the module script tag's src from an absolute path to a
relative path so Vite's base option is respected: change the script element that
currently uses "/src/page/search.ts" to use "./src/page/search.ts" (and apply
the same fix to the similar script tag in index.html); locate the <script
type="module" ...> entry in search.html (and the matching one in index.html) and
replace the leading slash with "./" so assets resolve correctly in production.
In `@src/api.ts`:
- Around line 5-27: The fetcher function creates timeoutPromise unconditionally,
which causes an unhandled rejection when timeoutMs is falsy; change the logic in
fetcher so the timeout promise (and its setTimeout) is only created when
timeoutMs is truthy (e.g., guard creation of timeoutPromise behind if
(timeoutMs) or build the PromiseRace array conditionally), and ensure you clear
the timer when the response resolves to avoid leaks (reference: fetcher,
timeoutPromise, timeoutMs, response).
In `@src/event/bindSelectRatingEvent.ts`:
- Around line 8-10: The code calls event.target.closest(...) for dialogElement
and formElement without null checks, which can cause runtime errors when either
is null; update the handler (the function containing the lines that reference
dialogElement and formElement) to guard both values immediately after obtaining
them (e.g., if (!dialogElement || !formElement) return or handle the error),
only access dialogElement.dataset.movieId and formElement thereafter, and also
keep the existing movieIdStr validation after these null checks to ensure no
dereference occurs when closest() returns null.
In `@src/observer/bindTopInfiniteScrollObserver.ts`:
- Around line 3-14: The current throttle function does not await async callbacks
and so can allow concurrent network requests from renderSearchMoviePage and
renderPopularMoviePage; change the approach so the throttled wrapper waits for
the returned Promise before allowing the next call (or alternatively use an
external in-flight flag inside the IntersectionObserver callback), e.g., update
throttle to treat callback as potentially async by storing the Promise in a
local inFlight variable and only clearing it when the Promise resolves/rejects,
or add an inFlight boolean around the IntersectionObserver handler that
early-returns while a previous renderSearchMoviePage/renderPopularMoviePage
Promise is pending.
In `@src/render/renderLoadMoreTopButton.ts`:
- Around line 9-19: The existing renderLoadMoreTopButton reuses
loadMoreTopButtonElement without updating its click handler, so stale closure
callbacks can run; in renderLoadMoreTopButton ensure the element's listener is
synchronized by removing the old handler and adding the new one (e.g., track the
previous handler reference on the element or in a module-scoped variable and
call removeEventListener(previousHandler) before addEventListener(callback)), or
switch to event delegation on a parent element; update references to
loadMoreTopButtonElement and callback accordingly so the active handler always
matches the latest callback.
In `@src/render/renderModal.ts`:
- Around line 10-47: The current use of dialogElement.insertAdjacentHTML to
inject external data (movieDetail.title, movieDetail.genres,
movieDetail.overview and inline onerror image src) opens an XSS vector; change
the implementation in renderModal by creating the modal DOM structure via DOM
APIs (createElement/appendChild or template cloning) and then set textContent
and safe attribute properties for poster src, alt, and ratings instead of
interpolating into a string; populate genre list using safe joins into
textContent, attach the image error handler via addEventListener('error', ...)
rather than inline onerror, and ensure values like movieDetail.title,
movieDetail.overview, and poster_path are assigned via
element.textContent/setAttribute to sanitize output.
In `@src/render/renderMovieItems.ts`:
- Around line 11-25: The current use of liElement.insertAdjacentHTML with a
template string inserts untrusted values like movie.title directly into HTML
causing XSS/markup breakage; replace this block in renderMovieItems (the code
that calls liElement.insertAdjacentHTML) with explicit DOM construction: create
elements (img.thumbnail, div.item-desc, p.rate, img.star, span, p.movie-title),
set attributes via setAttribute or property assignment (src using
import.meta.env.VITE_IMAGE_BASE_URL + '/w300' + movie.poster_path, alt,
className), and assign movie.title and vote_average into textContent (not
innerHTML) to ensure values are escaped and safe. Ensure the onerror fallback is
set as a function or attribute safely (not by injecting raw HTML) and that
movie.vote_average is formatted with toFixed(1) before assigning to textContent.
In `@src/styles/modal.css`:
- Around line 121-124: The CSS uses an absolute path in modal.css
(background-image: url('/images/star_sprites.png')) which will break when the
app is hosted in a subdirectory; change it to a path that respects BASE_URL —
either make it relative (background-image: url('images/star_sprites.png')) or,
if you need dynamic BASE_URL, expose import.meta.env.BASE_URL from JS into a CSS
custom property (e.g., set
document.documentElement.style.setProperty('--base-url',
import.meta.env.BASE_URL) in your bootstrap and then use background-image:
url(var(--base-url) 'images/star_sprites.png')) so the sprite URL matches how
renderModal.ts and renderMovieItems.ts build image paths.
In `@src/toast.ts`:
- Around line 7-13: Replace the unsafe innerHTML assignment on the toast element
by creating DOM nodes and assigning textContent to avoid XSS: instead of setting
toast.innerHTML with template strings that interpolate title and message, create
a container DIV, create two P elements (with class names "toast-title" and
"toast-message") and set their textContent to the title and message variables,
and create the close BUTTON (class "toast-close", aria-label "닫기") then append
them to toast; update any code that queries or styles these nodes to use the
same class names so behavior remains unchanged.
In `@vite.config.js`:
- Around line 1-13: vite.config.js uses __dirname but the file is an ES module
so __dirname is undefined; add imports for fileURLToPath and dirname and define
__dirname from import.meta.url before using it: import fileURLToPath from 'url'
and dirname from 'path' (or the equivalent named imports), then set const
__dirname = dirname(fileURLToPath(import.meta.url)); place this immediately
after the existing import lines so the resolve(...) calls (the
build.rollupOptions.input keys) can use the now-defined __dirname.
---
Minor comments:
In `@README.md`:
- Line 106: Fix the typos in the README sentence: replace "localStorag" with
"localStorage" and change "구현하돼" to "구현하되" so the line reads correctly; locate
the sentence in README.md containing "localStorag로 구현하돼 서버 API로 쉽고 안전하게 갈아끼울 수
있는 구조로 개발하여야한다." and apply those two replacements.
In `@src/error.ts`:
- Around line 70-77: When handling APIError in src/error.ts (the block using
APIError, error.response.json(), body and bodyStr), make JSON parsing robust by
wrapping the call to error.response.json() in a try/catch and falling back to
error.response.text() if json parsing fails or throws; normalize the resulting
body value so that if it's an object you convert it via JSON.stringify (with a
try/catch and a sensible small-length fallback like "<<non-serializable body>>")
and if it’s not a string ensure you coerce to a readable string; then build
message from that safe body string (with a max length if desired) before calling
showErrorToast so the toast always receives a human-readable message instead of
"[object Object]" or blowing up on invalid JSON.
In `@src/event/bindSelectRatingEvent.ts`:
- Line 9: The type assertion on formElement is incorrect: change the cast of the
result of event.target.closest(".modal-movie-my-rating-selector") from
HTMLFormElement to a more accurate type (e.g., HTMLDivElement or HTMLElement)
and update any downstream assumptions in bindSelectRatingEvent that rely on
form-specific APIs (replace form-only methods or guard them with appropriate
checks) so interactions with formElement use properties/methods valid for a
div/HTMLElement.
In `@src/page/popular.ts`:
- Around line 9-11: The catch block currently calls handleError(error) then
removeSkeletonItems(), which can be skipped if handleError throws; update the
error handling so cleanup always runs: either wrap handleError(error) in its own
try/catch or move removeSkeletonItems() into a finally block that runs
regardless of handleError outcome; also inspect the handleError implementation
to confirm it already swallows internal parsing/formatting errors (check the
handleError function) and if not, add internal try/catch there to prevent
rethrows.
In `@src/render/renderPopularMoviePage.ts`:
- Line 33: 검증 누락으로 response.results가 빈 배열일 때 response.results[0]이 undefined가 되어
renderTopRatedMovie에 잘못 전달됩니다; 호출부에서 isTopPage &&
renderTopRatedMovie(response.results[0]); 대신 response.results가 배열인지와 길이 > 0인지를
확인한 뒤 renderTopRatedMovie를 호출하도록 수정하세요 (예: check Array.isArray(response.results)
&& response.results.length > 0), 대상 식별자: response.results, renderTopRatedMovie,
isTopPage, Movie.
In `@src/render/renderSearchListEmptyAlert.ts`:
- Line 1: Rename the misspelled function createSearchListrEmptyAlertTemplate to
createSearchListEmptyAlertTemplate, update its declaration and every reference
(calls, exports, imports, tests) to the new name, and ensure any exported symbol
or default export that used the old name is updated to match.
In `@src/styles/main.css`:
- Around line 54-57: `#wrap에` 고정된 min-width: 1440px가 반응형 미디어 쿼리와 충돌해 작은 화면에서 가로
스크롤을 유발합니다; 이 문제를 해결하려면 `#wrap의` min-width 속성을 제거하거나 미디어 쿼리(현재 작은 화면 처리를 담당하는 규칙)
내부에서만 적용하도록 옮기고, 대신 필요하면 max-width 또는 width: 100%를 사용해 컨테이너가 뷰포트 이하로 줄어들 수 있게 하며
모바일/태블릿 크기에서 레이아웃을 깨지 않도록 해당 미디어 쿼리에서 min-width를 재정의하거나 제거하세요.
In `@src/styles/modal.css`:
- Around line 148-155: The selector list for
.modal-movie-my-rating-selector[data-rating="10"] contains a duplicated entry
button:nth-of-type(1); remove the duplicate selector (or, if the intent was to
target a different button, replace the duplicate with the correct nth-of-type
index) so the rules only include each button selector once (refer to
.modal-movie-my-rating-selector and the data-rating="10" selector to locate the
block).
In `@src/styles/tab.css`:
- Around line 8-20: 스타일 규칙 위반은 .tab-item 블록 내부의 선언 앞에 불필요한 빈 줄이 있는 것입니다;
.tab-item 선택자 내에서 display, align-items, justify-content 등 선언들 사이의 빈 줄(현재 Lines
12, 18에 해당)을 제거해 선언이 연속되도록 정렬하고 프로젝트 스타일 가이드에 맞춰 공백을 없애세요.
In `@src/styles/thumbnail.css`:
- Around line 77-87: The CSS currently applies both gap: 4px on .item-desc and
margin-bottom: 4px on .item-desc>*:not(:last-child), causing doubled spacing;
choose one approach and remove the other to avoid the duplicate 8px
spacing—either delete margin-bottom from .item-desc>*:not(:last-child) and rely
on .item-desc { gap: 4px; } for consistent flex gaps, or remove gap and keep
margin-bottom for manual control; update the rule(s) for .item-desc and
.item-desc>*:not(:last-child) accordingly.
---
Nitpick comments:
In `@src/api.ts`:
- Around line 65-88: The function updateMyRating currently proceeds to call
localStorage.setItem even when RATING_OPTIONS.includes(rating) is false, causing
a no-op write; change updateMyRating to early-return when the provided rating is
not in RATING_OPTIONS (e.g., check RATING_OPTIONS.includes(rating) near the top
and return immediately) so you don't perform unnecessary storage writes, and
update the JSDoc for updateMyRating to state that invalid ratings are ignored
(or alternatively throw a ValidationError instead of returning if you prefer
strict behavior).
In `@src/constants.ts`:
- Around line 11-19: Update the constants to be readonly literal types by
appending "as const" to both RATING_OPTIONS and RATING_MESSAGES so TypeScript
infers them as tuple/readonly literal mappings rather than loose number[] and
string index types; modify the declarations for RATING_OPTIONS and
RATING_MESSAGES accordingly (refer to the RATING_OPTIONS and RATING_MESSAGES
symbols) so downstream code benefits from stronger type safety.
In `@src/error.ts`:
- Around line 30-48: The ParseError constructor currently embeds the full
context.raw into the error message which can be extremely large; update the
constructor in class ParseError to truncate context.raw to a safe length (e.g.,
200 chars) or replace it with a summarized snippet/placeholder before calling
super, e.g., create a local shortRaw from context.raw (handling null/undefined)
and use that in the super message and/or as an additional property so the full
raw is not in log messages by default; keep the original raw in the public
readonly raw field unchanged if you want to preserve full data but avoid
printing it in the error message.
In `@src/event/bindClickMovieEvent.ts`:
- Around line 11-13: The current check uses if (!movieId) which treats 0 as
invalid and can be ambiguous; in bindClickMovieEvent replace that falsy check
with an explicit NaN check using Number.isNaN(movieId) (and optionally also
guard an empty movieIdStr if needed) so only truly non-numeric values cause the
throw in the block that references movieId and movieIdStr.
In `@src/observer/bindBottomInfiniteScrollObserver.ts`:
- Around line 3-15: There is a duplicated throttle implementation used by
bindBottomInfiniteScrollObserver and bindTopInfiniteScrollObserver; extract the
shared function into a single utility module (e.g., create a util file exporting
throttle) and update both bindBottomInfiniteScrollObserver and
bindTopInfiniteScrollObserver to import and use that exported throttle
implementation; ensure the exported function preserves the same signature
(throttle<T extends (...args: any[]) => void>(callback: T, ms: number)) and
behavior (single timer, immediate call, reset after ms).
In `@src/render/renderMyRatingSelector.ts`:
- Around line 5-7: The variable name formElement in createMyRateSelectorElement
is misleading because it holds a div; rename it (e.g., to divElement or
wrapperElement) and update all usages inside that function to match, and remove
any incorrect HTMLFormElement type assertions or casts so the element is treated
as an HTMLDivElement where needed; ensure event listeners and DOM manipulations
reference the new variable name and correct element type
(createMyRateSelectorElement, formElement).
In `@src/render/renderPopularMoviePage.ts`:
- Around line 14-60: renderPopularMoviePage and renderSearchMoviePage duplicate
page-state management, skeleton rendering, observer binding and error handling;
refactor by extracting shared behavior into a reusable helper (e.g., a new
function like renderPaginatedMovies or managePaginatedRender) that accepts the
endpoint/loader callback, page number, direction and a top-item renderer
callback, then update renderPopularMoviePage to call that helper (preserving
unique bits such as renderTopRatedMovie and fetchMovies call); ensure the helper
encapsulates firstLoadedPage/lastLoadedPage updates,
renderSkeletonItemsToList/removeSkeletonItems, restoreScrollPosition, and
bindTopInfiniteScrollObserver/bindBottomInfiniteScrollObserver wiring so both
renderPopularMoviePage and renderSearchMoviePage delegate common flow while
keeping their specific differences.
In `@src/render/renderSearchListEmptyAlert.ts`:
- Around line 15-21: renderSearchListEmptyAlert may insert duplicate alerts when
called repeatedly; before inserting the template from
createSearchListrEmptyAlertTemplate(), check for an existing element with the
'.empty-list-alert' selector (e.g., via
document.querySelector('.empty-list-alert')) and remove it or early-return if
present, or alternatively remove any existing alert unconditionally before
calling listEl.insertAdjacentHTML; update the logic around
listEl.children.length and the else branch to ensure a single alert element is
maintained.
In `@src/render/renderSearchMoviePage.ts`:
- Around line 51-58: The call to bindBottomInfiniteScrollObserver(...) is
missing a trailing semicolon which breaks consistency; update the statement that
starts with bindBottomInfiniteScrollObserver(hasNextPage, async () => { ... })
to end with a semicolon so the call is terminated properly (locate the call in
src/render/renderSearchMoviePage.ts near the existing async callback and add the
semicolon after the closing parenthesis).
- Line 38: The hasPrevPage assignment contains an unnecessary null check;
simplify the expression by removing the redundant "firstLoadedPage !== null" and
set hasPrevPage using only "firstLoadedPage > 1" (update the variable defined as
hasPrevPage that references firstLoadedPage accordingly).
In `@src/render/restoreScrollPosition.ts`:
- Around line 6-10: restoreScrollPosition currently uses setTimeout(...,0) which
can run before async rendering (e.g., renderSearchMoviePage,
renderPopularMoviePage, or renderMovieItems) finishes and the target
li[data-movie-id] exists; change restoreScrollPosition to wait for the element
instead of a zero-timeout: either make renderMovieItems (or the callers
renderSearchMoviePage/renderPopularMoviePage) return/await a Promise and call
await restoreScrollPosition after rendering completes, or implement a
short-element-wait inside restoreScrollPosition (poll with requestAnimationFrame
or use a MutationObserver watching the container until
document.querySelector(`li[data-movie-id="${targetMovieId}"]`) is present) and
then call scrollIntoView on the found element.
In `@src/styles/colors.css`:
- Around line 1-13: Two color variables are duplicated: --color-white and
--color-ffffff both equal `#ffffff`, and --color-bluegray-100 and --color-242a32
both equal `#242a32`; consolidate by removing one of each pair or make one an
alias to the other (e.g., set --color-ffffff to reference var(--color-white)
and/or set --color-242a32 to reference var(--color-bluegray-100)) so code uses a
single source of truth for those values and avoids redundant literals.
In `@src/styles/modal.css`:
- Around line 157-159: The selector .modal-movie-my-rating-message is defined
twice; consolidate them into a single declaration by removing the duplicate and
merging any differing properties so all styles for
.modal-movie-my-rating-message live in one place; update the remaining rule to
include any properties from the removed rule (preserve intent/ordering) and run
a quick style check to ensure no visual regressions.
In `@src/styles/skeleton.css`:
- Around line 1-6: Add a prefers-reduced-motion media query to disable or
simplify the .skeleton animation for users who request reduced motion: detect
the user's preference with `@media` (prefers-reduced-motion: reduce) and inside it
remove the animation on the .skeleton selector (or replace it with a
non-animating static background) and ensure any `@keyframes` shimmer is not
applied; update both the .skeleton rule and any related rules (e.g., the
`@keyframes` shimmer block) so the shimmer is suppressed when
prefers-reduced-motion is set.
In `@src/styles/tab.css`:
- Around line 19-27: The gradients in tab.css are using hardcoded hex colors for
the .tab-item.selected and .tab-item:hover (and the base background rule)
instead of the CSS variables from colors.css; update these rules to reference
the appropriate CSS custom properties (e.g., var(--background, ...),
var(--accent, ...), var(--hover, ...)) defined in colors.css, replacing the
literal hex values so the gradients use those variables and fallback values if
needed, ensuring selectors .tab-item.selected and .tab-item:hover are updated
accordingly.
In `@src/styles/toast.css`:
- Around line 40-52: .toast-close currently only styles hover, so keyboard users
can't see focus; add a :focus-visible rule for .toast-close to mirror hover
(e.g., set opacity: 1 and visible focus ring/outlines) and ensure it doesn't
conflict with existing styles; update the .toast-close:focus-visible selector to
provide a clear visible indicator (outline or box-shadow) and preserve
accessibility for keyboard navigation.
In `@src/url.ts`:
- Around line 28-33: The integer validation in getPage currently uses the
expression pageNum % 1 which works but is unclear; update getPage to use
Number.isInteger(pageNum) when deciding whether to fall back to 1 so the intent
is explicit (keep the existing getURLSearchParam("page", "1") and NaN check but
replace the pageNum % 1 condition with !Number.isInteger(pageNum) in the ternary
that computes page).
- Around line 1-4: getURLSearchParam's signature wrongly implies it returns T
but urlParams.get(name) yields string | null, so update the function signature
and return type to reflect actual values (e.g. export function
getURLSearchParam<T>(name: string, defaultValue: T): string | T) or simplify by
constraining defaultValue to string and returning string only; modify call sites
like getPage and getQuery if needed to match the chosen contract and ensure
callers handle string | T correctly.
🪄 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: 7b3ec77a-9f2f-4801-9654-310428a9d73e
⛔ Files ignored due to path filters (12)
package-lock.jsonis excluded by!**/package-lock.jsonpublic/favicon.icois excluded by!**/*.icopublic/images/default_movie_image.pngis excluded by!**/*.pngpublic/images/logo.pngis excluded by!**/*.pngpublic/images/modal_button_close.pngis excluded by!**/*.pngpublic/images/star_empty.pngis excluded by!**/*.pngpublic/images/star_filled.pngis excluded by!**/*.pngpublic/images/star_sprites.pngis excluded by!**/*.pngpublic/images/woowacourse_logo.pngis excluded by!**/*.pngpublic/svg/planet.svgis excluded by!**/*.svgpublic/svg/search.svgis excluded by!**/*.svgpublic/svg/x.svgis excluded by!**/*.svg
📒 Files selected for processing (53)
.gitignoreREADME.mdcypress/e2e/spec.cy.tscypress/fixtures/empty-search.jsoncypress/fixtures/movie-detail.jsoncypress/fixtures/popular-movies-p1.jsoncypress/fixtures/popular-movies-p2.jsoncypress/fixtures/search-results.jsoncypress/support/commands.tscypress/support/e2e.tscypress/tsconfig.jsonindex.htmlpackage.jsonsearch.htmlsrc/api.tssrc/constants.tssrc/error.tssrc/event/bindClickMovieEvent.tssrc/event/bindSelectRatingEvent.tssrc/main.tssrc/observer/bindBottomInfiniteScrollObserver.tssrc/observer/bindTopInfiniteScrollObserver.tssrc/observer/bindVisibleMovieObserver.tssrc/page/popular.tssrc/page/search.tssrc/render/renderLoadMoreTopButton.tssrc/render/renderModal.tssrc/render/renderMovieItems.tssrc/render/renderMyRatingSelector.tssrc/render/renderPopularMoviePage.tssrc/render/renderSearchInput.tssrc/render/renderSearchListEmptyAlert.tssrc/render/renderSearchListTitle.tssrc/render/renderSearchMoviePage.tssrc/render/renderSkeletonItems.tssrc/render/renderTopRatedMovie.tssrc/render/restoreScrollPosition.tssrc/styles/colors.csssrc/styles/index.csssrc/styles/main.csssrc/styles/modal.csssrc/styles/reset.csssrc/styles/skeleton.csssrc/styles/tab.csssrc/styles/thumbnail.csssrc/styles/toast.csssrc/toast.tssrc/type.tssrc/url.tstemplates/styles/thumbnail.csstsconfig.jsonvite-env.d.tsvite.config.js
💤 Files with no reviewable changes (1)
- src/main.ts
Comment on lines
19
to
+20
| cy.visit("localhost:5173"); | ||
| cy.wait("@popularMovies"); |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
fd -HI 'cypress\.config\.(ts|js|mjs|cjs)$' | while read -r file; do
echo "### $file"
sed -n '1,200p' "$file"
done
echo
echo "### cy.visit 사용 위치"
rg -n 'cy\.visit\(' cypressRepository: woowacourse/javascript-movie-review
Length of output: 840
cy.visit()에 프로토콜이 없는 URL을 넘기고 있습니다.
cypress.config.ts에 baseUrl이 정의되지 않았는데, cy.visit("localhost:5173")처럼 프로토콜 없이 호출하면 상대 경로로 해석돼 테스트가 실패합니다.
두 가지 방법으로 수정할 수 있습니다:
cypress.config.ts에baseUrl: "http://localhost:5173"을 추가하고cy.visit("/")나cy.visit("/?page=2")처럼 호출- 모든
cy.visit()호출을cy.visit("http://localhost:5173")처럼 절대 URL(프로토콜 포함)로 변경
지금 패턴이 이 파일 전반에 반복되고 있으니, 어느 접근이 프로젝트에 맞는지 판단한 후 일관되게 적용해 주세요.
Also applies to: 96-97, 107-107, 162-163, 172-173, 177-178, 183-184, 193-194
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@cypress/e2e/spec.cy.ts` around lines 19 - 20, The cy.visit calls in this spec
use URLs without a protocol (e.g., cy.visit("localhost:5173")), which are
treated as relative because cypress.config.ts has no baseUrl; either add
baseUrl: "http://localhost:5173" to cypress.config.ts and change calls like
cy.visit("localhost:5173") to cy.visit("/") or cy.visit("/?page=2"), or update
every cy.visit(...) in this file (including the occurrences paired with
cy.wait("@popularMovies") and other visits at the lines cited) to use absolute
URLs with protocol (e.g., "http://localhost:5173"); choose one approach and
apply it consistently across all cy.visit calls in this file.
Comment on lines
+22
to
+27
| <img class="logo" src="/images/logo.png" alt="MovieList" /> | ||
| </a> | ||
| <form method="get" action="%BASE_URL%search" class="search-form"> | ||
| <input type="text" name="query" class="search-input" placeholder="검색어를 입력하세요" required /> | ||
| <button type="submit" class="search-submit"> | ||
| <img src="/svg/search.svg" alt="search" /> |
There was a problem hiding this comment.
배포 시 경로 문제: 절대 경로가 서브디렉토리에서 동작하지 않습니다.
/images/logo.png, /svg/search.svg 같은 절대 경로는 --base=/javascript-movie-review/로 배포할 때 올바르게 해석되지 않습니다. Line 21, 24처럼 %BASE_URL%를 사용하거나 상대 경로를 사용해야 합니다.
🐛 경로 수정 제안
<a href="%BASE_URL%">
- <img class="logo" src="/images/logo.png" alt="MovieList" />
+ <img class="logo" src="%BASE_URL%images/logo.png" alt="MovieList" />
</a>
<form method="get" action="%BASE_URL%search" class="search-form">
<input type="text" name="query" class="search-input" placeholder="검색어를 입력하세요" required />
<button type="submit" class="search-submit">
- <img src="/svg/search.svg" alt="search" />
+ <img src="%BASE_URL%svg/search.svg" alt="search" />
</button>
</form>📝 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
| <img class="logo" src="/images/logo.png" alt="MovieList" /> | |
| </a> | |
| <form method="get" action="%BASE_URL%search" class="search-form"> | |
| <input type="text" name="query" class="search-input" placeholder="검색어를 입력하세요" required /> | |
| <button type="submit" class="search-submit"> | |
| <img src="/svg/search.svg" alt="search" /> | |
| <img class="logo" src="%BASE_URL%images/logo.png" alt="MovieList" /> | |
| </a> | |
| <form method="get" action="%BASE_URL%search" class="search-form"> | |
| <input type="text" name="query" class="search-input" placeholder="검색어를 입력하세요" required /> | |
| <button type="submit" class="search-submit"> | |
| <img src="%BASE_URL%svg/search.svg" alt="search" /> |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@search.html` around lines 22 - 27, In search.html replace hard-coded absolute
asset paths for the logo and search icon (the <img class="logo"
src="/images/logo.png" ...> and the <img src="/svg/search.svg" ...> used inside
the .search-form/.search-submit) so they respect the app base URL; use the
existing %BASE_URL% prefix (e.g., %BASE_URL%images/logo.png and
%BASE_URL%svg/search.svg) or convert them to relative paths instead, ensuring
both src attributes are updated so assets resolve when deployed to a
subdirectory.
| <p><img src="./images/woowacourse_logo.png" alt="woowacourse" width="180" /></p> | ||
| </footer> | ||
| </div> | ||
| <script type="module" src="/src/page/search.ts"></script> |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Description: index.html과 비교하여 스크립트 경로 패턴 확인
echo "=== search.html script path ==="
grep -n "script.*src=" search.html
echo ""
echo "=== index.html script path (for comparison) ==="
grep -n "script.*src=" index.html 2>/dev/null || echo "index.html not found"Repository: woowacourse/javascript-movie-review
Length of output: 286
🏁 Script executed:
# Check for Vite configuration files
find . -maxdepth 2 -type f \( -name "vite.config.*" -o -name "package.json" \) | head -20Repository: woowacourse/javascript-movie-review
Length of output: 110
🏁 Script executed:
# Check the project structure to understand the deployment/build setup
fd -H "vite.config|tsconfig|package.json" --max-one-result --type fRepository: woowacourse/javascript-movie-review
Length of output: 303
🏁 Script executed:
# Examine package.json to understand the build configuration
cat package.json | head -50Repository: woowacourse/javascript-movie-review
Length of output: 1206
🏁 Script executed:
# Read vite configuration
cat vite.config.jsRepository: woowacourse/javascript-movie-review
Length of output: 366
🏁 Script executed:
# Check both HTML files for full context on script paths
echo "=== search.html (lines 40-55) ===" && sed -n '40,55p' search.html && echo "" && echo "=== index.html (lines 55-75) ===" && sed -n '55,75p' index.htmlRepository: woowacourse/javascript-movie-review
Length of output: 1129
모듈 스크립트의 절대 경로를 상대 경로로 수정해야 합니다.
/src/page/search.ts와 같은 절대 경로는 Vite 빌드 시 --base=/javascript-movie-review/ 옵션에서 제대로 처리되지 않습니다. 프로덕션 환경에서 스크립트가 도메인 루트에서 로드되어 404 오류가 발생합니다. ./src/page/search.ts로 변경하세요. (index.html도 동일한 패턴을 사용 중입니다.)
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@search.html` at line 49, Update the module script tag's src from an absolute
path to a relative path so Vite's base option is respected: change the script
element that currently uses "/src/page/search.ts" to use "./src/page/search.ts"
(and apply the same fix to the similar script tag in index.html); locate the
<script type="module" ...> entry in search.html (and the matching one in
index.html) and replace the leading slash with "./" so assets resolve correctly
in production.
Comment on lines
+5
to
+27
| function fetcher<T>(url: string, { timeoutMs, ...options }: RequestInit & { timeoutMs?: number }) { | ||
| const response = fetch(url, options).then(async res => { | ||
| if (!res.ok) { | ||
| throw new APIError(res); | ||
| } | ||
|
|
||
| const text = await res.text(); | ||
|
|
||
| try { | ||
| return JSON.parse(text) as T; | ||
| } catch (cause) { | ||
| throw new ParseError({ type: "json", sourceName: "API 응답", raw: text }, cause); | ||
| } | ||
| }) | ||
|
|
||
| const timeoutPromise = new Promise<never>((_, reject) => { | ||
| setTimeout(() => { | ||
| reject(new TimeoutError(url, timeoutMs!)); | ||
| }, timeoutMs); | ||
| }) | ||
|
|
||
| return timeoutMs ? Promise.race([response, timeoutPromise]) : response | ||
| } |
There was a problem hiding this comment.
timeoutMs가 falsy일 때 미처리 Promise rejection 발생 가능성
timeoutPromise가 조건과 무관하게 항상 생성되고 있습니다. timeoutMs가 undefined인 경우:
setTimeout(..., undefined)는 즉시(0ms처럼) 실행됨- 하지만 line 26에서
response만 반환되므로timeoutPromise의 rejection이 처리되지 않음
이 구조가 어떤 문제를 야기할 수 있는지 생각해보셨나요? timeoutMs가 truthy일 때만 timeout 로직이 실행되도록 구조를 변경하는 것을 고려해보세요.
💡 힌트: 조건부 timeout 생성
- const timeoutPromise = new Promise<never>((_, reject) => {
- setTimeout(() => {
- reject(new TimeoutError(url, timeoutMs!));
- }, timeoutMs);
- })
-
- return timeoutMs ? Promise.race([response, timeoutPromise]) : response
+ if (!timeoutMs) {
+ return response;
+ }
+
+ const timeoutPromise = new Promise<never>((_, reject) => {
+ setTimeout(() => {
+ reject(new TimeoutError(url, timeoutMs));
+ }, timeoutMs);
+ });
+
+ return Promise.race([response, timeoutPromise]);📝 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
| function fetcher<T>(url: string, { timeoutMs, ...options }: RequestInit & { timeoutMs?: number }) { | |
| const response = fetch(url, options).then(async res => { | |
| if (!res.ok) { | |
| throw new APIError(res); | |
| } | |
| const text = await res.text(); | |
| try { | |
| return JSON.parse(text) as T; | |
| } catch (cause) { | |
| throw new ParseError({ type: "json", sourceName: "API 응답", raw: text }, cause); | |
| } | |
| }) | |
| const timeoutPromise = new Promise<never>((_, reject) => { | |
| setTimeout(() => { | |
| reject(new TimeoutError(url, timeoutMs!)); | |
| }, timeoutMs); | |
| }) | |
| return timeoutMs ? Promise.race([response, timeoutPromise]) : response | |
| } | |
| function fetcher<T>(url: string, { timeoutMs, ...options }: RequestInit & { timeoutMs?: number }) { | |
| const response = fetch(url, options).then(async res => { | |
| if (!res.ok) { | |
| throw new APIError(res); | |
| } | |
| const text = await res.text(); | |
| try { | |
| return JSON.parse(text) as T; | |
| } catch (cause) { | |
| throw new ParseError({ type: "json", sourceName: "API 응답", raw: text }, cause); | |
| } | |
| }) | |
| if (!timeoutMs) { | |
| return response; | |
| } | |
| const timeoutPromise = new Promise<never>((_, reject) => { | |
| setTimeout(() => { | |
| reject(new TimeoutError(url, timeoutMs)); | |
| }, timeoutMs); | |
| }); | |
| return Promise.race([response, timeoutPromise]); | |
| } |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/api.ts` around lines 5 - 27, The fetcher function creates timeoutPromise
unconditionally, which causes an unhandled rejection when timeoutMs is falsy;
change the logic in fetcher so the timeout promise (and its setTimeout) is only
created when timeoutMs is truthy (e.g., guard creation of timeoutPromise behind
if (timeoutMs) or build the PromiseRace array conditionally), and ensure you
clear the timer when the response resolves to avoid leaks (reference: fetcher,
timeoutPromise, timeoutMs, response).
Comment on lines
+8
to
+10
| const dialogElement = event.target.closest("dialog") as HTMLDialogElement; | ||
| const formElement = event.target.closest(".modal-movie-my-rating-selector") as HTMLFormElement; | ||
| const movieIdStr = dialogElement.dataset.movieId; |
There was a problem hiding this comment.
closest() 반환값의 null 체크가 누락되어 있습니다.
dialogElement나 formElement가 null일 경우 Line 10, 19에서 런타임 에러가 발생합니다. movieIdStr 검증(Line 13) 전에 null 역참조가 일어날 수 있습니다.
🐛 null 안전성 개선 제안
try {
if (!(event.target instanceof HTMLButtonElement)) return
const dialogElement = event.target.closest("dialog") as HTMLDialogElement;
- const formElement = event.target.closest(".modal-movie-my-rating-selector") as HTMLFormElement;
+ const formElement = event.target.closest(".modal-movie-my-rating-selector") as HTMLElement;
+
+ if (!dialogElement || !formElement) return;
+
const movieIdStr = dialogElement.dataset.movieId;
const ratingStr = event.target.dataset.rating;📝 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 dialogElement = event.target.closest("dialog") as HTMLDialogElement; | |
| const formElement = event.target.closest(".modal-movie-my-rating-selector") as HTMLFormElement; | |
| const movieIdStr = dialogElement.dataset.movieId; | |
| const dialogElement = event.target.closest("dialog") as HTMLDialogElement; | |
| const formElement = event.target.closest(".modal-movie-my-rating-selector") as HTMLElement; | |
| if (!dialogElement || !formElement) return; | |
| const movieIdStr = dialogElement.dataset.movieId; |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/event/bindSelectRatingEvent.ts` around lines 8 - 10, The code calls
event.target.closest(...) for dialogElement and formElement without null checks,
which can cause runtime errors when either is null; update the handler (the
function containing the lines that reference dialogElement and formElement) to
guard both values immediately after obtaining them (e.g., if (!dialogElement ||
!formElement) return or handle the error), only access
dialogElement.dataset.movieId and formElement thereafter, and also keep the
existing movieIdStr validation after these null checks to ensure no dereference
occurs when closest() returns null.
Comment on lines
+10
to
+47
| dialogElement.insertAdjacentHTML('afterbegin', /* html */` | ||
| <form method="dialog"> | ||
| <button class="modal-close-button"> | ||
| <img src="${import.meta.env.BASE_URL}svg/x.svg" alt="close button" /> | ||
| </button> | ||
| </form> | ||
| <div class="modal-body"> | ||
| <div class="modal-movie-poster"> | ||
| <img | ||
| src="${import.meta.env.VITE_IMAGE_BASE_URL}/w400${movieDetail.poster_path}" | ||
| onerror="this.src='${import.meta.env.BASE_URL}images/default_movie_image.png'" | ||
| alt="${movieDetail.title}" | ||
| /> | ||
| </div> | ||
| <div class="modal-movie-content"> | ||
| <div class="modal-movie-header"> | ||
| <p class="modal-movie-title">${movieDetail.title}</p> | ||
| <p class="modal-movie-info">${movieDetail.release_date.split("-")[0]} · ${movieDetail.genres.map(genre => genre.name).join(", ")}</p> | ||
| <p class="modal-movie-rate"> | ||
| <span>평균</span> | ||
| <span> | ||
| <img src="${import.meta.env.BASE_URL}images/star_filled.png" alt="filled star" class="star" /> | ||
| <span class="modal-movie-rate-value">${movieDetail.vote_average.toFixed(1)}</span> | ||
| </span> | ||
| </p> | ||
| </div> | ||
| <div class="modal-movie-my-rating"> | ||
| <h3 class="modal-movie-content-title">내 평점</h3> | ||
| <div class="modal-movie-my-rating-body"> | ||
| </div> | ||
| </div> | ||
| <div class="modal-movie-plot"> | ||
| <h3 class="modal-movie-content-title">줄거리</h3> | ||
| <div class="modal-movie-plot-body">${movieDetail.overview}</div> | ||
| </div> | ||
| </div> | ||
| </div> | ||
| `) |
There was a problem hiding this comment.
외부 API 값을 그대로 insertAdjacentHTML()에 넣으면 XSS 경로가 열립니다.
movieDetail.title, genres, overview를 HTML 문자열에 바로 섞고 있고, 인라인 onerror도 같이 넣고 있습니다. 응답값에 태그/속성이 섞이면 모달 DOM이 그대로 오염될 수 있어서, 이 구간은 마크업 골격만 만들고 텍스트/속성은 DOM API로 채우는 쪽이 안전합니다.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/render/renderModal.ts` around lines 10 - 47, The current use of
dialogElement.insertAdjacentHTML to inject external data (movieDetail.title,
movieDetail.genres, movieDetail.overview and inline onerror image src) opens an
XSS vector; change the implementation in renderModal by creating the modal DOM
structure via DOM APIs (createElement/appendChild or template cloning) and then
set textContent and safe attribute properties for poster src, alt, and ratings
instead of interpolating into a string; populate genre list using safe joins
into textContent, attach the image error handler via addEventListener('error',
...) rather than inline onerror, and ensure values like movieDetail.title,
movieDetail.overview, and poster_path are assigned via
element.textContent/setAttribute to sanitize output.
Comment on lines
+11
to
+25
| liElement.insertAdjacentHTML('beforeend', /*html*/` | ||
| <img | ||
| class="thumbnail" | ||
| src="${import.meta.env.VITE_IMAGE_BASE_URL}/w300${movie.poster_path}" | ||
| onerror="this.src='${import.meta.env.BASE_URL}images/default_movie_image.png'" | ||
| alt="${movie.title}" | ||
| /> | ||
| <div class="item-desc"> | ||
| <p class="rate"> | ||
| <img src="${import.meta.env.BASE_URL}images/star_empty.png" alt="empty star" class="star" /> | ||
| <span>${movie.vote_average.toFixed(1)}</span> | ||
| </p> | ||
| <p class="movie-title">${movie.title}</p> | ||
| </div> | ||
| `) |
There was a problem hiding this comment.
리스트 카드도 외부 문자열을 그대로 HTML에 삽입하고 있습니다.
movie.title이 insertAdjacentHTML()의 텍스트/속성 값으로 바로 들어가므로, 응답값이 예상과 다르면 카드 마크업이 깨지거나 스크립트 주입 지점이 됩니다. 이 구간도 문자열 템플릿 대신 안전한 DOM 조립으로 바꾸는 편이 좋습니다.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/render/renderMovieItems.ts` around lines 11 - 25, The current use of
liElement.insertAdjacentHTML with a template string inserts untrusted values
like movie.title directly into HTML causing XSS/markup breakage; replace this
block in renderMovieItems (the code that calls liElement.insertAdjacentHTML)
with explicit DOM construction: create elements (img.thumbnail, div.item-desc,
p.rate, img.star, span, p.movie-title), set attributes via setAttribute or
property assignment (src using import.meta.env.VITE_IMAGE_BASE_URL + '/w300' +
movie.poster_path, alt, className), and assign movie.title and vote_average into
textContent (not innerHTML) to ensure values are escaped and safe. Ensure the
onerror fallback is set as a function or attribute safely (not by injecting raw
HTML) and that movie.vote_average is formatted with toFixed(1) before assigning
to textContent.
src/styles/modal.css
Outdated
Comment on lines
+121
to
+124
| background-image: url('/images/star_sprites.png'); | ||
| background-size: cover; | ||
| background-position: 0px 0px; | ||
| } |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Description: Check how other image paths are handled in CSS files
rg -n "url\(" --type=cssRepository: woowacourse/javascript-movie-review
Length of output: 156
🏁 Script executed:
# Check project structure and image locations
find . -type f -name "*.png" -o -name "*.jpg" -o -name "*.jpeg" -o -name "*.svg" | head -20Repository: woowacourse/javascript-movie-review
Length of output: 561
🏁 Script executed:
# Check for image references in JS files to see import.meta.env.BASE_URL usage
rg -n "import\.meta\.env\.BASE_URL" --type=js --type=tsRepository: woowacourse/javascript-movie-review
Length of output: 810
🏁 Script executed:
# Check if vite config exists and what it contains
find . -name "vite.config.*" -o -name "package.json" | head -10Repository: woowacourse/javascript-movie-review
Length of output: 110
🏁 Script executed:
# Check the images directory structure
find . -type d -name "images" -o -type d -name "img" -o -type d -name "assets"Repository: woowacourse/javascript-movie-review
Length of output: 113
🏁 Script executed:
# Check vite config to understand BASE_URL configuration
cat vite.config.jsRepository: woowacourse/javascript-movie-review
Length of output: 366
🏁 Script executed:
# Check if there are other references to star_sprites.png
rg -n "star_sprites" --type=css --type=ts --type=jsRepository: woowacourse/javascript-movie-review
Length of output: 156
🏁 Script executed:
# Check the CSS file context around the problematic line
sed -n '115,130p' src/styles/modal.cssRepository: woowacourse/javascript-movie-review
Length of output: 491
CSS 절대 경로 수정 필요
background-image: url('/images/star_sprites.png')에서 절대 경로를 사용하고 있으나, 다른 모든 이미지 요청(renderModal.ts, renderMovieItems.ts 등)은 ${import.meta.env.BASE_URL}images/... 패턴으로 처리됩니다.
GitHub Pages와 같이 서브디렉토리에 배포될 경우 절대 경로는 작동하지 않으므로, 상대 경로로 변경하거나 다른 방식으로 BASE_URL을 고려한 경로 처리가 필요합니다.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/styles/modal.css` around lines 121 - 124, The CSS uses an absolute path
in modal.css (background-image: url('/images/star_sprites.png')) which will
break when the app is hosted in a subdirectory; change it to a path that
respects BASE_URL — either make it relative (background-image:
url('images/star_sprites.png')) or, if you need dynamic BASE_URL, expose
import.meta.env.BASE_URL from JS into a CSS custom property (e.g., set
document.documentElement.style.setProperty('--base-url',
import.meta.env.BASE_URL) in your bootstrap and then use background-image:
url(var(--base-url) 'images/star_sprites.png')) so the sprite URL matches how
renderModal.ts and renderMovieItems.ts build image paths.
Comment on lines
+7
to
+13
| toast.innerHTML = /*html*/` | ||
| <div> | ||
| <p class="toast-title">${title}</p> | ||
| <p class="toast-message">${message}</p> | ||
| </div> | ||
| <button class="toast-close" aria-label="닫기">✕</button> | ||
| `; |
There was a problem hiding this comment.
XSS 취약점: innerHTML에 외부 데이터를 직접 삽입하고 있습니다.
정적 분석 도구가 지적한 것처럼, title과 message가 API 응답에서 올 수 있고(error.ts의 error.response.json() 참조), 이 데이터가 악의적인 스크립트를 포함할 수 있습니다. textContent 사용을 고려해보세요.
🛡️ 안전한 DOM 조작으로 수정 제안
function createToast({ title, message }: { title?: string, message?: string }): HTMLElement {
const toast = document.createElement("div");
toast.className = "toast";
- toast.innerHTML = /*html*/`
- <div>
- <p class="toast-title">${title}</p>
- <p class="toast-message">${message}</p>
- </div>
- <button class="toast-close" aria-label="닫기">✕</button>
- `;
+ const contentDiv = document.createElement("div");
+ const titleEl = document.createElement("p");
+ titleEl.className = "toast-title";
+ titleEl.textContent = title ?? "";
+ const messageEl = document.createElement("p");
+ messageEl.className = "toast-message";
+ messageEl.textContent = message ?? "";
+ contentDiv.append(titleEl, messageEl);
+
+ const closeBtn = document.createElement("button");
+ closeBtn.className = "toast-close";
+ closeBtn.setAttribute("aria-label", "닫기");
+ closeBtn.textContent = "✕";
+ closeBtn.addEventListener("click", () => removeToast(toast));
+
+ toast.append(contentDiv, closeBtn);
- const closeBtn = toast.querySelector<HTMLButtonElement>(".toast-close")!;
- closeBtn.addEventListener("click", () => removeToast(toast));
return toast;
}📝 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
| toast.innerHTML = /*html*/` | |
| <div> | |
| <p class="toast-title">${title}</p> | |
| <p class="toast-message">${message}</p> | |
| </div> | |
| <button class="toast-close" aria-label="닫기">✕</button> | |
| `; | |
| const contentDiv = document.createElement("div"); | |
| const titleEl = document.createElement("p"); | |
| titleEl.className = "toast-title"; | |
| titleEl.textContent = title ?? ""; | |
| const messageEl = document.createElement("p"); | |
| messageEl.className = "toast-message"; | |
| messageEl.textContent = message ?? ""; | |
| contentDiv.append(titleEl, messageEl); | |
| const closeBtn = document.createElement("button"); | |
| closeBtn.className = "toast-close"; | |
| closeBtn.setAttribute("aria-label", "닫기"); | |
| closeBtn.textContent = "✕"; | |
| closeBtn.addEventListener("click", () => removeToast(toast)); | |
| toast.append(contentDiv, closeBtn); |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/toast.ts` around lines 7 - 13, Replace the unsafe innerHTML assignment on
the toast element by creating DOM nodes and assigning textContent to avoid XSS:
instead of setting toast.innerHTML with template strings that interpolate title
and message, create a container DIV, create two P elements (with class names
"toast-title" and "toast-message") and set their textContent to the title and
message variables, and create the close BUTTON (class "toast-close", aria-label
"닫기") then append them to toast; update any code that queries or styles these
nodes to use the same class names so behavior remains unchanged.
Comment on lines
+1
to
+13
| import { defineConfig } from "vite"; | ||
| import { resolve } from "path"; | ||
|
|
||
| export default defineConfig({ | ||
| build: { | ||
| rollupOptions: { | ||
| input: { | ||
| main: resolve(__dirname, "index.html"), | ||
| search: resolve(__dirname, "search.html"), // search 페이지 추가 | ||
| }, | ||
| }, | ||
| }, | ||
| }); |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Description: Check if package.json has "type": "module" which would require ESM handling for __dirname
cat package.json | jq '.type'Repository: woowacourse/javascript-movie-review
Length of output: 87
🏁 Script executed:
cat -n vite.config.jsRepository: woowacourse/javascript-movie-review
Length of output: 457
__dirname을 ES 모듈 환경에서 정의해야 합니다
현재 프로젝트의 package.json에 "type": "module"이 설정되어 있습므로, 이 파일은 ES 모듈로 실행됩니다. 하지만 __dirname은 CommonJS 전역 변수로서 ES 모듈에서 기본적으로 제공되지 않습니다. 현재 코드에서 __dirname을 정의하지 않고 사용하고 있습니다.
이를 해결하려면 다음과 같이 수정해야 합니다:
import { fileURLToPath } from 'url';
import { dirname } from 'path';
const __dirname = dirname(fileURLToPath(import.meta.url));이 코드를 파일 상단(기존 import 문 아래)에 추가한 후 기존 코드를 유지하면 됩니다.
핵심 질문: 이 패턴이 왜 필요한지 이해해 보세요. import.meta.url은 현재 모듈의 URL을 나타내고, fileURLToPath는 이를 파일 경로로 변환합니다. dirname은 경로에서 디렉토리 부분만 추출합니다. 이 세 단계가 함께 ES 모듈에서 __dirname의 역할을 수행하는 방식을 생각해 보세요.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@vite.config.js` around lines 1 - 13, vite.config.js uses __dirname but the
file is an ES module so __dirname is undefined; add imports for fileURLToPath
and dirname and define __dirname from import.meta.url before using it: import
fileURLToPath from 'url' and dirname from 'path' (or the equivalent named
imports), then set const __dirname = dirname(fileURLToPath(import.meta.url));
place this immediately after the existing import lines so the resolve(...) calls
(the build.rollupOptions.input keys) can use the now-defined __dirname.
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
2 participants
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.
🎯 미션 소개
🕵️ 셀프 리뷰(Self-Review)
제출 전 체크 리스트
리뷰 요청 & 논의하고 싶은 내용
1) 이번 단계에서 가장 많이 고민했던 문제와 해결 과정에서 배운 점
에러 핸들링
[1단계 - 영화 목록 불러오기]에서 피드백 받았던 '에러를 제대로 핸들링하지 않거나, 너무 이르게 try/catch로 에러를 잡고 rethrow해 에러 정보가 유실되는 문제'를 해결하기 위해 다음과 같이 접근했습니다.
우선 fetcher에서 200번대가 아닌 응답을 받았을 때 아무 정보 없는 에러 객체를 던지던 것을, response 프로퍼티에 실패한 응답 결과를 담아 에러 객체가 맥락을 가질 수 있도록 수정했습니다. 그리고 나머지 에러들은 최대한 그대로 상위로 전파되도록 두고, 최상단의 try/catch에서 한 번에 처리하는 방향을 시도해보았는데요.
막상 이렇게 해보니, 최상단에서 에러를 잡았을 때 어디서 어떤 이유로 발생한 에러인지 구분하기 어렵다는 문제가 생겼습니다. 이를 해결하기 위해 고민하던 중 Error 객체의 cause 프로퍼티를 활용하는 방법을 알게되었는데요. 에러가 발생할 수 있는 지점에서 try/catch로 에러를 잡되, 기존 에러를 cause에 담아 다시 던지는 방식입니다.
이렇게 하니 기존 에러 객체를 보존하면서도, 최상단에서 에러를 처리할 때 발생 위치와 원인을 명확하게 파악할 수 있어 좋았습니다.
다만 여전히 문제가 있었습니다. 에러를 다시 던질 때 메시지를 직접 작성하다 보니, 자연스럽게 '화면에 어떤 메시지를 보여줄지'를 고민하며 메시지를 작성하게 되었던것인데요.
최종적으로 이를 해결하기 위해 기존 Error 객체를 상속받아, 에러 맥락과 기존 에러만 입력받고 메시지는 내부 규칙에 따라 생성하는 커스텀 Error 클래스를 만들어 사용하는 방법을 선택했습니다.
양방향 무한스크롤
[1단계 - 영화 목록 불러오기]에서 스스로 세웠던 'URL에 페이지 상태가 반영되어야한다'라는 전제로 인해 URL에 높은 페이지 번호가 담겨 오게되면 다량의 요청이 발생하는 문제가 있었습니다.
이를 해결하기위해 양방향 무한스크롤을 도입했는데요, 우선 URL에 담겨온 페이지를 가져오고, 이전 페이지가 있다면 페이지 상단, 다음 페이지가 있다면 페이지 하단으로 스크롤할 경우 인터섹션 옵져버로 이를 감지해 해당 페이지를 가져오는 방식입니다.
영화 목록의 각 요소에 데이터 어트리뷰트로 요소가 속한 페이지와 영화 ID를 달아주고 인터섹션 옵져버를 사용해 뷰포트안에 온전히 들어오는 첫 요소의 페이지와 영화 ID를 URL에 반영하도록 했고, 새로고침이나 첫 진입시 해당 정보를 바탕으로 첫 페이지를 그리고 스크롤 위치를 되돌려주는 동작을 추가했습니다.
CSS를 사용한 조건부 렌더링
별점 기능을 구현하면서 평점 상태에 따라 별 아이콘과 "명작이에요"와 같은 문구를 조건부로 표시해야 했는데, JS로 DOM을 직접 조작하는 방식 대신 CSS 형제 선택자와 데이터 어트리뷰트를 활용하는 방식을 시도해보았습니다.
구체적인 동작은 다음과 같습니다. 2/4/6/8/10점에 해당하는 버튼을 누르면 my-rating-selector의 데이터 어트리뷰트와 로컬 스토리지에 평점을 기록하고, 기록된 데이터 어트리뷰트를 바탕으로 아래와 같이 CSS로 조건부 렌더링합니다.
이 방식을 적용하면서 접근성 문제가 없는지도 확인했는데요,
display:none의 경우 스크린리더가 읽지 않아 크게 문제가 되지 않는다는 것을 알게 되었습니다.2) 이번 리뷰를 통해 논의하고 싶은 부분
에러 핸들링
에러 핸들링 관련해 피드백 받았던 부분들을 보완하며 아래와 같은 흐름으로 개선 과정을 거쳤던 것 같은데요.
Error cause를 통해 기존 에러 정보를 유실되지 않도록하고, 에러 생성시 맥락을 입력받아 내부 규칙에 의해 에러 메시지가 생성되는 커스텀 에러 객체까지 만들었지면 여전히 확신히 들지 않는 것 같습니다.
'어떤 작업을 하던 중 어떤 이유로 어느 지점에서 에러가 발생했는지' 조금더 상세한 정보를 에러 객체가 가져야하지 않나하는 생각이드는데요.
현재 커스텀 에러 객체가 갖는 정보가 충분한지, 내부적으로 생성하는 에러 메세지에는 문제가 없는지, 실제 프로덕션 환경에서는 어떤식으로 에러 객체를 다루는지 궁금합니다.
패턴
[1단계 - 영화 목록 불러오기]에서는 '1단계에서 MVC 패턴, React를 따라 만드는 패턴을 사용하지 않는다.'라는 제약으로 인해 별도의 패턴을 적용하지 않았습니다. 2단계에서는 패턴을 도입해보고 싶었으나, 지난 목요일 테코톡 발표로 인해 사실상 그날 저녁부터 미션을 시작하게 되었고, 시간이 부족해 기존 구조를 최대한 유지하며 구현을 진행하게 되었습니다.
주변 크루들과 이야기를 나눠보면 MVC 패턴을 적용한 경우를 많이 보게 되는데, 저도 도입을 고민하고 있습니다. 다만 현재 코드베이스를 보면 render 함수들의 인터페이스조차 아직 일관적이지 않은 상태라, 패턴을 도입하기에 앞서 기존 구조의 일관성을 먼저 갖추는 것이 순서가 아닐까 하는 생각도 드는데요.
현재 코드 베이스를 기준으로, 만약 선택을 해야한다면 제한된 시간 안에서 패턴 적용과 기존 구조의 일관성 확보 중 어떤 것을 우선하실 것 같은지, 그리고 그 이유가 무엇인지 궁금합니다.
✅ 리뷰어 체크 포인트