[2단계 - 상세 정보 & UI/UX 개선하기] 애니 미션 제출합니다.#317
Open
inaemin wants to merge 31 commits intowoowacourse:inaeminfrom
Open
[2단계 - 상세 정보 & UI/UX 개선하기] 애니 미션 제출합니다.#317inaemin wants to merge 31 commits intowoowacourse:inaeminfrom
inaemin wants to merge 31 commits intowoowacourse:inaeminfrom
Conversation
- ModalContent 인터페이스 추가 (title, genres, year, rating, overview, posterSrc) - open(content)으로 콘텐츠를 받아 DOM에 채우도록 변경 - modal-background를 element로 반환 - 각 DOM 생성 로직을 함수로 분리 (createCloseButton, createPosterImage 등) - 닫기 버튼 및 배경 클릭 시 close() 호출
- TMDBGenre, TMDBMovieDetail 타입 추가 - ENDPOINTS에 MOVIE_DETAIL(id) 엔드포인트 추가 - fetchMovieDetail(id) 함수 추가
- MovieItem 인터페이스에 id: number 추가 - toMovieItem에서 movie.id 매핑
- 하드코딩된 500을 TMDB_MAX_PAGE 상수로 추출
- 객체를 URLSearchParams로 변환하는 유틸 함수 추가
- MovieDetail 앱 도메인 타입 추가 - MovieDetailParams API 파라미터 타입 추가 - AppendToResponse 유니온 타입 추가
- TMDBMovieDetail을 MovieDetail로 변환하는 toMovieDetail 추가 - toMovieItem, toMovieDetail 평점 소수점 첫째자리 반올림 처리
- fetchMovieDetail 반환 타입을 TMDBMovieDetail에서 MovieDetail로 변경 - toURLSearchParams로 파라미터 생성 방식 통일
- 중복 fetch 로직을 fetchMoviePage 메서드로 추출 - loadMore에 isPending 가드 추가
- step2 구현 예정 stub이었던 Movie.ts 삭제 - index.ts에서 Movie export 제거
- IntersectionObserver 기반 무한 스크롤 구현 - disconnect 함수 반환으로 마지막 페이지/에러 시 옵저버 해제
- localStorage 기반 별점 저장/조회 서비스 구현
- 영화 상세 정보 로드 및 구독자 패턴 구현 - isPending, error 상태 관리 및 early return 처리
- createModal 팩토리 함수에서 Modal 클래스로 전환 - MovieService 구독으로 isPending/error/detail 상태 반영 - RatingService 연동으로 별점 저장/조회 구현 - 스켈레톤 모드, 에러 상태 표시 구현 - 별점 hover/click 이벤트 처리
- createHero, createDetailButton에서 onDetailClick 파라미터 제거
- movie-card li 요소에 data-id 속성 추가로 이벤트 위임 방식 지원
- 무한 스크롤 컴포넌트 연결 - 영화 카드 클릭 시 모달 오픈 이벤트 위임 처리
- more-button을 무한 스크롤로 교체 - 영화 카드 클릭 시 모달 오픈 연동
- mask 기반 shimmer 효과 구현 - skeleton-box, skeleton-rate, skeleton-title 클래스 추가
- 데스크탑/태블릿/모바일 반응형 레이아웃 - 내 별점 clip-path 기반 star fill 애니메이션 - 포스터 고정 크기 컨테이너로 layout shift 방지 - 스켈레톤 shimmer 적용
- 799px 이하에서 flex-direction column, 수평 가운데 정렬
- 모바일(389px 이하)에서 300px으로 축소
- empty.css 삭제, movie-list-message.css로 통합 - empty.ts, error.ts 클래스명 변경
- step2 구현 항목 체크리스트 추가
- empty 클래스명을 movie-list-message-content로 변경에 따른 테스트 수정 - 두 번째 페칭 실패 시 에러 컴포넌트 표시 테스트 추가
- MovieList import를 domains/movie/index 경유에서 직접 경로로 변경 - 불필요한 index.ts 삭제
- src/domains/ → src/services/ 폴더 구조 변경 - MovieList 클래스 및 파일명 → MovieListService로 변경 - 관련 import 경로 전체 업데이트
- cypress/e2e/*.cy.ts → cypress/e2e/step1/ 로 이동
- modal.cy.ts: 모달 열기/닫기, 영화 정보 표시, 에러/스켈레톤 상태 테스트 - rating.cy.ts: 별점 클릭, localStorage 저장, hover, 재오픈 유지 테스트 - infinite-scroll.cy.ts: 스크롤 시 추가 로드, 마지막 페이지, 검색 페이지 테스트 - fixtures.ts: 공통 fixture 분리
Walkthrough이 PR은 영화 리뷰 애플리케이션의 step2 기능들을 구현합니다. README에서 작업을 step1과 step2로 구분하고, 영화 상세정보 모달(포스터/제목 클릭 시 열림, ESC/배경 클릭으로 닫힘)을 추가하며, 별점 기능을 localStorage에 저장하도록 구현합니다. 무한 스크롤을 인기 영화와 검색 결과에 적용하고, 기존 도메인 레이어(Movie, MovieList)를 제거하여 서비스 레이어(MovieListService, MovieService)로 교체합니다. API에 영화 상세정보 엔드포인트를 추가하고, 모달 컴포넌트를 native HTML dialog 기반으로 완전 재작성합니다. 반응형 CSS를 확대하고 관련 Cypress e2e 테스트를 다수 추가합니다. Estimated code review effort🎯 5 (Critical) | ⏱️ ~120 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 |
- deploy.yml에 step2 브랜치 push 시 배포되도록 추가
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
templates/styles/components/thumbnail.css (1)
67-67:⚠️ Potential issue | 🟡 Minor
line-height: auto는 유효하지 않은 CSS 값입니다
line-height속성은auto값을 지원하지 않습니다. 브라우저가 이 값을 무시하고 기본값을 적용할 것입니다.의도한 동작에 따라
normal, 숫자 값(예:1.2), 또는 단위가 있는 값(예:24px)으로 변경하시면 됩니다.🔧 수정 제안
.item-desc strong { font-size: 20px; font-weight: bold; - line-height: auto; + line-height: normal; }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@templates/styles/components/thumbnail.css` at line 67, The CSS rule contains an invalid value "line-height: auto;" which browsers ignore; locate the declaration "line-height: auto;" in thumbnail.css and replace it with a valid value such as "line-height: normal" or a numeric/unit value (e.g., 1.2 or 24px) depending on the intended vertical spacing to ensure consistent rendering.
🧹 Nitpick comments (14)
src/components/infinite-scroll.ts (2)
11-16: IntersectionObserver 콜백에서entries배열 접근에 대해현재
entries[0].isIntersecting으로 직접 접근하고 있습니다. IntersectionObserver는 관찰 중인 요소에 대해 항상 entry를 제공하므로 실제로 문제가 발생하지는 않지만, 방어적 코딩 관점에서 선택적 체이닝을 고려해볼 수 있습니다.if (entries[0]?.isIntersecting) onIntersect();이것은 필수는 아니지만, 코드의 의도를 더 명확히 하고 예상치 못한 상황에 대비할 수 있습니다.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/components/infinite-scroll.ts` around lines 11 - 16, The IntersectionObserver callback currently accesses entries[0].isIntersecting directly; update the callback in the observer creation (the function passed to new IntersectionObserver) to defensively check the first entry before reading isIntersecting (e.g., use optional chaining or a null-check on entries[0]) and then call onIntersect() only when that check passes—this targets the callback where observer and onIntersect are used.
9-9: sentinel 요소의 가시성 고려생성된
<div>sentinel 요소에 스타일이 없어서 레이아웃에 영향을 주지 않을 것으로 보입니다. 다만, sentinel이 의도치 않게 보이거나 공간을 차지하는 경우가 있다면 명시적으로 높이나 가시성을 설정하는 것을 고려해보세요.현재 사용처(
mainPage.ts,searchPage.ts)에서 문제없이 동작하고 있다면 이대로 진행해도 무방합니다.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/components/infinite-scroll.ts` at line 9, The created sentinel div (const sentinel) may be visible or take layout space; ensure it is non-intrusive by explicitly setting its presentation when created in the infinite-scroll logic: set CSS properties such as height: 1px (or 0), width: 1px (or 100%), margin: 0, padding: 0, overflow: hidden, and visibility: hidden or aria-hidden="true" so it doesn't affect layout or accessibility, and keep the existing IntersectionObserver/observer logic (the sentinel identifier) unchanged.cypress/e2e/step1/search.cy.ts (1)
15-25: 테스트 3과 4의 중복 검증 고려두 테스트가 동일한 URL(
/search?query=zzzzzzzzzzzzzzzzzzz)을 방문하고 유사한 조건을 검증합니다:
- 테스트 3:
.movie-list-message-content존재 + "검색 결과가 없습니다." 텍스트 확인- 테스트 4:
.item미존재 +.movie-list-message-content존재 확인두 테스트를 하나로 통합하면 테스트 실행 시간을 줄이고 유지보수가 더 용이해질 수 있습니다.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@cypress/e2e/step1/search.cy.ts` around lines 15 - 25, The two tests titled "검색 결과가 없을 때 empty 상태를 표시한다" and "검색 결과가 없을 때 아이템 없이 안내 메시지만 표시한다" are redundant—combine them into a single test that visits "/search?query=zzzzzzzzzzzzzzzzzzz" once and asserts all expectations: cy.get(".movie-list-message-content").should("exist"), cy.contains("검색 결과가 없습니다.").should("be.visible"), and cy.get(".item").should("not.exist"); update or remove the duplicate it block so only the consolidated test remains.src/utils/searchParams.ts (1)
1-7:null/undefined값 처리에 대해 생각해보셨나요?현재 구현에서
String(v)는null을"null",undefined를"undefined"문자열로 변환합니다. 이런 값들이 쿼리 파라미터에 포함되는 것이 의도한 동작인지 확인이 필요합니다.일반적으로
null/undefined값은 쿼리 파라미터에서 제외하는 것이 관례입니다. 어떤 상황에서 이 함수가 호출되는지, 그리고 그 상황에서null/undefined가 발생할 수 있는지 검토해보시면 좋겠습니다.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/utils/searchParams.ts` around lines 1 - 7, The toURLSearchParams function currently converts null/undefined to the literal strings "null"/"undefined"; update it to skip any entries whose value is null or undefined before building the URLSearchParams. Specifically, inside toURLSearchParams, filter Object.entries(params) to exclude v === null || v === undefined, then map remaining entries to [k, String(v)] and pass through Object.fromEntries so null/undefined keys are omitted from the resulting URLSearchParams (keep the function name toURLSearchParams as the location to change).src/services/movie/MovieListService.ts (1)
76-79:setIsPending(true)호출 시 빈 배열로 notify하는 의도가 있나요?
setIsPending이notify([])를 호출하는데, 로딩 시작 시점에 빈 movies 배열을 전달하면 구독자 측에서 어떻게 처리되는지 확인해 보셨나요?기존 영화 목록이 잠깐 사라지거나 하는 의도치 않은 동작이 없는지 점검해 보시면 좋겠습니다.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/services/movie/MovieListService.ts` around lines 76 - 79, setIsPending currently calls notify([]) which will replace subscribers' movie list with an empty array when loading starts; instead avoid clearing the list unintentionally by changing setIsPending so that when value is true it does not send an empty array — either call notify(this.movies) to keep the current list, or skip notifying on start and only notify when loading finishes (value false) with the updated movies; update the setIsPending implementation that references isPending, notify, and movies accordingly.src/pages/searchPage.ts (1)
17-22: 이벤트 리스너 정리(cleanup)에 대해 고려해 보셨나요?constructor에서
this.app에 click 이벤트 리스너를 등록하고 있는데, 페이지 전환 시 이 리스너가 정리되지 않을 수 있습니다.SPA에서 페이지 전환이 발생할 때 기존 리스너가 남아있으면 어떤 문제가 생길 수 있을지, 그리고 어떻게 해결할 수 있을지 생각해 보시면 좋겠습니다.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/pages/searchPage.ts` around lines 17 - 22, The click listener added on this.app in the constructor can leak across SPA page transitions; extract the inline handler into a named method/property (e.g., this.handleAppClick) and register that function with this.app.addEventListener in the constructor, then remove it with this.app.removeEventListener in a corresponding teardown method (e.g., destroy/cleanup/unmount) that is called when the page is removed; reference the constructor, this.app, and modal.open to locate where to attach/detach the listener.cypress/e2e/step2/rating.cy.ts (1)
33-39: localStorage 검증을 더 정확하게 할 수 있을까요?현재
expect(stored).to.include("10")은 문자열 "10"이 포함되어 있는지만 확인합니다. 만약 다른 영화의 rating이나 id 값이 "10"을 포함하면 의도치 않게 통과할 수 있습니다.저장된 데이터를 파싱해서 특정 movieId에 대한 rating 값을 직접 확인하는 방식을 고려해 보시겠어요?
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@cypress/e2e/step2/rating.cy.ts` around lines 33 - 39, The current assertion only checks that the string "10" appears in the serialized localStorage value; instead, retrieve win.localStorage.getItem("ratings"), JSON.parse it, locate the entry for the specific movieId you expect (use the same id your test interacts with), and assert that its rating property equals 10; update the test around the .my-rating-star click and the stored variable to parse JSON and perform a precise equality check for the targeted movieId.cypress/e2e/step2/modal.cy.ts (1)
17-20: intercept 패턴**/movie/1**이 의도한 대로만 매칭되는지 확인해 보셨나요?이 패턴은
/movie/1,/movie/10,/movie/100등 "1"로 시작하는 모든 movie ID를 매칭할 수 있습니다.더 정확한 매칭이 필요하다면 정규식을 사용하거나 패턴을 조정하는 방법을 고려해 보세요.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@cypress/e2e/step2/modal.cy.ts` around lines 17 - 20, The intercept pattern currently used in cy.intercept("GET", "**/movie/1**", ...) will match any movie ID that starts with "1" (e.g., /movie/10), so update the cy.intercept call to a more precise matcher (use a RegExp that anchors the path to exactly /movie/1, optionally allowing query params) instead of the loose glob; keep the same reply (MOVIE_DETAIL_FIXTURE) and alias ("movieDetail") after changing the matcher so tests continue to use the same fixture and alias.src/services/rating/RatingService.ts (1)
19-25:JSON.parse결과의 타입 검증에 대해 생각해 보셨나요?현재
JSON.parse가 성공하면 그 결과를 그대로 반환하고 있는데, localStorage에 저장된 값이 예상과 다른 형태일 수 있습니다 (예:"[]","null","123"등).파싱된 결과가 실제로
Record<number, number>형태인지 어떻게 확인할 수 있을까요?🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/services/rating/RatingService.ts` around lines 19 - 25, The load method currently returns whatever JSON.parse yields which can be non-object types; update RatingService.load to validate the parsed value is an object (not null/array) and that each property is a numeric key and numeric value: parse the JSON, check typeof parsed === "object" && parsed !== null && !Array.isArray(parsed), iterate Object.entries(parsed) and for each [k,v] ensure Number.isInteger(Number(k)) and typeof v === "number" && Number.isFinite(v), then build and return a Record<number, number> with numeric keys; if any check fails or parsing throws, return an empty object; reference STORAGE_KEY and the load() method in RatingService.src/services/movie/MovieService.ts (1)
25-40: 빠른 연속 호출 시 동작을 검토해 보세요.
isPending가드로 동시 요청은 방지되지만, 사용자가 빠르게 다른 영화를 클릭하면 첫 번째 요청이 완료될 때까지 두 번째 영화 정보가 로드되지 않습니다.현재 동작:
- 영화 A 클릭 → 로딩 시작
- 영화 B 클릭 →
isPending이true이므로 무시됨- 영화 A 로딩 완료 → 모달에 영화 A 정보 표시 (사용자는 영화 B를 기대)
이 시나리오가 UX에 문제가 되는지 확인해 보시고, 필요하다면 요청 취소(AbortController) 또는 로드된 ID와 현재 ID 비교 로직을 고려해 보시겠어요?
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/services/movie/MovieService.ts` around lines 25 - 40, The load method on MovieService uses an isPending guard which blocks new requests, causing stale results when users quickly select another movie; update load(id: number) to cancel or ignore prior fetches by introducing an AbortController (or similar) per in-flight request and attaching it to the fetchMovieDetail call, or track a currentRequestedId property and on response only call notify(detail) if the returned id matches currentRequestedId; ensure you reset/clear the controller/currentRequestedId and set isPending accordingly in both success and error paths, and propagate the abort error to avoid marking error=true when a request was intentionally cancelled.templates/styles/components/modal.css (1)
196-206: 네이티브<dialog>요소의 기본 위치 지정과 충돌할 수 있습니다.
<dialog>요소는showModal()호출 시 기본적으로 viewport 중앙에 위치합니다. 여기서position: fixed와transform: translate(-50%, -50%)를 추가로 적용하고 있는데, 이는 브라우저마다 다르게 동작할 수 있습니다.대부분의 최신 브라우저에서는 동작하지만, 네이티브 dialog의 기본 중앙 정렬을 사용하거나
inset: 0; margin: auto;패턴을 고려해 볼 수 있습니다.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@templates/styles/components/modal.css` around lines 196 - 206, The CSS in the `@media` (min-width: 800px) block is forcing dialog.modal to use position: fixed and transform which can conflict with the native <dialog> centering behavior when showModal() is used; update the dialog.modal rules (in that media query) to stop overriding the dialog's native positioning by removing position: fixed and transform: translate(-50%, -50%) and instead center using either the browser-native behavior (no positioning overrides) or the CSS centering pattern (e.g., use inset: 0; margin: auto; and explicit width/height) so the selector dialog.modal no longer conflicts with showModal().src/components/movie-list.ts (1)
48-50: 에러/빈 상태 표시 시 기존 콘텐츠가 남아있을 수 있습니다.
showError()와showEmpty()가appendChild만 호출하고 있어서, 이전에 로드된 영화 목록이 있다면 에러/빈 메시지가 기존 콘텐츠 아래에 추가됩니다. 첫 페이지 로드 실패 시에는 문제없지만, 추후 상태 전환 시 의도치 않은 동작이 발생할 수 있습니다.이 부분이 의도된 동작인지 확인해 보시겠어요? 현재
page === 1에서만 빈 상태를 체크하고 있으니, 실제 시나리오에서는 문제가 없을 수도 있습니다.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/components/movie-list.ts` around lines 48 - 50, The error/empty handlers (showError and showEmpty) currently only append their messages, so previous movie DOM (created by createMovieUl and appended to this.grid) can remain underneath; ensure the grid is cleared before showing error/empty states by removing existing children (e.g., clear this.grid's contents) either inside showError/showEmpty or immediately before calling them in the branch; likewise clear the grid before appending new movie content when hasMovies is true to guarantee state transitions replace old content.cypress/e2e/step2/infinite-scroll.cy.ts (1)
29-37: 마지막 페이지 테스트에서 타이밍 검증을 강화할 수 있습니다.현재 테스트에서 마지막 스크롤 후
cy.get(".item").should("have.length", 60)이 즉시 실행됩니다. 실제로 추가 요청이 발생하지 않았는지 확인하려면, 짧은 대기 후에도 여전히 60개인지 검증하거나, 네트워크 요청이 추가로 발생하지 않았음을 확인하는 방법을 고려해 보세요.// 예시: 추가 요청이 없음을 더 명확히 검증 cy.scrollTo("bottom"); cy.wait(500); // 또는 cy.get("@popularMovies.all").should("have.length", 3) cy.get(".item").should("have.length", 60);🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@cypress/e2e/step2/infinite-scroll.cy.ts` around lines 29 - 37, The final-page test in infinite-scroll.cy.ts currently asserts item count immediately after the last scroll; strengthen it by asserting no further network requests occurred after the last scroll — use the existing network alias "@popularMovies" to assert its total call count (e.g., ensure it did not increase) or add a short wait (e.g., stable wait then assert) before checking that cy.get(".item") still has length 60; update the "마지막 페이지 도달 시 더 이상 로드되지 않는다" test to perform one of these checks using the "@popularMovies" alias or a brief delay prior to the final item count assertion.src/components/modal.ts (1)
32-50: 서비스 의존성 주입을 고려해 볼 수 있습니다.현재
Modal클래스가MovieService와RatingService를 직접 생성하고 있습니다. 이 방식은 동작에는 문제가 없지만, 단위 테스트 시 서비스를 모킹하기 어려울 수 있습니다.향후 테스트 용이성을 높이려면 생성자에서 서비스를 주입받는 방식을 고려해 보시겠어요? 예를 들어:
constructor( movieService: MovieService = new MovieService(), ratingService: RatingService = new RatingService() )이렇게 하면 기본값을 유지하면서도 테스트 시 모킹이 가능해집니다.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/components/modal.ts` around lines 32 - 50, The Modal constructor currently instantiates MovieService and RatingService directly, making unit tests hard to mock; change the Modal constructor to accept movieService and ratingService parameters with defaults (e.g., constructor(movieService: MovieService = new MovieService(), ratingService: RatingService = new RatingService())), assign them to this.movieService and this.ratingService, and keep the existing this.element = this.buildDialog() and the subscribe logic that references this.movieService.subscribe(({ isPending, detail, error }) => { ... }) so tests can inject mocked services while preserving default 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 `@src/utils/transform.ts`:
- Around line 18-19: The transform function currently builds posterSrc and
releaseYear without null checks which can produce invalid URLs or throw on null
release_date; update the logic in the transform (where posterSrc is created
using TMDB_IMAGE_BASE_URL and TMDB_IMAGE_SIZE and releaseYear is created via
movie.release_date.slice) to: if movie.poster_path is truthy return the
constructed URL, otherwise return a safe fallback (null or a shared
FALLBACK_POSTER_URL constant) so callers (e.g., modal rendering) can handle
missing images; and compute releaseYear defensively by checking
movie.release_date before slicing (e.g., return undefined or a default string
like "Unknown" when release_date is null/empty) so modal.ts and movie-card.ts
won't crash.
In `@templates/styles/components/search-form.css`:
- Around line 5-15: The .search-form-wrapper media rule currently forces
width:525px inside the `@media` (min-width: 390px) and (max-width: 799px) block
which causes overflow on 390–524px devices; update that rule to use a fluid
width with constraints (e.g., width: 100% plus max-width: 525px or use
max-width: 525px and an appropriate min-width) so the element scales within the
viewport, and ensure the small-screen `@media` (max-width: 389px) rule remains
consistent (refer to .search-form-wrapper and the two media query blocks to
locate the changes).
---
Outside diff comments:
In `@templates/styles/components/thumbnail.css`:
- Line 67: The CSS rule contains an invalid value "line-height: auto;" which
browsers ignore; locate the declaration "line-height: auto;" in thumbnail.css
and replace it with a valid value such as "line-height: normal" or a
numeric/unit value (e.g., 1.2 or 24px) depending on the intended vertical
spacing to ensure consistent rendering.
---
Nitpick comments:
In `@cypress/e2e/step1/search.cy.ts`:
- Around line 15-25: The two tests titled "검색 결과가 없을 때 empty 상태를 표시한다" and "검색
결과가 없을 때 아이템 없이 안내 메시지만 표시한다" are redundant—combine them into a single test that
visits "/search?query=zzzzzzzzzzzzzzzzzzz" once and asserts all expectations:
cy.get(".movie-list-message-content").should("exist"), cy.contains("검색 결과가
없습니다.").should("be.visible"), and cy.get(".item").should("not.exist"); update or
remove the duplicate it block so only the consolidated test remains.
In `@cypress/e2e/step2/infinite-scroll.cy.ts`:
- Around line 29-37: The final-page test in infinite-scroll.cy.ts currently
asserts item count immediately after the last scroll; strengthen it by asserting
no further network requests occurred after the last scroll — use the existing
network alias "@popularMovies" to assert its total call count (e.g., ensure it
did not increase) or add a short wait (e.g., stable wait then assert) before
checking that cy.get(".item") still has length 60; update the "마지막 페이지 도달 시 더 이상
로드되지 않는다" test to perform one of these checks using the "@popularMovies" alias
or a brief delay prior to the final item count assertion.
In `@cypress/e2e/step2/modal.cy.ts`:
- Around line 17-20: The intercept pattern currently used in cy.intercept("GET",
"**/movie/1**", ...) will match any movie ID that starts with "1" (e.g.,
/movie/10), so update the cy.intercept call to a more precise matcher (use a
RegExp that anchors the path to exactly /movie/1, optionally allowing query
params) instead of the loose glob; keep the same reply (MOVIE_DETAIL_FIXTURE)
and alias ("movieDetail") after changing the matcher so tests continue to use
the same fixture and alias.
In `@cypress/e2e/step2/rating.cy.ts`:
- Around line 33-39: The current assertion only checks that the string "10"
appears in the serialized localStorage value; instead, retrieve
win.localStorage.getItem("ratings"), JSON.parse it, locate the entry for the
specific movieId you expect (use the same id your test interacts with), and
assert that its rating property equals 10; update the test around the
.my-rating-star click and the stored variable to parse JSON and perform a
precise equality check for the targeted movieId.
In `@src/components/infinite-scroll.ts`:
- Around line 11-16: The IntersectionObserver callback currently accesses
entries[0].isIntersecting directly; update the callback in the observer creation
(the function passed to new IntersectionObserver) to defensively check the first
entry before reading isIntersecting (e.g., use optional chaining or a null-check
on entries[0]) and then call onIntersect() only when that check passes—this
targets the callback where observer and onIntersect are used.
- Line 9: The created sentinel div (const sentinel) may be visible or take
layout space; ensure it is non-intrusive by explicitly setting its presentation
when created in the infinite-scroll logic: set CSS properties such as height:
1px (or 0), width: 1px (or 100%), margin: 0, padding: 0, overflow: hidden, and
visibility: hidden or aria-hidden="true" so it doesn't affect layout or
accessibility, and keep the existing IntersectionObserver/observer logic (the
sentinel identifier) unchanged.
In `@src/components/modal.ts`:
- Around line 32-50: The Modal constructor currently instantiates MovieService
and RatingService directly, making unit tests hard to mock; change the Modal
constructor to accept movieService and ratingService parameters with defaults
(e.g., constructor(movieService: MovieService = new MovieService(),
ratingService: RatingService = new RatingService())), assign them to
this.movieService and this.ratingService, and keep the existing this.element =
this.buildDialog() and the subscribe logic that references
this.movieService.subscribe(({ isPending, detail, error }) => { ... }) so tests
can inject mocked services while preserving default behavior.
In `@src/components/movie-list.ts`:
- Around line 48-50: The error/empty handlers (showError and showEmpty)
currently only append their messages, so previous movie DOM (created by
createMovieUl and appended to this.grid) can remain underneath; ensure the grid
is cleared before showing error/empty states by removing existing children
(e.g., clear this.grid's contents) either inside showError/showEmpty or
immediately before calling them in the branch; likewise clear the grid before
appending new movie content when hasMovies is true to guarantee state
transitions replace old content.
In `@src/pages/searchPage.ts`:
- Around line 17-22: The click listener added on this.app in the constructor can
leak across SPA page transitions; extract the inline handler into a named
method/property (e.g., this.handleAppClick) and register that function with
this.app.addEventListener in the constructor, then remove it with
this.app.removeEventListener in a corresponding teardown method (e.g.,
destroy/cleanup/unmount) that is called when the page is removed; reference the
constructor, this.app, and modal.open to locate where to attach/detach the
listener.
In `@src/services/movie/MovieListService.ts`:
- Around line 76-79: setIsPending currently calls notify([]) which will replace
subscribers' movie list with an empty array when loading starts; instead avoid
clearing the list unintentionally by changing setIsPending so that when value is
true it does not send an empty array — either call notify(this.movies) to keep
the current list, or skip notifying on start and only notify when loading
finishes (value false) with the updated movies; update the setIsPending
implementation that references isPending, notify, and movies accordingly.
In `@src/services/movie/MovieService.ts`:
- Around line 25-40: The load method on MovieService uses an isPending guard
which blocks new requests, causing stale results when users quickly select
another movie; update load(id: number) to cancel or ignore prior fetches by
introducing an AbortController (or similar) per in-flight request and attaching
it to the fetchMovieDetail call, or track a currentRequestedId property and on
response only call notify(detail) if the returned id matches currentRequestedId;
ensure you reset/clear the controller/currentRequestedId and set isPending
accordingly in both success and error paths, and propagate the abort error to
avoid marking error=true when a request was intentionally cancelled.
In `@src/services/rating/RatingService.ts`:
- Around line 19-25: The load method currently returns whatever JSON.parse
yields which can be non-object types; update RatingService.load to validate the
parsed value is an object (not null/array) and that each property is a numeric
key and numeric value: parse the JSON, check typeof parsed === "object" &&
parsed !== null && !Array.isArray(parsed), iterate Object.entries(parsed) and
for each [k,v] ensure Number.isInteger(Number(k)) and typeof v === "number" &&
Number.isFinite(v), then build and return a Record<number, number> with numeric
keys; if any check fails or parsing throws, return an empty object; reference
STORAGE_KEY and the load() method in RatingService.
In `@src/utils/searchParams.ts`:
- Around line 1-7: The toURLSearchParams function currently converts
null/undefined to the literal strings "null"/"undefined"; update it to skip any
entries whose value is null or undefined before building the URLSearchParams.
Specifically, inside toURLSearchParams, filter Object.entries(params) to exclude
v === null || v === undefined, then map remaining entries to [k, String(v)] and
pass through Object.fromEntries so null/undefined keys are omitted from the
resulting URLSearchParams (keep the function name toURLSearchParams as the
location to change).
In `@templates/styles/components/modal.css`:
- Around line 196-206: The CSS in the `@media` (min-width: 800px) block is forcing
dialog.modal to use position: fixed and transform which can conflict with the
native <dialog> centering behavior when showModal() is used; update the
dialog.modal rules (in that media query) to stop overriding the dialog's native
positioning by removing position: fixed and transform: translate(-50%, -50%) and
instead center using either the browser-native behavior (no positioning
overrides) or the CSS centering pattern (e.g., use inset: 0; margin: auto; and
explicit width/height) so the selector dialog.modal no longer conflicts with
showModal().
🪄 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: 5acb7072-c1ce-4e5a-8958-086bb5023d52
📒 Files selected for processing (35)
README.mdcypress/e2e/step1/main.cy.tscypress/e2e/step1/routing.cy.tscypress/e2e/step1/search.cy.tscypress/e2e/step2/fixtures.tscypress/e2e/step2/infinite-scroll.cy.tscypress/e2e/step2/modal.cy.tscypress/e2e/step2/rating.cy.tssrc/api/constants.tssrc/api/movies.tssrc/api/types.tssrc/components/detail-button.tssrc/components/empty.tssrc/components/error.tssrc/components/hero.tssrc/components/infinite-scroll.tssrc/components/modal.tssrc/components/movie-card.tssrc/components/movie-list.tssrc/domains/movie/Movie.tssrc/domains/movie/index.tssrc/pages/mainPage.tssrc/pages/searchPage.tssrc/services/movie/MovieListService.tssrc/services/movie/MovieService.tssrc/services/rating/RatingService.tssrc/utils/searchParams.tssrc/utils/transform.tstemplates/styles/base.csstemplates/styles/components/header.csstemplates/styles/components/modal.csstemplates/styles/components/movie-list-message.csstemplates/styles/components/search-form.csstemplates/styles/components/thumbnail.csstemplates/styles/main.css
💤 Files with no reviewable changes (2)
- src/domains/movie/index.ts
- src/domains/movie/Movie.ts
Comment on lines
+18
to
+19
| posterSrc: `${TMDB_IMAGE_BASE_URL}/${TMDB_IMAGE_SIZE}${movie.poster_path}`, | ||
| releaseYear: movie.release_date.slice(0, 4), |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Description: Check if there's fallback handling for null poster_path elsewhere
# Search for poster_path null handling
rg -n "poster_path|posterSrc" --type ts -C 2Repository: woowacourse/javascript-movie-review
Length of output: 6170
🏁 Script executed:
cat -n src/api/types.ts | head -80Repository: woowacourse/javascript-movie-review
Length of output: 2035
🏁 Script executed:
cat -n src/utils/transform.tsRepository: woowacourse/javascript-movie-review
Length of output: 1134
🏁 Script executed:
rg -n "onerror|fallback|DEFAULT" src/components/ --type tsRepository: woowacourse/javascript-movie-review
Length of output: 475
🏁 Script executed:
cat -n src/components/modal.ts | sed -n '80,95p'Repository: woowacourse/javascript-movie-review
Length of output: 923
🏁 Script executed:
rg -n "release_date" src/ --type ts -C 1 | head -40Repository: woowacourse/javascript-movie-review
Length of output: 693
poster_path와 release_date에 대한 null 안전성 검토 필요
테스트 fixture에서 poster_path: null 사용이 확인되었고, 실제 TMDB API도 이 필드가 null을 반환할 수 있습니다. 현재 코드를 살펴보면:
- 19번 줄:
movie.release_date.slice(0, 4)-release_date가 null이거나 빈 문자열이면 TypeError 발생 - 18번 줄:
poster_path가 null이면"/w500null"같은 유효하지 않은 URL 생성
18번 줄의 경우 movie-card.ts에서 이미지 로드 실패 시 처리하는 onerror 핸들러가 있지만, modal.ts에는 이런 fallback이 없습니다. 특히 19번 줄은 모달 렌더링 자체를 중단시킬 수 있으므로 주의가 필요합니다.
타입 정의에서 이 필드들이 non-nullable로 선언되어 있으나 실제 데이터는 null을 포함할 수 있다는 점을 고려하여, 어떻게 안전하게 처리할지 검토해 보세요.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/utils/transform.ts` around lines 18 - 19, The transform function
currently builds posterSrc and releaseYear without null checks which can produce
invalid URLs or throw on null release_date; update the logic in the transform
(where posterSrc is created using TMDB_IMAGE_BASE_URL and TMDB_IMAGE_SIZE and
releaseYear is created via movie.release_date.slice) to: if movie.poster_path is
truthy return the constructed URL, otherwise return a safe fallback (null or a
shared FALLBACK_POSTER_URL constant) so callers (e.g., modal rendering) can
handle missing images; and compute releaseYear defensively by checking
movie.release_date before slicing (e.g., return undefined or a default string
like "Unknown" when release_date is null/empty) so modal.ts and movie-card.ts
won't crash.
Comment on lines
+5
to
+15
| @media (min-width: 390px) and (max-width: 799px) { | ||
| .search-form-wrapper { | ||
| width: 525px; | ||
| } | ||
| } | ||
|
|
||
| @media (max-width: 389px) { | ||
| .search-form-wrapper { | ||
| width: 300px; | ||
| } | ||
| } |
There was a problem hiding this comment.
모바일(390~524px)에서 검색폼 가로 오버플로우가 발생할 수 있습니다.
Line [7]의 width: 525px가 390~799px에 고정 적용되어, 390~524px 기기에서 뷰포트보다 넓어집니다. 반응형 구간에서는 고정폭보다 max/ min 기반이 안전합니다.
수정 예시
.search-form-wrapper {
- width: 525px;
+ width: min(100%, 525px);
}
-@media (min-width: 390px) and (max-width: 799px) {
- .search-form-wrapper {
- width: 525px;
- }
-}
-
`@media` (max-width: 389px) {
.search-form-wrapper {
- width: 300px;
+ width: min(100%, 300px);
}
}🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@templates/styles/components/search-form.css` around lines 5 - 15, The
.search-form-wrapper media rule currently forces width:525px inside the `@media`
(min-width: 390px) and (max-width: 799px) block which causes overflow on
390–524px devices; update that rule to use a fluid width with constraints (e.g.,
width: 100% plus max-width: 525px or use max-width: 525px and an appropriate
min-width) so the element scales within the viewport, and ensure the
small-screen `@media` (max-width: 389px) rule remains consistent (refer to
.search-form-wrapper and the two media query blocks to locate the changes).
- will-change: clip-path 추가로 GPU 합성 레이어 활용
JUDONGHYEOK
requested changes
Apr 14, 2026
JUDONGHYEOK
left a comment
JUDONGHYEOK
left a comment
There was a problem hiding this comment.
안녕하세요 애니! 이번미션도 깔끔하게 잘 수행해주신 것 같습니다ㅎㅎ
1단계에서 적용해주신 옵저버패턴을 2단계에서도 연속적으로 잘 적용해주신 것 같아요. 또한 IntersectionObserver를 사용해서 인피니티 스크롤도 깔끔하게 구현해주셨고 별도 모듈로 분리해서 재사용을 해주신 모습도 좋았습니다. 또 clip-path를 사용해서 reflow를 방지해주신 모습도 이런 디테일까지 생각해주셔서 너무너무 좋았습니다!
다만 몇몇 모듈들이 비대해진 것 같습니다. 이건 구조자체의 문제라기보다는 현재 구조내에서 각 모듈 자체를 잘 쪼개는 것 만으로도 충분히 해결가능하다고 생각하여 애니의 판단대로 잘 쪼개주시면 좋을 것 같아요!
Q&A
! assertion 사용: Modal 클래스는 fetch 결과를 DOM에 반영하는 역할이 많아 클래스가 보유해야 하는 DOM 필드가 많습니다. 전부 constructor에서 직접 할당하기보다 가독성을 위해 메서드로 분리했는데, 그러다 보니 TypeScript가 할당을 추적하지 못해 ! assertion 사용이 사실상 강제되었습니다. 이전 리뷰에서 assertion 사용을 지양하라는 피드백을 주신 것으로 기억하는데, 이 구조에서 더 나은 방법이 있는지 여쭤보고 싶습니다.
참 난감한 상황입니다. 이런상황에서 가장 간단한 해결책은 메소드를 반환값을 넘겨주는 형태로 변경하여 필드를 할당해주는 것이에요. 요러면 자연스럽게 타입스크립트가 필드할당을 추적하게 될 것 같습니다.
2단계 미션도 1단계 미션 구조 그대로 잘 구현해주셔서 리뷰가 많지 않았네요..ㅎㅎ 말씀해주신 부분의 수정과 곳곳의 비대해진 모듈들만 추상화 해주시면 완벽할 것 같습니다! 수고많으셨어요 애니!!
|
|
||
| private load(): Record<number, number> { | ||
| try { | ||
| return JSON.parse(localStorage.getItem(STORAGE_KEY) ?? "{}"); |
There was a problem hiding this comment.
RatingService가 localStorage를 직접 호출하고 있어요. 저장소를 sessionStorage, IndexedDB, 서버 API 등으로 바꾸려면 이 클래스를 직접 수정해야 합니다. Storage 인터페이스를 추상화해서 생성자로 주입하는 것을 시도해보는 것은 어떨까요?
| 10: "명작이에요", | ||
| }; | ||
|
|
||
| export class Modal { |
There was a problem hiding this comment.
Modal의 추상화가 필요한 시점인 것 같아요. Dialog DOM 생성, 영화 상세 데이터 fetch 구독, 별점 UI 렌더링, 이벤트 바인딩, 별점 저장 로직, 스켈레톤/에러 상태 관리 등등이 있는 것 같은데요. 확실히 역할에 맞게 레벨을 변화하는 추상화가 필요할 것 같은데 어떻게 생각하시나요?
Comment on lines
+13
to
+15
| private isPending = false; | ||
| private error = false; | ||
| private subscribers: Set<Subscriber> = new Set(); |
There was a problem hiding this comment.
요런 구조가 반복된다면 상위 객체를 만들어서 추상화를 시도해도 좋을 것 같습니다! 중복되는 코드가 많아지는 것 같아서요!
| cy.get(".my-rating-star").eq(4).click(); | ||
| cy.window().then((win) => { | ||
| const stored = win.localStorage.getItem("ratings"); | ||
| expect(stored).to.include("10"); |
There was a problem hiding this comment.
"10"이라는 문자열 포함 여부만 체크하면 movieId가 10인 경우에도 통과할 것 같은데요. JSON.parse 후 직접 rating을 확인해야 더 정확할 것 같아요ㅎㅎ
| }); | ||
|
|
||
| it("검색 결과가 없을 때 그리드 레이아웃이 적용되지 않는다", () => { | ||
| it("검색 결과가 없을 때 아이템 없이 안내 메시지만 표시한다", () => { |
| width: "fit", | ||
| size: "s", | ||
| onClick, | ||
| onClick: () => {}, |
Comment on lines
+86
to
+87
| this.posterImg.src = content.posterSrc; | ||
| this.posterImg.alt = `poster of ${content.title}`; |
There was a problem hiding this comment.
여기 postImg에서도 대체이미지가 있으면 좋을 것 같습니다!
| private router: Router, | ||
| ) {} | ||
| ) { | ||
| this.app.addEventListener("click", (e) => { |
There was a problem hiding this comment.
여기 할당된 이벤트를 페이지 이동시에 정리할 필요는 없을까요?
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)
제출 전 체크 리스트
리뷰 요청 & 논의하고 싶은 내용
안녕하세요 동키콩! step2 리뷰도 잘 부탁드립니다 ㅎ
일단 step1에서 리뷰해주신 내용을 바탕으로 먼저 수정한 뒤 구현을 시작했습니다.
조언해주신 내용을 바탕으로 domain을 services로 이동했습니다.
원래 각 컴포넌트에서 필요로 하는 이벤트를 각각 정의한 뒤 딱 필요한 데이터만 받아서 업데이트하는 설계로 수정하려고 했는데, 아직 구현하지 못했습니다. RC 주시면 다시 수정해보겠습니다.
모달은 하나의 컴포넌트로 만들고 CSS(media-query)로 반응형 레이아웃을 구성했습니다.
아직 모달 CSS에 버그가 많습니다. RC 주시면 수정할 예정입니다.
무한 스크롤은 IntersectionObserver API를 사용한 컴포넌트를 만들어서 적용했습니다.
1) 이번 단계에서 가장 많이 고민했던 문제와 해결 과정에서 배운 점
모달 컴포넌트 설계에서 가장 많이 고민했습니다.
처음에는
createModal()팩토리 함수로 시작했지만, 영화 상세 데이터 로딩 상태(isPending, error, success)와 별점 인터랙션을 함께 관리하다 보니 상태를 다루기 어려워졌습니다. 이를Modal클래스로 전환하고,MovieService와RatingService를 주입받아 구독하는 방식으로 해결했습니다.MovieService는MovieListService와 동일한 subscriber 패턴으로 설계해 일관성을 유지했습니다.isPending가드로 중복 요청을 막고, 상태 전환(skeleton → content / skeleton → error)을 명확하게 처리할 수 있었습니다.별점 hover 시 reflow 문제도 고민했는데, filled/empty 이미지를 겹쳐두고
clip-path: inset()으로 토글하는 방식으로 해결했습니다.src를 교체하면 reflow가 발생하지만clip-path는 GPU 합성 레이어에서 처리되기 때문입니다.2) MovieService ↔ Modal 데이터 흐름
sequenceDiagram actor User participant Modal participant RatingService participant MovieService participant API User->>Modal: 영화 카드 클릭 (open(id)) Modal->>Modal: element.showModal() Modal->>RatingService: get(id) RatingService-->>Modal: rating | null Modal->>Modal: updateMyRating(rating) Modal->>MovieService: load(id) MovieService-->>Modal: notify { isPending: true } Modal->>Modal: showSkeleton() MovieService->>API: fetchMovieDetail(id) alt 성공 API-->>MovieService: MovieDetail MovieService-->>Modal: notify { isPending: false, detail } Modal->>Modal: update(detail) else 실패 API-->>MovieService: Error MovieService-->>Modal: notify { isPending: false, error: true } Modal->>Modal: showError() end3) 이번 리뷰를 통해 논의하고 싶은 부분
!assertion 사용:Modal클래스는 fetch 결과를 DOM에 반영하는 역할이 많아 클래스가 보유해야 하는 DOM 필드가 많습니다. 전부 constructor에서 직접 할당하기보다 가독성을 위해 메서드로 분리했는데, 그러다 보니 TypeScript가 할당을 추적하지 못해!assertion 사용이 사실상 강제되었습니다. 이전 리뷰에서 assertion 사용을 지양하라는 피드백을 주신 것으로 기억하는데, 이 구조에서 더 나은 방법이 있는지 여쭤보고 싶습니다.✅ 리뷰어 체크 포인트