[2단계 - 상세 정보 & UI/UX 개선하기] 벤지 미션 제출합니다.#297
Open
hjkim0905 wants to merge 42 commits intowoowacourse:hjkim0905from
Open
[2단계 - 상세 정보 & UI/UX 개선하기] 벤지 미션 제출합니다.#297hjkim0905 wants to merge 42 commits intowoowacourse:hjkim0905from
hjkim0905 wants to merge 42 commits intowoowacourse:hjkim0905from
Conversation
Co-authored-by: th-97 <hood3392@gmail.com>
Co-authored-by: th-97 <hood3392@gmail.com>
Co-authored-by: th-97 <hood3392@gmail.com>
Co-authored-by: th-97 <hood3392@gmail.com>
Co-authored-by: th-97 <hood3392@gmail.com>
Co-authored-by: th-97 <hood3392@gmail.com>
Co-authored-by: th-97 <hood3392@gmail.com>
Co-authored-by: th-97 <hood3392@gmail.com>
Co-authored-by: th-97 <hood3392@gmail.com>
Co-authored-by: th-97 <hood3392@gmail.com>
Co-authored-by: th-97 <hood3392@gmail.com>
Added a module relationship diagram to the README.
…nto SearchHandler and ModalHandler
…ating class for rate trace
둘러보기이 풀 리퀘스트는 영화 평가 애플리케이션을 전체적으로 구현합니다. 인기 영화 초기 로딩(20개), 무한 스크롤을 통한 추가 로딩, 검색 기능(엔터/버튼 실행, 결과 없을 때 안내 메시지), 클릭으로 열리는 영화 상세 정보 모달(ESC/배경 클릭 닫기), 별점 평가 시스템(1~5개 별, 2점 단위, localStorage로 새로고침 후 유지), 반응형 레이아웃(모바일/태블릿/데스크톱), 그리고 Cypress E2E 테스트를 포함합니다. 애플리케이션 로직, 스타일시트, 타입 정의, 저장소 구현, 테스트 픽스처, 그리고 포괄적인 테스트 커버리지가 추가되었습니다. 예상 코드 리뷰 노력🎯 4 (Complex) | ⏱️ ~75 minutes 🚥 Pre-merge checks | ✅ 3✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 18
Note
Due to the large number of review comments, Critical, Major severity comments were prioritized as inline comments.
🟡 Minor comments (10)
README.md-12-14 (1)
12-14:⚠️ Potential issue | 🟡 Minor문서의 페이징 방식 설명을 하나로 통일해 주세요.
동일 README 안에서
더 보기 버튼설명과무한 스크롤 전환설명이 함께 있어 현재 동작 기준이 모호합니다. 최종 구현 기준(무한 스크롤)으로 관련 문구를 정리하면 혼선을 줄일 수 있습니다.Also applies to: 22-23, 63-65, 86-88
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@README.md` around lines 12 - 14, README 내의 페이징 관련 문구들이 '더 보기 버튼(더 보기(Pagination))'과 '무한 스크롤 전환'으로 혼재되어 있어 동작 기준이 모호하니 최종 구현 기준인 무한 스크롤 동작으로 모든 관련 문구를 통일하세요; 구체적으로 README의 "더 보기(Pagination)" 항목과 "더 보기 버튼" 언급들(현재 문제 지점: 해당 문구가 있는 블록들, 예: 12-14, 22-23, 63-65, 86-88)에 들어간 설명을 모두 "무한 스크롤로 추가 영화 로드" 또는 유사한 단일 표현으로 바꾸고, 버튼 제거/표시 관련 문구 대신 더 이상 로드할 데이터가 없을 때의 무한 스크롤 종료 동작(예: 더 이상 항목이 없을 경우 스크롤 리스너 해제 또는 완료 메시지 표시)을 기술하도록 업데이트하세요; 변경 시 '더 보기 버튼' 용어를 제거하고 '무한 스크롤', '스크롤로 추가 로드', '로딩 종료' 같은 일관된 키워드를 사용해 문장들을 통일하세요.public/styles/tab.css-8-20 (1)
8-20:⚠️ Potential issue | 🟡 MinorStylelint 에러(빈 줄 규칙) 정리가 필요합니다.
Line 12, Line 18에서
declaration-empty-line-before규칙 위반이 발생합니다. 공백 줄만 정리하면 됩니다.수정 예시
.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 `@public/styles/tab.css` around lines 8 - 20, Remove the stray blank lines inside the .tab-item rule to satisfy the stylelint declaration-empty-line-before rule: open the CSS rule for .tab-item and delete the empty lines between declarations (e.g., between "user-select: none;" and "display: flex;" and between "height: 40px;" and "border-radius: 40px;") so all declarations are contiguous with no extra blank lines.cypress/fixtures/movies.json-137-142 (1)
137-142:⚠️ Potential issue | 🟡 Minor마지막 아이템 이미지 경로가 이전 아이템과 중복됩니다.
Line 141-142가 Line 134-135와 동일해서 20번째 데이터 식별성이 떨어집니다. 테스트 의도가 “20개 서로 다른 카드”라면 경로를 분리하는 편이 안전합니다.
수정 예시
- "poster_path": "/test18.jpg", - "backdrop_path": "/backdrop18.jpg" + "poster_path": "/test19.jpg", + "backdrop_path": "/backdrop19.jpg"🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@cypress/fixtures/movies.json` around lines 137 - 142, The last fixture entry (id 1020, title "영화 20") reuses the same poster_path/backdrop_path values as the previous item, reducing test distinctiveness; update the poster_path and backdrop_path fields for the object with id 1020 (and any related "영화 20" references) to unique file names (e.g., /test20.jpg and /backdrop20.jpg) so each of the 20 fixtures has distinct image paths.__test__/ApiRequest.test.ts-5-13 (1)
5-13:⚠️ Potential issue | 🟡 Minor전역
fetch스텁 정리가 없어 테스트 격리가 약합니다.현재는
beforeEach에서 계속 덮어쓰기만 하고 해제는 하지 않습니다. 파일 간 간섭이 생기는지 확인해 보시고,afterEach에서 global/mock 정리를 추가하는 쪽을 고려해 주세요.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@__test__/ApiRequest.test.ts` around lines 5 - 13, Add cleanup in an afterEach to unstub and clear the global fetch mock introduced in beforeEach: afterEach should call vi.unstubAllGlobals() (to remove the stubbed global "fetch") and vi.clearAllMocks()/vi.restoreAllMocks() to reset mock state; update the test file to add an afterEach that calls vi.unstubAllGlobals(); vi.clearAllMocks(); (referencing beforeEach, vi.stubGlobal, fetch, vi.unstubAllGlobals, vi.clearAllMocks/vi.restoreAllMocks).templates/modal.html-26-27 (1)
26-27:⚠️ Potential issue | 🟡 Minor접근성: 이미지 대체 텍스트 정책을 정리해 주세요.
별점 아이콘/푸터 로고/포스터 이미지에
alt가 누락되어 있습니다. 장식 이미지는alt=""로 숨기고, 정보 이미지는 의미 있는alt를 넣어 구분해 주세요.Also applies to: 386-387, 397-397
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@templates/modal.html` around lines 26 - 27, The img elements are missing alt attributes: add appropriate alt text to all <img> tags (e.g., the image with class="star" next to <span class="rate-value"> should be marked decorative with alt=""), and for footer logo and poster images provide meaningful alt text that describes their content (e.g., "Site logo" or the poster title). Ensure decorative images use alt="" and informational images use concise, descriptive alt strings; update the img tags for the star (class="star") plus the footer/logo and poster image elements referenced in the template.src/StarRating.ts-93-93 (1)
93-93:⚠️ Potential issue | 🟡 Minor사용자 노출 문구 오탈자입니다
Line 93의
"최악이예요"는"최악이에요"가 자연스럽습니다.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/StarRating.ts` at line 93, StarRating.ts에 있는 사용자 노출 문구 배열에서 현재 항목 "최악이예요"의 오탈자를 수정하세요: 해당 배열(별점 라벨을 정의한 상수/변수, 즉 StarRating 내의 라벨 배열에서)에서 "최악이예요"를 찾아 "최악이에요"로 바꿔 저장하면 됩니다.cypress/e2e/movie.cy.ts-37-42 (1)
37-42:⚠️ Potential issue | 🟡 Minor“마지막 페이지에서 더 로드되지 않음”을 직접 검증하지는 못하고 있습니다
현재는 2페이지까지 로드된 결과(40개)만 확인합니다. 한 번 더 sentinel을 트리거했을 때 개수/요청 수가 유지되는지까지 확인해야 테스트 이름과 일치합니다.
Also applies to: 83-90
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@cypress/e2e/movie.cy.ts` around lines 37 - 42, The test "마지막 페이지 도달 시 스크롤 시에 영화를 더 불러오지 않는다." only asserts 40 items after page2 loads but doesn't verify that a subsequent sentinel trigger doesn't cause another network request or increase items; update the spec to trigger the sentinel a second time (cy.get("#scroll-sentinel").scrollIntoView()), assert no additional network call was made (use cy.wait with a timeout/fail-safe or better assert request count on the "@getMovies" / "@getMoviesPage2" aliases or use cy.intercept().as and cy.get('@getMoviesPage2.all') to check length), and re-assert cy.get(".thumbnail-list li").should("have.length", 40). Apply the same additional trigger+assert logic to the other test block referenced (lines ~83-90) so both tests confirm no extra load occurs after the last page.cypress/e2e/modal.cy.ts-71-71 (1)
71-71:⚠️ Potential issue | 🟡 Minor고정 픽셀 width 단정은 의도와 맞지 않을 수 있습니다
Line 71에서
"768px"를 단정하고 있지만,modal.css의 태블릿 미디어 쿼리(line 191)에서는.modal을width: 100%로 정의하고 있습니다. viewport가 768×1024로 설정된 환경에서는 결과적으로 768px가 되겠지만, CSS 정의와 테스트 단정 간의 괴리가 있습니다.다음을 생각해 보세요:
- CSS에서 정의한 방식(상대값 vs 절대값)과 테스트에서 검증하는 방식이 일관성 있는가?
- 태블릿 반응형 의도를 가장 잘 표현하는 assertion은 무엇인가? (box-sizing, padding 등을 고려할 때)
- viewport 크기 변화에도 안정적으로 유지되려면 어떤 방식으로 검증해야 하는가?
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@cypress/e2e/modal.cy.ts` at line 71, The test asserts a fixed "768px" width for .modal which conflicts with the CSS tablet breakpoint that sets .modal { width: 100% }; update the assertion in modal.cy.ts (the cy.get(".modal") check) to first set the viewport to the intended tablet size and then assert the responsive behavior instead of a hard pixel value: either assert the computed CSS is "100%" (should("have.css","width","100%")) at that breakpoint, or assert the modal element's actual pixel width equals the current viewport width minus known horizontal padding (use a callback assertion like .should($el => { const vw = Cypress.config("viewportWidth"); expect($el.outerWidth()).to.equal(vw - <paddingSum>); }) so the test follows the CSS intent rather than hard-coding 768px.templates/index.html-43-46 (1)
43-46:⚠️ Potential issue | 🟡 Minor접근성: 푸터 로고 이미지에 alt 속성이 누락되었습니다.
푸터의 우아한테크코스 로고 이미지에
alt속성이 없습니다.♿ 제안하는 수정
<footer class="footer"> - <p><img src="/images/woowacourse_logo.png" width="180" /></p> + <p><img src="/images/woowacourse_logo.png" width="180" alt="우아한테크코스 로고" /></p> <p>© 우아한테크코스 All Rights Reserved.</p> </footer>🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@templates/index.html` around lines 43 - 46, The footer's logo <img> element (src="/images/woowacourse_logo.png") in the <footer class="footer"> lacks an alt attribute; add a descriptive alt attribute (e.g., alt="우아한테크코스 로고") to the <img> to improve accessibility and convey the image purpose to assistive technologies.templates/index.html-51-53 (1)
51-53:⚠️ Potential issue | 🟡 Minor접근성: 닫기 버튼 이미지에 alt 속성이 누락되었습니다.
모달 닫기 버튼의
<img>태그에alt속성이 없어 스크린 리더 사용자가 버튼의 목적을 알 수 없습니다.♿ 제안하는 수정
<button class="close-modal" id="closeModal"> - <img src="/images/modal_button_close.png" /> + <img src="/images/modal_button_close.png" alt="닫기" /> </button>🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@templates/index.html` around lines 51 - 53, 이미지에 alt 속성이 없어 스크린 리더에서 버튼 목적을 알 수 없습니다; closeModal 버튼(요소 id: closeModal, 클래스: close-modal)에 있는 <img> 태그에 설명적인 대체 텍스트를 추가해 주세요(예: alt="모달 닫기" 또는 alt="닫기"), 필요하면 버튼 자체에도 aria-label="닫기"를 추가해 보조기기에서 명확하게 인식되도록 만드세요.
🧹 Nitpick comments (7)
templates/styles/thumbnail.css (1)
25-45: 스켈레톤 색상도 토큰화하면 유지보수가 쉬워집니다.현재 그라데이션 색이 하드코딩되어 있어 테마 변경 시 수정 범위가 커집니다.
colors.css변수를 재사용하는 방향을 고려해 주세요.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@templates/styles/thumbnail.css` around lines 25 - 45, Replace the hardcoded gradient colors in the skeleton styles with CSS color tokens from your colors.css (e.g., use var(--skeleton-base), var(--skeleton-highlight) or similar) so theme changes are centralized; update the selectors that use the gradient (.skeleton, .skeleton-title, .skeleton-rate) to build the same linear-gradient using those variables and keep the existing background-size and animation properties unchanged, and add or document the corresponding token names in colors.css if they don't already exist.public/styles/colors.css (1)
11-12: 색상 토큰 중복으로 네이밍 체계가 흔들립니다.
--color-white/--color-ffffff,--color-bluegray-100/--color-242a32처럼 동일 의미 토큰이 공존합니다. 한 체계(의미 기반 또는 값 기반)로 통일하는 쪽이 유지보수에 유리합니다.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@public/styles/colors.css` around lines 11 - 12, 파일의 CSS 변수에 동일 색상을 의미하는 중복 토큰(--color-ffffff vs --color-white, --color-242a32 vs --color-bluegray-100)이 섞여 있어 네이밍 체계를 통일해야 합니다; 결정(의미 기반 예: --color-white, --color-bluegray-100 또는 값 기반 예: --color-ffffff, --color-242a32)을 내려 하나의 스타일으로 합치고 중복된 변수명 중 하나만 남긴 뒤 프로젝트 전역에서 해당 변수명을 일관되게 교체하세요(사용처 검색/치환). 또한 남긴 변수에 대해 파일 상단에 짧은 주석으로 네이밍 규칙을 적어 유지보수를 쉽게 하고, 제거한 변수와의 매핑을 잠정적으로 주석에 기록한 뒤 관련 CSS/JS 참조가 모두 업데이트되면 중복 주석을 삭제하십시오.public/styles/modal.css (1)
4-6: 모달 오픈 시 바디 스크롤 락이 실제로 연결되지 않은 상태로 보입니다
body.modal-open스타일은 있는데, 모달 열기/닫기 핸들러에서 해당 클래스를 토글하는 코드가 보이지 않습니다. CSS를 유지할 거라면 JS 이벤트 흐름과 연결해 주세요.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@public/styles/modal.css` around lines 4 - 6, CSS defines a body.modal-open rule but nothing toggles it; update your modal open/close logic to add document.body.classList.add('modal-open') when opening the modal and remove it on close (in the functions that currently handle modal state, e.g., openModal/closeModal/toggleModal or your event listeners on the modal trigger and close button), and ensure the code also removes the class on backdrop or escape-key dismissal so the body scroll is always restored.src/movieRenderer.ts (1)
75-82: 함수 시그니처와 변수 할당에 개선 여지가 있습니다.몇 가지 질문을 드립니다:
fristMovieData→firstMovieData오타가 아닌지 확인해 주세요.async키워드가 선언되어 있지만, 함수 내에서await를 사용하지 않습니다. 이 함수가 정말 비동기여야 하나요?- Line 76의
movies = fristMovieData와 Line 82의mostPopularMovie = movies는 동일한 객체를 세 개의 다른 이름으로 참조하고 있습니다. 이렇게 하는 이유가 있나요?코드를 단순화할 방법을 고민해 보세요.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/movieRenderer.ts` around lines 75 - 82, 함수 renderBanner의 매개변수 fristMovieData 오타를 firstMovieData로 수정하고, 내부에서 movies와 mostPopularMovie로 동일 객체를 중복 참조하지 않고 한 이름만 사용하도록 정리하세요 (예: use firstMovieData directly); 또한 함수 선언에서 async가 있지만 await를 사용하지 않으니 비동기 작업이 필요하지 않다면 async를 제거하거나 실제 비동기 호출(예: 이미지 fetch 등)을 추가해 await를 사용하도록 변경하세요.src/SearchHandler.ts (2)
26-44: 페이지네이션 로직의 의도를 명확히 이해하고 있는지 확인해 보세요.
handleLoadMoreScroll에서totalPopularPages === this.state.moviePageCount조건으로 마지막 페이지를 판단하고 있습니다.한 가지 질문: 만약 API 호출이 실패해서
renderMovies가0을 반환한다면,moviePageCount가 1일 때0 === 1은false가 되어 스크롤이 계속 트리거될 수 있습니다. 이 경우 어떤 동작이 예상되나요? 에러 상황에서 무한 스크롤을 멈추는 것이 더 나은 사용자 경험일 수 있습니다.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/SearchHandler.ts` around lines 26 - 44, handleLoadMoreScroll currently treats equality of totalPopularPages/searchPages to the current page count as end-of-pages but doesn't handle error/falsy returns (e.g., 0) from renderMovies or renderSearchedMovies; update the logic in handleLoadMoreScroll to detect error or zero returns and stop further scrolling by treating totalPopularPages/searchPages <= this.state.moviePageCount (or <= this.state.searchPageCount) or total === 0/null/undefined as terminal, and ensure any promise rejection from renderMovies/renderSearchedMovies is caught so you return true (stop) on error; refer to handleLoadMoreScroll, renderMovies, renderSearchedMovies, state.moviePageCount, and state.searchPageCount when making the change.
49-50: Non-null assertion 사용에 대해 생각해 볼 점이 있습니다.Line 50에서
!(non-null assertion)를 사용하고 있습니다. 만약.search-input요소가 DOM에 존재하지 않는다면 어떤 일이 발생할까요?
handleSearchSubmit이 호출될 수 있는 모든 시점에서 해당 요소가 반드시 존재한다고 확신할 수 있나요?replaceBanner함수가 호출된 후에도 동일한 셀렉터로 요소를 찾을 수 있을지 확인해 보시기 바랍니다.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/SearchHandler.ts` around lines 49 - 50, The code uses a non-null assertion when reading document.querySelector<HTMLInputElement>(".search-input")!.value which will throw if the element is missing (e.g., after replaceBanner); modify handleSearchSubmit to safely query the element (using a local const like el = document.querySelector<HTMLInputElement>(".search-input")), check for null, and handle the absent case (early return, fallback value, or read from the event/ref) before assigning this.state.currentKeyword; also consider scoping the query to a known container or updating replaceBanner to preserve the input element if appropriate.public/styles/thumbnail.css (1)
76-78: Stylelint: 복잡한:not()셀렉터 표기법 개선이 필요합니다.현재
:not(.skeleton-rate):not(.skeleton-title)형태로 연속된:not()의사 클래스를 사용하고 있습니다. 최신 CSS 표기법에서는 쉼표로 구분된 단일:not()사용을 권장합니다.♻️ 제안하는 수정
-.item.skeleton .item-desc > *:not(.skeleton-rate):not(.skeleton-title) { +.item.skeleton .item-desc > *:not(.skeleton-rate, .skeleton-title) { display: none; }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@public/styles/thumbnail.css` around lines 76 - 78, The selector ".item.skeleton .item-desc > *:not(.skeleton-rate):not(.skeleton-title)" uses consecutive :not() pseudo-classes; change it to the modern single negation with a comma-separated list (i.e., :not(.skeleton-rate, .skeleton-title)) to satisfy Stylelint and simplify the rule, updating the selector accordingly wherever ".item.skeleton .item-desc > *:not(.skeleton-rate):not(.skeleton-title)" appears.
🤖 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/movie.cy.ts`:
- Around line 50-61: These tests currently call the real search API; add
cy.intercept fixtures to stub responses for both scenarios before triggering the
UI: intercept the search API (match the same request your app uses, e.g., GET
/api/search or path with query) and return a fixture like
"search-harry-potter.json" for the "Harry Potter" test and "search-empty.json"
for the no-results test; place the intercepts before using
cy.get(".search-input")/cy.get(".search-button") (or cy.press("Enter")), alias
the routes (cy.as('search')) and optionally cy.wait('@search') before asserting
against ".thumbnail-list li" and "#no-result" so CI is stable and deterministic.
- Line 9: In cypress/e2e/movie.cy.ts the cy.visit calls (the cy.visit(...)
invocations) omit the protocol causing environment-dependent failures; fix by
either setting baseUrl in cypress.config.ts and replacing those
cy.visit("localhost:5173") calls with cy.visit("/") across the file, or update
each cy.visit invocation (the three cy.visit(...) occurrences) to include the
full URL with protocol (e.g., "http://localhost:5173") so the tests are
consistent and stable.
In `@cypress/e2e/rating.cy.ts`:
- Around line 18-25: The test currently skips assertions when
localStorage.getItem("movieRatings") returns null; change the check to
explicitly assert existence first and only then parse and deep-equal the value.
Specifically, after cy.window().its("localStorage").invoke("getItem",
"movieRatings"), add an assertion that the returned val is not null/undefined
(e.g., expect(val).to.exist) and then JSON.parse(val) and expect the parsed
object to.deep.equal({ "1001": 8 }), so the test fails if the item was never
stored.
In `@public/styles/main.css`:
- Line 1: Replace the `@import` url(...) at the top with the import-notation style
required by the linter (use `@import`
"https://fonts.googleapis.com/css2?family=Poppins:wght@600&display=swap";) and
remove the double quotes around the Poppins family in the font-family
declarations (change "Poppins" to Poppins in the font-family properties found at
the existing font-family lines) so the file's `@import` and font-family usages
conform to the stylelint rules.
In `@public/styles/reset.css`:
- Around line 90-91: The two font declarations conflict: "font-size: 100%" is
immediately overridden by the shorthand "font: inherit", triggering the
Stylelint rule; remove the shorthand "font: inherit" (or alternatively
remove/replace "font-size: 100%" if you intended to inherit everything) so only
one font-related declaration remains (referencing the properties "font-size" and
"font" in the CSS block).
In `@src/App.ts`:
- Around line 38-41: The scroll observer callback can re-enter and trigger
overlapping requests to search.handleLoadMoreScroll(); modify the callback
created in createScrollObserver where cleanup, sentinel and
search.handleLoadMoreScroll() are used to prevent concurrent runs by adding an
inFlight boolean (or temporarily unobserving sentinel) around the async call:
set inFlight=true (or call observer.unobserve(sentinel)) before awaiting
handleLoadMoreScroll(), and in finally set inFlight=false (or re-observe) so
only one request runs at a time; still call cleanup() when isLastPage is true.
In `@src/ModalHandler.ts`:
- Around line 29-45: The click handling duplicates the same logic because two
independent ifs can both match and call renderMovieDetail() twice; in
ModalHandler (the method containing the two if blocks that call
renderMovieDetail and this.showModal), unify the branches by combining the
selectors (e.g., use a single closest call with ".item, .primary.detail" or use
else-if) so you compute movieId once, remove the modal once, await
renderMovieDetail(movieId) a single time, then call this.showModal(); ensure you
reuse the found element rather than calling closest multiple times.
- Around line 30-35: Validate the parsed movieId and only open the modal if
renderMovieDetail succeeds: parse movieId from the clicked element and check
Number.isFinite(movieId) (or !Number.isNaN(movieId)); if invalid, abort
(optionally log or show a user-facing error) and do not call showModal. Wrap the
await renderMovieDetail(movieId) call in a try/catch (or check its returned
success value) and only call this.showModal() when renderMovieDetail completes
successfully; on failure, clean up any partial DOM (e.g., remove
".modal-container" if created) and handle/log the error instead of opening the
modal.
In `@src/movieAPIResponse.ts`:
- Around line 30-31: The URL builder in movieAPIResponse.ts concatenates
searchKeyword directly into the query string (the template using `searchKeyword`
and `searchPageCount`), which can break for spaces and reserved characters;
update the request construction to encode the query parameter (e.g., use
encodeURIComponent(searchKeyword) or build the query with URLSearchParams) so
the `search` parameter is safely escaped while keeping `page=${searchPageCount}`
unchanged; locate the template string that contains
`https://api.themoviedb.org/3/search/movie?language=ko-KR&query=${searchKeyword}&page=${searchPageCount}`
and replace the raw insertion of `searchKeyword` with an encoded value.
- Around line 7-66: Network errors from fetch can reject (e.g., offline/timeout)
and bypass your response.ok checks, causing inconsistent error types; update the
three fetch functions (the popular-movies block, fetchSearchedMovies, and
fetchMovieDetail) to wrap the fetch+json logic in a try/catch (or extract a
shared helper like safeFetch) that catches any thrown error, normalizes and
rethrows a consistent Error (include original error.message or stack) with the
existing failure text (e.g., "FAILED TO FETCH POPULAR MOVIES", "FAILED TO FETCH
SEARCHED MOVIES", "FAILED TO FETCH MOVIE DETAIL") so callers always receive
predictable errors regardless of network rejection or non-OK responses.
In `@src/movieDetailRenderer.ts`:
- Around line 25-62: The injected HTML using modalDiv.insertAdjacentHTML is
vulnerable to XSS because external fields (movieDetailData.title,
releaseDate/parseGenre(movieDetailData), movieDetailData.vote_average,
movieDetailData.overview and any poster URL) are interpolated directly; replace
the template string with DOM construction: create the container elements
(modal-image, modal-description, h2, p.category, .rate, .my-rate, .detail etc.)
via document.createElement, set text nodes via element.textContent for title,
releaseDate/genre, vote_average, overview, and set img.src only after
validating/sanitizing the poster URL, or use a trusted URL map; ensure
parseGenre output is treated as plain text (not HTML). Locate the insertion code
around modalDiv.insertAdjacentHTML and update it to append the built nodes
instead of using innerHTML.
In `@src/movieRenderer.ts`:
- Line 16: The template currently sets a non-unique id ("movie-item") for every
movie element which breaks HTML uniqueness; update the element in the template
(the string containing <div id="movie-item" data-id = "${movie.id}">) to either
use a class (class="movie-item" data-id="${movie.id}") or make the id unique
(id=`movie-item-${movie.id}`) and then update any code that references
"movie-item" (e.g., calls using getElementById or selectors) to use the new
class selector or the unique id or access via dataset (element.dataset.id)
accordingly.
In `@src/StarRating.ts`:
- Around line 16-17: Normalize and validate stored ratings coming from
storage.getRating(this.movieId) before assigning to this.currentScore: implement
a small normalization routine (e.g., clamp between 0 and 10 and round to the
nearest allowed step of 2) and call it in the constructor where
this.currentScore is set and anywhere else you read from storage (referenced
block using this.currentScore and storage.getRating and the code region around
lines 92-109). Replace direct assignment this.currentScore =
this.storage.getRating(this.movieId) with this.currentScore =
normalizeStoredScore(this.storage.getRating(this.movieId)) and ensure any setter
or update path uses the same normalizeStoredScore function so labels/ratings
remain within {0,2,4,6,8,10}.
In `@src/storage/LocalRatingStorage.ts`:
- Around line 7-14: The current getRating and setRating flow calls JSON.parse on
localStorage.getItem(this.KEY) which will throw on corrupted JSON and stop
execution; wrap the parse in a try/catch in methods like getRating and setRating
(and any helper that reads movieRatings) to recover to an empty object if
parsing fails (optionally remove or overwrite the corrupted value) and proceed;
also wrap localStorage.setItem(this.KEY, ...) in a try/catch to handle
quota/permission errors and fall back to a safe behavior (e.g., log the error
and keep in-memory state or silently no-op) so LocalRatingStorage methods
(getRating, setRating) never throw due to broken storage or setItem failures.
In `@src/utils/scrollObserver.ts`:
- Around line 5-9: The IntersectionObserver callback can call onIntersect
multiple times concurrently causing duplicate page requests; add an in-flight
guard (e.g., a boolean like isFetching) around onIntersect inside the
IntersectionObserver callback to prevent re-entry, set the flag before invoking
the async work and clear it in a finally block to ensure recovery on error, and
stop further triggers when you reach the last page by checking a
hasMore/lastPage condition or calling observer.unobserve(target) (reference:
IntersectionObserver, observer, onIntersect).
In `@templates/modal.html`:
- Around line 392-394: The close button (button.close-modal with
id="closeModal") lacks an accessible name; add an explicit label by setting an
aria-label (e.g., aria-label="Close modal") or include hidden text inside the
button (visually-hidden span with "Close") so assistive tech can announce its
purpose; ensure the change applies to the button element (not just the nested
img) and preserve the existing icon.
In `@templates/styles/thumbnail.css`:
- Around line 50-52: The current selector `.item.skeleton .item-desc > *` hides
all children including skeleton placeholders like `skeleton-title` and
`skeleton-rate`; change the selector to only target real content nodes (or
exclude skeleton nodes) by narrowing it to the actual content class names (e.g.
target `.title`, `.rate`, `.meta` children) or use a negation such as
`:not(.skeleton-*)` so skeleton elements remain visible when the
`.item.skeleton` state is present; update the selector(s) that reference
`.item-desc` accordingly (look for `.item.skeleton .item-desc > *`,
`skeleton-title`, `skeleton-rate`) and test in the real DOM to confirm only
non-skeleton content is hidden.
In `@types/MovieDetail.ts`:
- Around line 1-11: MovieDetail 인터페이스의 poster_path, release_date, overview를 TMDB
응답에 맞게 string | null로 변경하고, 이 변경에 따라 해당 타입을 사용하는 컴포넌트/함수(예: 렌더러에서 poster_path를
바로 img src로 사용하는 곳, release_date를 포맷하는 로직, overview를 바로 출력하는 컴포넌트)를 찾아 null-safe
처리로 바꾸세요 (예: 기본 포스터 이미지 또는 "정보 없음" 문자열 fallback, null 체크/옵셔널 체이닝 또는 가드 함수 적용).
이때 수정 대상 식별을 쉽게 하려면 MovieDetail 인터페이스와 poster_path/release_date/overview 참조를 먼저
찾아 변경하고, 그 참조들에서 타입 에러가 나는 지점에만 null-handling을 추가하세요.
---
Minor comments:
In `@__test__/ApiRequest.test.ts`:
- Around line 5-13: Add cleanup in an afterEach to unstub and clear the global
fetch mock introduced in beforeEach: afterEach should call vi.unstubAllGlobals()
(to remove the stubbed global "fetch") and
vi.clearAllMocks()/vi.restoreAllMocks() to reset mock state; update the test
file to add an afterEach that calls vi.unstubAllGlobals(); vi.clearAllMocks();
(referencing beforeEach, vi.stubGlobal, fetch, vi.unstubAllGlobals,
vi.clearAllMocks/vi.restoreAllMocks).
In `@cypress/e2e/modal.cy.ts`:
- Line 71: The test asserts a fixed "768px" width for .modal which conflicts
with the CSS tablet breakpoint that sets .modal { width: 100% }; update the
assertion in modal.cy.ts (the cy.get(".modal") check) to first set the viewport
to the intended tablet size and then assert the responsive behavior instead of a
hard pixel value: either assert the computed CSS is "100%"
(should("have.css","width","100%")) at that breakpoint, or assert the modal
element's actual pixel width equals the current viewport width minus known
horizontal padding (use a callback assertion like .should($el => { const vw =
Cypress.config("viewportWidth"); expect($el.outerWidth()).to.equal(vw -
<paddingSum>); }) so the test follows the CSS intent rather than hard-coding
768px.
In `@cypress/e2e/movie.cy.ts`:
- Around line 37-42: The test "마지막 페이지 도달 시 스크롤 시에 영화를 더 불러오지 않는다." only asserts
40 items after page2 loads but doesn't verify that a subsequent sentinel trigger
doesn't cause another network request or increase items; update the spec to
trigger the sentinel a second time
(cy.get("#scroll-sentinel").scrollIntoView()), assert no additional network call
was made (use cy.wait with a timeout/fail-safe or better assert request count on
the "@getMovies" / "@getMoviesPage2" aliases or use cy.intercept().as and
cy.get('@getMoviesPage2.all') to check length), and re-assert
cy.get(".thumbnail-list li").should("have.length", 40). Apply the same
additional trigger+assert logic to the other test block referenced (lines
~83-90) so both tests confirm no extra load occurs after the last page.
In `@cypress/fixtures/movies.json`:
- Around line 137-142: The last fixture entry (id 1020, title "영화 20") reuses
the same poster_path/backdrop_path values as the previous item, reducing test
distinctiveness; update the poster_path and backdrop_path fields for the object
with id 1020 (and any related "영화 20" references) to unique file names (e.g.,
/test20.jpg and /backdrop20.jpg) so each of the 20 fixtures has distinct image
paths.
In `@public/styles/tab.css`:
- Around line 8-20: Remove the stray blank lines inside the .tab-item rule to
satisfy the stylelint declaration-empty-line-before rule: open the CSS rule for
.tab-item and delete the empty lines between declarations (e.g., between
"user-select: none;" and "display: flex;" and between "height: 40px;" and
"border-radius: 40px;") so all declarations are contiguous with no extra blank
lines.
In `@README.md`:
- Around line 12-14: README 내의 페이징 관련 문구들이 '더 보기 버튼(더 보기(Pagination))'과 '무한 스크롤
전환'으로 혼재되어 있어 동작 기준이 모호하니 최종 구현 기준인 무한 스크롤 동작으로 모든 관련 문구를 통일하세요; 구체적으로 README의
"더 보기(Pagination)" 항목과 "더 보기 버튼" 언급들(현재 문제 지점: 해당 문구가 있는 블록들, 예: 12-14, 22-23,
63-65, 86-88)에 들어간 설명을 모두 "무한 스크롤로 추가 영화 로드" 또는 유사한 단일 표현으로 바꾸고, 버튼 제거/표시 관련 문구
대신 더 이상 로드할 데이터가 없을 때의 무한 스크롤 종료 동작(예: 더 이상 항목이 없을 경우 스크롤 리스너 해제 또는 완료 메시지 표시)을
기술하도록 업데이트하세요; 변경 시 '더 보기 버튼' 용어를 제거하고 '무한 스크롤', '스크롤로 추가 로드', '로딩 종료' 같은 일관된
키워드를 사용해 문장들을 통일하세요.
In `@src/StarRating.ts`:
- Line 93: StarRating.ts에 있는 사용자 노출 문구 배열에서 현재 항목 "최악이예요"의 오탈자를 수정하세요: 해당 배열(별점
라벨을 정의한 상수/변수, 즉 StarRating 내의 라벨 배열에서)에서 "최악이예요"를 찾아 "최악이에요"로 바꿔 저장하면 됩니다.
In `@templates/index.html`:
- Around line 43-46: The footer's logo <img> element
(src="/images/woowacourse_logo.png") in the <footer class="footer"> lacks an alt
attribute; add a descriptive alt attribute (e.g., alt="우아한테크코스 로고") to the <img>
to improve accessibility and convey the image purpose to assistive technologies.
- Around line 51-53: 이미지에 alt 속성이 없어 스크린 리더에서 버튼 목적을 알 수 없습니다; closeModal 버튼(요소
id: closeModal, 클래스: close-modal)에 있는 <img> 태그에 설명적인 대체 텍스트를 추가해 주세요(예: alt="모달
닫기" 또는 alt="닫기"), 필요하면 버튼 자체에도 aria-label="닫기"를 추가해 보조기기에서 명확하게 인식되도록 만드세요.
In `@templates/modal.html`:
- Around line 26-27: The img elements are missing alt attributes: add
appropriate alt text to all <img> tags (e.g., the image with class="star" next
to <span class="rate-value"> should be marked decorative with alt=""), and for
footer logo and poster images provide meaningful alt text that describes their
content (e.g., "Site logo" or the poster title). Ensure decorative images use
alt="" and informational images use concise, descriptive alt strings; update the
img tags for the star (class="star") plus the footer/logo and poster image
elements referenced in the template.
---
Nitpick comments:
In `@public/styles/colors.css`:
- Around line 11-12: 파일의 CSS 변수에 동일 색상을 의미하는 중복 토큰(--color-ffffff vs
--color-white, --color-242a32 vs --color-bluegray-100)이 섞여 있어 네이밍 체계를 통일해야 합니다;
결정(의미 기반 예: --color-white, --color-bluegray-100 또는 값 기반 예: --color-ffffff,
--color-242a32)을 내려 하나의 스타일으로 합치고 중복된 변수명 중 하나만 남긴 뒤 프로젝트 전역에서 해당 변수명을 일관되게
교체하세요(사용처 검색/치환). 또한 남긴 변수에 대해 파일 상단에 짧은 주석으로 네이밍 규칙을 적어 유지보수를 쉽게 하고, 제거한 변수와의
매핑을 잠정적으로 주석에 기록한 뒤 관련 CSS/JS 참조가 모두 업데이트되면 중복 주석을 삭제하십시오.
In `@public/styles/modal.css`:
- Around line 4-6: CSS defines a body.modal-open rule but nothing toggles it;
update your modal open/close logic to add
document.body.classList.add('modal-open') when opening the modal and remove it
on close (in the functions that currently handle modal state, e.g.,
openModal/closeModal/toggleModal or your event listeners on the modal trigger
and close button), and ensure the code also removes the class on backdrop or
escape-key dismissal so the body scroll is always restored.
In `@public/styles/thumbnail.css`:
- Around line 76-78: The selector ".item.skeleton .item-desc >
*:not(.skeleton-rate):not(.skeleton-title)" uses consecutive :not()
pseudo-classes; change it to the modern single negation with a comma-separated
list (i.e., :not(.skeleton-rate, .skeleton-title)) to satisfy Stylelint and
simplify the rule, updating the selector accordingly wherever ".item.skeleton
.item-desc > *:not(.skeleton-rate):not(.skeleton-title)" appears.
In `@src/movieRenderer.ts`:
- Around line 75-82: 함수 renderBanner의 매개변수 fristMovieData 오타를 firstMovieData로
수정하고, 내부에서 movies와 mostPopularMovie로 동일 객체를 중복 참조하지 않고 한 이름만 사용하도록 정리하세요 (예: use
firstMovieData directly); 또한 함수 선언에서 async가 있지만 await를 사용하지 않으니 비동기 작업이 필요하지 않다면
async를 제거하거나 실제 비동기 호출(예: 이미지 fetch 등)을 추가해 await를 사용하도록 변경하세요.
In `@src/SearchHandler.ts`:
- Around line 26-44: handleLoadMoreScroll currently treats equality of
totalPopularPages/searchPages to the current page count as end-of-pages but
doesn't handle error/falsy returns (e.g., 0) from renderMovies or
renderSearchedMovies; update the logic in handleLoadMoreScroll to detect error
or zero returns and stop further scrolling by treating
totalPopularPages/searchPages <= this.state.moviePageCount (or <=
this.state.searchPageCount) or total === 0/null/undefined as terminal, and
ensure any promise rejection from renderMovies/renderSearchedMovies is caught so
you return true (stop) on error; refer to handleLoadMoreScroll, renderMovies,
renderSearchedMovies, state.moviePageCount, and state.searchPageCount when
making the change.
- Around line 49-50: The code uses a non-null assertion when reading
document.querySelector<HTMLInputElement>(".search-input")!.value which will
throw if the element is missing (e.g., after replaceBanner); modify
handleSearchSubmit to safely query the element (using a local const like el =
document.querySelector<HTMLInputElement>(".search-input")), check for null, and
handle the absent case (early return, fallback value, or read from the
event/ref) before assigning this.state.currentKeyword; also consider scoping the
query to a known container or updating replaceBanner to preserve the input
element if appropriate.
In `@templates/styles/thumbnail.css`:
- Around line 25-45: Replace the hardcoded gradient colors in the skeleton
styles with CSS color tokens from your colors.css (e.g., use
var(--skeleton-base), var(--skeleton-highlight) or similar) so theme changes are
centralized; update the selectors that use the gradient (.skeleton,
.skeleton-title, .skeleton-rate) to build the same linear-gradient using those
variables and keep the existing background-size and animation properties
unchanged, and add or document the corresponding token names in colors.css if
they don't already exist.
🪄 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: be77fbe1-6086-4dbe-9b54-4c2c5d250887
⛔ Files ignored due to path filters (9)
public/images/logo.pngis excluded by!**/*.pngpublic/images/modal_button_close.pngis excluded by!**/*.pngpublic/images/no_image.pngis excluded by!**/*.pngpublic/images/planet_icon.pngis excluded by!**/*.pngpublic/images/search_icon.pngis excluded by!**/*.pngpublic/images/star_empty.pngis excluded by!**/*.pngpublic/images/star_filled.pngis excluded by!**/*.pngpublic/images/woowacourse_logo.pngis excluded by!**/*.pngtemplates/images/no_image.pngis excluded by!**/*.png
📒 Files selected for processing (39)
.gitignoreREADME.md__test__/ApiRequest.test.tscypress/e2e/modal.cy.tscypress/e2e/movie.cy.tscypress/e2e/rating.cy.tscypress/e2e/spec.cy.tscypress/fixtures/moviedetail.jsoncypress/fixtures/movies.jsoncypress/fixtures/movies2.jsoncypress/tsconfig.jsonindex.htmlpublic/styles/colors.csspublic/styles/index.csspublic/styles/main.csspublic/styles/modal.csspublic/styles/reset.csspublic/styles/tab.csspublic/styles/thumbnail.csssrc/App.tssrc/AppState.tssrc/ModalHandler.tssrc/SearchHandler.tssrc/StarRating.tssrc/main.tssrc/movieAPIResponse.tssrc/movieDetailRenderer.tssrc/movieRenderer.tssrc/storage/LocalRatingStorage.tssrc/storage/RatingStorage.tssrc/utils/scrollObserver.tstemplates/index.htmltemplates/modal.htmltemplates/styles/thumbnail.csstypes/Movie.tstypes/MovieDetail.tstypes/MovieResponse.tstypes/global.d.tstypes/vite-env.d.ts
💤 Files with no reviewable changes (2)
- cypress/e2e/spec.cy.ts
- src/main.ts
| "https://api.themoviedb.org/3/movie/popular?language=ko-KR&page=2", | ||
| { body: { results: [], total_pages: 1 } }, | ||
| ); | ||
| cy.visit("localhost:5173"); |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Cypress baseUrl 설정과 visit 사용 패턴 확인
configs="$(fd -i 'cypress.config.*')"
echo "== Cypress config files =="
echo "$configs"
if [ -n "$configs" ]; then
echo "== baseUrl declarations =="
rg -n "baseUrl" $configs || true
fi
echo "== cy.visit usages in e2e specs =="
rg -nP 'cy\.visit\(' --type=ts cypress/e2eRepository: woowacourse/javascript-movie-review
Length of output: 957
🏁 Script executed:
cat -n cypress.config.tsRepository: woowacourse/javascript-movie-review
Length of output: 316
cy.visit() 호출에서 프로토콜 누락으로 환경 의존성 발생
cypress.config.ts에 baseUrl이 설정되지 않았으므로, cy.visit()는 항상 완전한 URL(프로토콜 포함)을 필요로 합니다. 현재 라인 9, 14, 47에서 cy.visit("localhost:5173")처럼 프로토콜 없이 작성되어 있어 테스트 실행이 불안정할 수 있습니다.
- Cypress는 프로토콜이 없으면 URL을 제대로 해석하지 못할 수 있습니다.
- 같은 파일 내 다른 라인들과 다른 스펙 파일들은 모두
"http://localhost:5173"로 일관되게 작성되어 있습니다.
두 가지 접근이 가능합니다:
cypress.config.ts에baseUrl을 설정하고cy.visit("/")로 통일하는 방식- 모든
cy.visit()호출에 프로토콜을 명시하여 일관성 있게 작성하는 방식
어느 방식을 선택하든 현재 3개 라인의 불일치를 해결해야 합니다.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@cypress/e2e/movie.cy.ts` at line 9, In cypress/e2e/movie.cy.ts the cy.visit
calls (the cy.visit(...) invocations) omit the protocol causing
environment-dependent failures; fix by either setting baseUrl in
cypress.config.ts and replacing those cy.visit("localhost:5173") calls with
cy.visit("/") across the file, or update each cy.visit invocation (the three
cy.visit(...) occurrences) to include the full URL with protocol (e.g.,
"http://localhost:5173") so the tests are consistent and stable.
Comment on lines
+50
to
+61
| it("Harry Potter를 검색 하면 검색에 따른 영화를 랜더링 한다.", () => { | ||
| cy.get(".search-input").type("Harry Potter"); | ||
| cy.get(".search-button").click(); | ||
| cy.get(".thumbnail-list li").should("have.length.at.least", 1); | ||
| }); | ||
|
|
||
| it("뷁뷁뷁을 검색 하면 검색 결과가 없어야 한다.", () => { | ||
| cy.get(".search-input").type("뷁뷁뷁"); | ||
| cy.press("Enter"); | ||
| cy.get(".thumbnail-list li").should("have.length", 0); | ||
| cy.get("#no-result").contains("검색 결과가 없습니다.").should("exist"); | ||
| }); |
There was a problem hiding this comment.
검색 시나리오가 실 API에 의존해 테스트가 흔들릴 수 있습니다
Line 50-61은 intercept 없이 외부 응답에 의존합니다. CI 안정성을 위해 검색 결과 유/무 시나리오도 fixture 기반으로 고정하는 게 좋습니다.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@cypress/e2e/movie.cy.ts` around lines 50 - 61, These tests currently call the
real search API; add cy.intercept fixtures to stub responses for both scenarios
before triggering the UI: intercept the search API (match the same request your
app uses, e.g., GET /api/search or path with query) and return a fixture like
"search-harry-potter.json" for the "Harry Potter" test and "search-empty.json"
for the no-results test; place the intercepts before using
cy.get(".search-input")/cy.get(".search-button") (or cy.press("Enter")), alias
the routes (cy.as('search')) and optionally cy.wait('@search') before asserting
against ".thumbnail-list li" and "#no-result" so CI is stable and deterministic.
cypress/e2e/rating.cy.ts
Outdated
Comment on lines
+18
to
+25
| cy.window() | ||
| .its("localStorage") | ||
| .invoke("getItem", "movieRatings") | ||
| .then((val) => { | ||
| if (val) { | ||
| expect(JSON.parse(val)).to.deep.equal({ "1001": 8 }); | ||
| } | ||
| }); |
There was a problem hiding this comment.
로컬스토리지 assertion이 조건문에 가려져 실패를 놓칠 수 있습니다
Line 21-24는 val이 null이어도 테스트가 통과합니다. 저장 실패를 확실히 잡으려면 먼저 val 존재를 단정한 뒤 파싱 검증으로 이어가세요.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@cypress/e2e/rating.cy.ts` around lines 18 - 25, The test currently skips
assertions when localStorage.getItem("movieRatings") returns null; change the
check to explicitly assert existence first and only then parse and deep-equal
the value. Specifically, after cy.window().its("localStorage").invoke("getItem",
"movieRatings"), add an assertion that the returned val is not null/undefined
(e.g., expect(val).to.exist) and then JSON.parse(val) and expect the parsed
object to.deep.equal({ "1001": 8 }), so the test fails if the item was never
stored.
| @@ -0,0 +1,303 @@ | |||
| @import url("https://fonts.googleapis.com/css2?family=Poppins:wght@600&display=swap"); | |||
There was a problem hiding this comment.
Stylelint 에러 3건으로 CSS 린트가 깨집니다
- Line 1:
@import url(...)표기가 현재 규칙(import-notation)과 불일치 - Line 63, 235:
font-family의"Poppins"따옴표가 규칙과 불일치
정적 분석 결과와 동일한 지점이라, 린트 통과 기준에 맞춰 표기를 정리해 주세요.
Also applies to: 63-63, 235-235
🧰 Tools
🪛 Stylelint (17.6.0)
[error] 1-1: Expected "url("https://fonts.googleapis.com/css2?family=Poppins:wght@600&display=swap")" to be ""https://fonts.googleapis.com/css2?family=Poppins:wght@600&display=swap"" (import-notation)
(import-notation)
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@public/styles/main.css` at line 1, Replace the `@import` url(...) at the top
with the import-notation style required by the linter (use `@import`
"https://fonts.googleapis.com/css2?family=Poppins:wght@600&display=swap";) and
remove the double quotes around the Poppins family in the font-family
declarations (change "Poppins" to Poppins in the font-family properties found at
the existing font-family lines) so the file's `@import` and font-family usages
conform to the stylelint rules.
Comment on lines
+90
to
+91
| font-size: 100%; | ||
| font: inherit; |
There was a problem hiding this comment.
Stylelint 에러를 유발하는 선언 순서입니다.
font-size 직후 font: inherit;가 다시 폰트 관련 속성을 덮어써서 린트 규칙(declaration-block-no-shorthand-property-overrides)에 걸립니다. 둘 중 하나만 유지하도록 정리해 주세요.
🧰 Tools
🪛 Stylelint (17.6.0)
[error] 91-91: Unexpected shorthand "font" after "font-size" (declaration-block-no-shorthand-property-overrides)
(declaration-block-no-shorthand-property-overrides)
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@public/styles/reset.css` around lines 90 - 91, The two font declarations
conflict: "font-size: 100%" is immediately overridden by the shorthand "font:
inherit", triggering the Stylelint rule; remove the shorthand "font: inherit"
(or alternatively remove/replace "font-size: 100%" if you intended to inherit
everything) so only one font-related declaration remains (referencing the
properties "font-size" and "font" in the CSS block).
Comment on lines
+7
to
+14
| const ratings = JSON.parse(localStorage.getItem(this.KEY) || "{}"); | ||
| return ratings[movieId] || 0; | ||
| } | ||
|
|
||
| setRating(movieId: number, score: number): void { | ||
| const ratings = JSON.parse(localStorage.getItem(this.KEY) || "{}"); | ||
| ratings[movieId] = score; | ||
| localStorage.setItem(this.KEY, JSON.stringify(ratings)); |
There was a problem hiding this comment.
저장소 값이 깨졌을 때 런타임 예외로 기능이 중단될 수 있습니다.
movieRatings가 비정상 JSON이면 JSON.parse에서 바로 터집니다. 먼저 손상된 localStorage 값을 넣어 재현해 보시고, 파싱 실패 시 기본값으로 복구하는 방어 경로를 추가해 보실래요? setItem 실패(용량/권한)도 함께 처리하면 내구성이 좋아집니다.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/storage/LocalRatingStorage.ts` around lines 7 - 14, The current getRating
and setRating flow calls JSON.parse on localStorage.getItem(this.KEY) which will
throw on corrupted JSON and stop execution; wrap the parse in a try/catch in
methods like getRating and setRating (and any helper that reads movieRatings) to
recover to an empty object if parsing fails (optionally remove or overwrite the
corrupted value) and proceed; also wrap localStorage.setItem(this.KEY, ...) in a
try/catch to handle quota/permission errors and fall back to a safe behavior
(e.g., log the error and keep in-memory state or silently no-op) so
LocalRatingStorage methods (getRating, setRating) never throw due to broken
storage or setItem failures.
Comment on lines
+5
to
+9
| const observer = new IntersectionObserver((entries) => { | ||
| entries.forEach((entry) => { | ||
| if (entry.isIntersecting) { | ||
| onIntersect(); | ||
| } |
There was a problem hiding this comment.
무한 스크롤 콜백 중복 실행으로 페이지 카운트 레이스가 발생할 수 있습니다.
Line 5-9에서 교차 이벤트가 연속으로 들어오면 onIntersect가 겹쳐 실행되어 중복 요청/페이지 건너뛰기가 생길 수 있습니다.
다음 순서로 점검해보세요:
onIntersect실행 중 재진입 차단(in-flight상태)- 비동기 실패 시 상태 복구(
finally) - 마지막 페이지 도달 시 추가 트리거 차단(관찰 해제 또는 조건 가드)
As per coding guidelines "Guide the programmer in problem-solving instead of providing direct answers."
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/utils/scrollObserver.ts` around lines 5 - 9, The IntersectionObserver
callback can call onIntersect multiple times concurrently causing duplicate page
requests; add an in-flight guard (e.g., a boolean like isFetching) around
onIntersect inside the IntersectionObserver callback to prevent re-entry, set
the flag before invoking the async work and clear it in a finally block to
ensure recovery on error, and stop further triggers when you reach the last page
by checking a hasMore/lastPage condition or calling observer.unobserve(target)
(reference: IntersectionObserver, observer, onIntersect).
Comment on lines
+392
to
+394
| <button class="close-modal" id="closeModal"> | ||
| <img src="./images/modal_button_close.png" /> | ||
| </button> |
There was a problem hiding this comment.
접근성: 모달 닫기 버튼에 접근 가능한 이름이 없습니다.
아이콘 이미지 버튼만 있고 aria-label(또는 대체 텍스트)이 없어 보조기기에서 버튼 의미를 알기 어렵습니다. 닫기 버튼 자체에 명시적 라벨을 추가해 주세요.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@templates/modal.html` around lines 392 - 394, The close button
(button.close-modal with id="closeModal") lacks an accessible name; add an
explicit label by setting an aria-label (e.g., aria-label="Close modal") or
include hidden text inside the button (visually-hidden span with "Close") so
assistive tech can announce its purpose; ensure the change applies to the button
element (not just the nested img) and preserve the existing icon.
Comment on lines
+50
to
+52
| .item.skeleton .item-desc > * { | ||
| display: none; | ||
| } |
There was a problem hiding this comment.
스켈레톤 요소까지 같이 숨겨질 가능성이 큽니다.
.item.skeleton .item-desc > * { display: none; }는 skeleton-title, skeleton-rate까지 숨길 수 있습니다. 실제 DOM에서 스켈레톤 클래스가 보이는지 먼저 확인해 보시고, 숨김 대상 선택자를 일반 콘텐츠로만 좁혀 주세요.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@templates/styles/thumbnail.css` around lines 50 - 52, The current selector
`.item.skeleton .item-desc > *` hides all children including skeleton
placeholders like `skeleton-title` and `skeleton-rate`; change the selector to
only target real content nodes (or exclude skeleton nodes) by narrowing it to
the actual content class names (e.g. target `.title`, `.rate`, `.meta` children)
or use a negation such as `:not(.skeleton-*)` so skeleton elements remain
visible when the `.item.skeleton` state is present; update the selector(s) that
reference `.item-desc` accordingly (look for `.item.skeleton .item-desc > *`,
`skeleton-title`, `skeleton-rate`) and test in the real DOM to confirm only
non-skeleton content is hidden.
Comment on lines
+1
to
+11
| export interface MovieDetail { | ||
| poster_path: string; | ||
| title: string; | ||
| release_date: string; | ||
| genres: { | ||
| id: number; | ||
| name: string; | ||
| }[]; | ||
| vote_average: number; | ||
| overview: string; | ||
| } |
There was a problem hiding this comment.
🧩 Analysis chain
🌐 Web query:
TMDB v3 movie details response schema에서 poster_path, release_date, overview 필드가 nullable인지 공식 문서 기준으로 확인해줘.
💡 Result:
TMDB v3 movie details response schema에서 poster_path, release_date, overview 필드의 nullable 여부 (공식 문서 기준): - poster_path: nullable (null 가능). 공식 문서 OpenAPI 스키마에서 "type": "string"으로 정의되지만, 실제 응답 예시와 개발자 토론에서 null 값이 확인됨. 이미지 없을 때 null 반환. - release_date: nullable (null 가능). 스키마 "type": "string"이지만, 출시일 미정 영화에서 null 반환. - overview: nullable (null 가능). 스키마 "type": "string"이지만, 설명 없을 때 빈 문자열 또는 null 반환 가능. 서드파티 스키마에서 string | null로 명시. 공식 OpenAPI 스키마는 OpenAPI 3에서 string 타입이 null을 허용하지 않으나, TMDB 실제 응답은 null을 반환하므로 클라이언트에서 null 체크 필요. 개발자 포럼에서 poster_path null 처리 논의 다수 확인.
Citations:
- 1: https://developer.themoviedb.org/reference/movie-details
- 2: https://www.mintlify.com/otniel0820/connect-world-v1/api/tmdb
- 3: https://www.themoviedb.org/talk/595ea1ea925141225107185d
- 4: https://www.themoviedb.org/talk/61e8a6c1fdf8b700c34ae063
- 5: https://developer.themoviedb.org/docs/image-basics
- 6: https://developer.themoviedb.org/openapi
TMDB API의 실제 응답에 맞춰 nullable 필드를 타입에 반영하세요.
MovieDetail 인터페이스에서 poster_path, release_date, overview가 모두 non-null string으로 정의되어 있습니다. 하지만 TMDB API는 다음과 같은 경우 이 필드들에 null을 반환합니다:
poster_path: 포스터 이미지가 없을 때release_date: 출시일이 미정인 영화overview: 설명이 없을 때
이 불일치로 인해 런타임에 null 값을 예상하지 못해 렌더링이 깨질 수 있습니다.
이를 해결하기 위해 다음을 생각해 보세요:
- 타입 정의에서 어느 필드가 실제로 nullable인지 파악했나요?
- nullable 필드에 대해 UI에서 어떻게 안전하게 fallback 처리할 수 있을까요? (예: 기본 이미지 사용, "정보 없음" 표시)
- 타입 안정성을 위해 이 필드들을
string | null로 수정하면 어떤 곳에서 타입 에러가 발생할까요?
이 접근 방식을 통해 타입과 실제 API 동작의 간극을 줄일 수 있습니다.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@types/MovieDetail.ts` around lines 1 - 11, MovieDetail 인터페이스의 poster_path,
release_date, overview를 TMDB 응답에 맞게 string | null로 변경하고, 이 변경에 따라 해당 타입을 사용하는
컴포넌트/함수(예: 렌더러에서 poster_path를 바로 img src로 사용하는 곳, release_date를 포맷하는 로직,
overview를 바로 출력하는 컴포넌트)를 찾아 null-safe 처리로 바꾸세요 (예: 기본 포스터 이미지 또는 "정보 없음" 문자열
fallback, null 체크/옵셔널 체이닝 또는 가드 함수 적용). 이때 수정 대상 식별을 쉽게 하려면 MovieDetail 인터페이스와
poster_path/release_date/overview 참조를 먼저 찾아 변경하고, 그 참조들에서 타입 에러가 나는 지점에만
null-handling을 추가하세요.
Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
cys4585
requested changes
Apr 12, 2026
cys4585
left a comment
cys4585
left a comment
There was a problem hiding this comment.
벤지 안녕하세요!
2단계 미션 수고하셨습니다.
동작 확인
배포 링크에서 동작을 확인해봤어요. 영화 목록, 무한 스크롤, 검색, 모달(열기/닫기 모두), 별점, 새로고침 후 별점 유지까지 잘 동작하네요!
몇 가지 이슈가 있었어요:
- 초기 진입 시 네트워크 탭을 보면 1페이지와 2페이지가 동시에 요청되고 있어요. 검색에서도 마찬가지예요.
- MovieList 로고를 클릭하면 앱이 아니라
https://hjkim0905.github.io/로 이동해요. - 목록 가져오기 요청을 실패시킨 후 다시 스크롤하면, 실패한 페이지는 건너뛰고 다음 페이지를 호출해요.
- 모달이 열린 상태에서 배경이 스크롤돼요.
원인이 될 만한 코드에 인라인 코멘트를 남겼으니 같이 봐주세요!
설계
1단계에서 App.ts에 몰려있던 핸들러를 SearchHandler, ModalHandler로 분리한 것 아주 좋은 개선입니다!
이렇게 분리하고 나니 각 모듈의 역할 경계가 눈에 띄어요. SearchHandler가 상태 관리도 하고, DOM 직접 조작도 하고, 렌더러 호출도 하고 있고요. movieRenderer는 API 호출부터 DOM 렌더링, 에러 처리까지 모두 담당하고 있어요. 그래서, 각 모듈의 역할을 뭐라고 정의하셨는지, 벤지 생각하시는지 궁금합니다!
그리고 한 가지 흥미로운 점이 있는데, StarRating은 자기 DOM 참조, 자기 이벤트 바인딩, 자기 상태 관리, 자기 UI 업데이트를 모두 자기 안에서 하는 구조인 반면, 나머지 모듈들은 App.ts가 이벤트를 등록하고 핸들러는 콜백만 제공하는 구조예요. 두 가지 패턴이 혼재되어 있는데, 이건 의도적인 선택인지 궁금합니다!
localStorage 추상화
RatingStorage 인터페이스를 분리하고 StarRating이 인터페이스에만 의존하도록 한 건 미션 요구사항에 딱 맞는 설계예요! LocalRatingStorage에서 implements RatingStorage를 명시한 것도 좋고요. 인라인에서 몇 가지 더 짚었으니 같이 봐주세요.
PR 본문 질문 답변
1. LocalRatingStorage를 클래스로 분리한 것이 적절한가?
미션 요구사항 자체가 "서버 API로 교체할 수 있는 구조"이니까 과한 추상화가 아니라고 생각합니다. 확정된 요구사항에 대한 아주 적절한 설계라고 볼 수 있을 것 같아요.
2. 고정된 요소 개수에서도 이벤트 위임이 나은가?
별 5개처럼 고정된 소수 요소에서는 직접 리스너도 충분하다고 봐요. 이벤트 위임이 필수는 아니니까요. 일관성 있게 선택한 거라면 괜찮아요.
3. E2E 테스트에서 localStorage를 직접 검증하는 것이 올바른가?
저도 벤지의 생각과 동일할 것 같습니다! 인라인에서 짚었으니 같이 봐주세요!
E2E 테스트
주요 유저 시나리오를 잘 커버하고 있는 것 같아요. 모달 열기/닫기, 반응형 테스트, 스켈레톤 UI, 검색 결과 없음까지 다루고 있는 것 좋네요!
| @@ -21,97 +20,26 @@ class App { | |||
| } | |||
There was a problem hiding this comment.
초기 1페이지와 2페이지가 동시에 요청되는 문제가 이 부분 때문인 것 같아요. 왜 문제가 발생하는지, 어떻게 해결하면 좋을지 궁금합니다!
There was a problem hiding this comment.
검색이나 무한 스크롤은 SearchHandler를 거치는데, 초기 로드만 여기서 렌더러를 직접 호출하고 있네요. 요것도 Handler 쪽으로 역할을 넘기면 좋을 것 같습니다! (기존 코드는 안건들려고 하신게 아닐까 추측해봅니다!)
Author
There was a problem hiding this comment.
원인은 IntersectionObserver로부터 있었습니다.
observe()호출 직후 sentinel이 뷰포트 안에 있으면 즉시 한 번 콜백을 실행하는데 이 과정에서 1페이지 데이터를 불러오기전에 sentinel이 뷰포트안에서 보이기 때문에 바로 콜백이 실행되서 2페이지 요청이 바로 나가는 문제였습니다. 따라서 App.ts에서도 async init() 메서드를 만들고 그 안에 1페이지가 다 불러질때까지 기다리게 await을 붙여 해결했습니다.
다만 MovieBrowseHandler.ts로 역할을 옮겨 init에서 renderMovies로 초기 로드를 할 수 있게 대처했습니다!!!
이렇게 진행하니 App.ts는 정말 초기 진입점으로서의 역할을 다하고 있다고 생각이 들고 어떤 핸들러 클래스가 바뀌어도 App.ts는 내부 수정은 몰라도 된다는 큰 이점이 생긴 것 같습니다:)
src/App.ts
Outdated
Comment on lines
35
to
42
| const sentinel = document.querySelector<HTMLElement>("#scroll-sentinel"); | ||
| if (sentinel) { | ||
| let cleanup: () => void; | ||
| cleanup = createScrollObserver(sentinel, async () => { | ||
| const isLastPage = await search.handleLoadMoreScroll(); | ||
| if (isLastPage) cleanup(); | ||
| }); | ||
| } |
There was a problem hiding this comment.
무한 스크롤만 여기에 로직이 인라인으로 풀어져 있네요. 그리고 무한 스크롤 관련 코드가 흩어져 있어서 있는 것 같아요. 한 곳에서 관리하면 어떨지 생각해보면 좋겠습니다!
Author
There was a problem hiding this comment.
아래에서 각 모듈이 각 모듈의 도메인에 맞는 이벤트를 스스로 관리하도록 init()함수를 추가하여 해결하는 과정에서 무한 스크롤 관련 로직도 SearchHandler 클래스에 포함시켰습니다!
다만 여기에서 드는 의문점이 네이밍을 잘못했다 생각이 들었습니다. 검색관련 로직뿐만 아니라 무한스크롤을 포함한 영화 목록의 탐색 전반을 이 클래스에서 다루고 있는데, 네이밍을 MovieBrowseHandler로 바꾸어야 더 명확하게 이 클래스가 어떤 도메인을 다루고 있는지 확실하게 알 수 있을 것 같아 바꾸었습니다!
Comment on lines
+26
to
+44
| handleLoadMoreScroll = async () => { | ||
| if (!this.state.isSearched) { | ||
| this.state.moviePageCount += 1; | ||
| const totalPopularPages = await renderMovies(this.state.moviePageCount); | ||
| if (totalPopularPages === this.state.moviePageCount) { | ||
| return true; | ||
| } | ||
| } else { | ||
| this.state.searchPageCount += 1; | ||
| const totalSearchPages = await renderSearchedMovies( | ||
| this.state.currentKeyword, | ||
| this.state.searchPageCount, | ||
| ); | ||
| if (totalSearchPages === this.state.searchPageCount) { | ||
| return true; | ||
| } | ||
| } | ||
| return false; | ||
| }; |
There was a problem hiding this comment.
네트워크 실패 시 page가 건너뛰는 문제가 이 부분 때문인 것 같네요! 어떻게 해결하면 좋을까요!
| } | ||
| }; | ||
| } | ||
|
|
There was a problem hiding this comment.
여기서 thumbnail-list 비우기, #header 교체, #section-title 텍스트 변경을 직접 하고 있는데, 이건 렌더러의 역할이 아닐까요?
Comment on lines
+6
to
+26
| // 모달 닫는 핸들러 | ||
| handleModalCloseButtonClick = (e: MouseEvent) => { | ||
| if ((e.target as HTMLElement).closest("#closeModal")) { | ||
| document.querySelector(".modal-container")?.remove(); | ||
| this.modalArea?.classList.remove("active"); | ||
| } | ||
| }; | ||
|
|
||
| handleModalCloseButtonKeyDown = (e: KeyboardEvent) => { | ||
| if (e.key === "Escape") { | ||
| document.querySelector(".modal-container")?.remove(); | ||
| this.modalArea?.classList.remove("active"); | ||
| } | ||
| }; | ||
|
|
||
| handleModalCloseBackdrop = (e: MouseEvent) => { | ||
| if ((e.target as HTMLElement) === this.modalArea) { | ||
| document.querySelector(".modal-container")?.remove(); | ||
| this.modalArea?.classList.remove("active"); | ||
| } | ||
| }; |
Comment on lines
+1
to
+2
| import { RatingStorage } from "./storage/RatingStorage"; | ||
|
|
Comment on lines
+7
to
+8
| const ratings = JSON.parse(localStorage.getItem(this.KEY) || "{}"); | ||
| return ratings[movieId] || 0; |
There was a problem hiding this comment.
여기 JSON에서 에러가 발생할 경우는 없을까요?
있다면, 예외처리는 어떻게 하면 좋을까요?
Comment on lines
+3
to
+4
| export class LocalRatingStorage implements RatingStorage { | ||
| private readonly KEY = "movieRatings"; |
There was a problem hiding this comment.
implements RatingStorage로 인터페이스를 명시한 거 좋네요! 👍
Author
There was a problem hiding this comment.
감사합니다!
말씀하신 것처럼 나중에 서버 API나 다른 저장소로 전환이 필요할 때 RatingStorage 인터페이스를 구현하는 클래스를 새로 추가하기만 하면 되도록 확장 가능성을 열어두었습니다:)
Comment on lines
+1
to
+4
| export interface RatingStorage { | ||
| getRating(movieId: number): number; | ||
| setRating(movieId: number, score: number): void; | ||
| } |
There was a problem hiding this comment.
지금은 동기 시그니처인데, 서버 API로 교체하면 비동기(Promise)가 되어야 할 것 같아요. 그때 인터페이스 시그니처가 바뀌면 StarRating도 수정해야 하는데, 이 부분 어떻게 생각하시나요?
Author
There was a problem hiding this comment.
지금 규모에서는 굳이 Promise로 만들 필요가 없다고 생각했습니다.
서버 API로 전환할 때 RatingStorage 인터페이스의 반환 타입과 StarRating에서 await를 추가하는 정도의 수정으로 충분하고, 그 변경량도 크지 않다고 생각이 드는데요.
반면 지금부터 Promise로 맞춰놓으면 LocalRatingStorage에서 Promise.resolve()로 억지로 감싸야 하고, StarRating 생성자에서 await를 쓸 수 없어 초기화 로직이 복잡해지는 문제가 생길 수도 있다고 생각이 들어 현시점에서는 오버엔지니어링이라고 판단했습니다!
수야의 의견은 어떠신지 궁금합니다!
cypress/e2e/rating.cy.ts
Outdated
Comment on lines
+16
to
+26
| it("별점을 클릭하면 로컬스토리지에 저장된다", () => { | ||
| cy.get(".my-rate-stars .my-star").eq(3).click(); | ||
| cy.window() | ||
| .its("localStorage") | ||
| .invoke("getItem", "movieRatings") | ||
| .then((val) => { | ||
| if (val) { | ||
| expect(JSON.parse(val)).to.deep.equal({ "1001": 8 }); | ||
| } | ||
| }); | ||
| }); |
There was a problem hiding this comment.
아래 테스트("새로고침 후 별점 유지")가 사용자 관점에서 검증하고 있는데, 이 로컬스토리지 저장 테스트가 필요할까요?
Author
There was a problem hiding this comment.
맞습니다! PR 본문에서도 고민했던 부분인데, 결론적으로 해당 테스트를 제거했습니다.
localStorage 직접 검증은 구현 세부사항을 테스트하는 것이고, "새로고침 후 별점 유지" 테스트가 이미 사용자 관점에서 동일한 동작을 검증하고 있어 중복이라고 판단했습니다. E2E 테스트의 목적이 사용자 시나리오 검증이라면 후자만으로 충분하다고 생각합니다!
Author
|
movieRenderer는 인기 영화 페이지와 검색 페이지에서 공통으로 사용되는 렌더링 관련 함수들을 도메인별로 모아둔 모듈입니다. SearchHandler는 검색 관련 이벤트 핸들러와 무한 스크롤 로직을 한 곳에 모아 관리하고자 했습니다. StarRating은 모달 내부의 특정 영역만을 담당하는 컴포넌트로, 해당 기능을 수정할 때 이 파일만 건드리면 되도록 자기 참조와 이벤트 바인딩을 내부에서 처리했습니다. 이벤트 등록을 각 클래스 안으로 옮기는 방안도 고려했지만, 클래스 길이가 늘어나는 것에 부담을 느껴 App.ts에서 일괄 등록하는 방식을 선택했습니다. 따라서 각 모듈이 각 모듈의 도메인에 맞는 이벤트를 스스로 관리하도록 init()함수를 추가하여 해결했습니다! |
hjkim0905
changed the title
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) 이번 단계에서 가장 많이 고민했던 문제와 해결 과정에서 배운 점
이벤트 핸들러가 늘어날수록 App.ts가 비대해지고 서로 다른 관심사가 뒤섞이는 문제가 생겼습니다. 검색 관련 핸들러(검색 버튼 클릭, 엔터 키 입력, 무한 스크롤)와 모달 관련 핸들러(모달 열기/닫기, ESC 키, 배경 클릭)는 서로 다른 관심사를 다루기 때문에 각각 SearchHandler, ModalHandler로 분리했습니다. 결과적으로 App.ts는 초기화와 이벤트 등록 역할만 담당하도록 했습니다.
영화 아이템에서 모달을 열 때 필요한 movieId는 data-* 속성을 통한 전달만으로 충분했지만, 별점 기능은 다르다고 판단했습니다. 제가 오랫동안 고민해서 생각한 클래스 분리 기준은 '상태를 지속적으로 업데이트해야 하는가'와 '독립적인 객체로 추상화할 수 있는가'인데, 별점은 마우스 호버와 클릭에 따라 실시간으로 점수 상태를 추적하고 별 이미지와 평가 텍스트 등의 UI를 지속적으로 갱신해야 합니다. 따라서 단순 식별자 전달에 그치는 id와 달리, StarRating이라는 독립적인 클래스로 캡슐화하는 것이 복잡도를 낮추고 유지보수에 적합하다고 판단했습니다.
처음에는 각 별 이미지마다 mouseenter/mouseleave 리스너를 직접 부착하려 했습니다. 그런데 mouseenter/mouseleave는 버블링이 발생하지 않아 부모 요소에 리스너를 달면 자식 요소의 이벤트를 감지하지 못한다는 문제가 있었습니다. 반면 mouseover/mouseout은 버블링이 발생하므로, 컨테이너에 리스너를 하나만 달고 e.target.closest()로 별 이미지를 특정하는 이벤트 위임 방식으로 전환했습니다. 이를 통해 리스너 수를 줄이고 동적으로 추가되는 요소에도 자연스럽게 대응할 수 있게 된 것 같습니다.
블로그를 통해 공부했을 때 기존에 알고 있던 window.addEventListener('scroll', ...) 방식은 스크롤 시 1초에 수십~수백 번 이벤트가 발생해 성능 저하가 발생하기 쉽다는 것을 꺠달았습니다. 이를 개선하기 위해 IntersectionObserver를 도입했습니다. IntersectionObserver는 브라우저가 백그라운드에서 비동기적으로 요소를 관찰하며, 요소가 뷰포트에 진입하거나 벗어날 때만 콜백을 실행하기 때문에 불필요한 연산을 크게 줄일 수 있고, 또한 마지막 페이지 도달 시 메모리 누수를 방지하기 위해 클린업 함수를 반환하는 구조로 작성했으며, 인기 영화 목록과 검색 결과 모두에서 재사용할 수 있도록 utils/scrollObserver.ts로 분리했습니다!
처음에는 StarRating 클래스 내부에서 localStorage를 직접 읽고 쓰는 방식으로 구현했습니다. 이 방식은 나중에 서버 API로 교체할 때 StarRating 내부 코드를 직접 수정해야 한다는 문제가 있었습니다. 그래서 RatingStorage 인터페이스를 별도로 분리하고 LocalRatingStorage 구현체를 만들어 StarRating 생성자에 주입하는 방향으로 변경했습니다. 이렇게 하면 ApiRatingStorage를 새로 만들어 주입만 바꾸면 StarRating 코드는 전혀 수정하지 않아도 되고, 테스트 시에도 mock storage를 주입해 localStorage 없이 단위 테스트할 수 있다는 장점이 있었던 것 같습니다.
2) 이번 리뷰를 통해 논의하고 싶은 부분
저는 '상태를 지속적으로 업데이트해야 하는가'와 '독립적인 객체로 추상화할 수 있는가'를 클래스 분리의 기준으로 삼았습니다. 그런데 LocalRatingStorage는 이 기준보다는 다른 이유로 클래스로 분리했습니다. 현재는 localStorage를 사용하지만, 추후 서버 API로 교체될 가능성을 고려해 StarRating의 도메인 로직을 건드리지 않고 저장소 구현체만 교체할 수 있도록 RatingStorage 인터페이스와 LocalRatingStorage 구현체로 분리했습니다. 즉, "지금은 임시 구현이지만 나중에 교체될 것"이라는 예측을 근거로 인터페이스를 도입한 셈인데, 이처럼 미래 교체 가능성을 이유로 추상화 레이어를 미리 두는 것이 현 시점에서 적절한 설계인지, 아니면 과한 추상화인지 수야의 의견이 궁금합니다.
현재 StarRating은 컨테이너(.my-rate) 전체에 mouseover/mouseout/click 리스너를 달고 e.target.closest()로 별 이미지를 특정하는 이벤트 위임 방식을 사용하고 있습니다. 이 방식이 리스너 수를 줄이고 동적 요소에 대응하기 좋다고 판단했는데, StarRating처럼 요소 개수가 고정된(별 5개) 경우에도 이벤트 위임 방식이 더 나은 선택인지, 아니면 각 별에 직접 리스너를 부착하는 방식이 더 명확할 수 있는지 의견을 듣고 싶습니다.
rating.cy.ts에서 별점 클릭 후 cy.window().its('localStorage')...로 localStorage에 저장된 값을 직접 확인하는 테스트를 작성했습니다. 그런데 E2E 테스트는 사용자 시나리오를 검증하는 것이 목적이라면, 저장소 구현 세부사항인 localStorage를 직접 들여다보는 것이 E2E 테스트의 역할 범위를 벗어난 것은 아닌지 고민이 됩니다. "새로고침 후 같은 영화를 열었을 때 별점이 유지된다"는 시나리오 검증만으로 충분한지, 아니면 localStorage 직접 검증도 E2E 테스트에서 유효한 접근인지 수야의 의견이 궁금합니다.
또한 movie.cy.ts에서 무한 스크롤 테스트 시 #scroll-sentinel을 scrollIntoView()로 강제로 끌어당기는 방식을 쓰는데, 실제 사용자의 자연스러운 스크롤 동작과 다르다는 점에서 이게 유효한 테스트인지도 혼란이 있는 것 같습니다.
수야 안녕하세요! 2단계 리뷰도 맡아주셔서 감사합니다!
배포링크 최신으로 업데이트 하였습니다!
아래는 주요스펙입니다!
주요 스펙 및 구현 결정사항
영화 목록 조회
더보기버튼 클릭 시 다음 페이지를 추가로 불러옵니다.더보기버튼을 숨깁니다.검색
영화 상세 정보 조회
별점 매기기
무한 스크롤
반응형 레이아웃
아래는 모듈 관계도 입니다!
flowchart TD App["App.ts 초기화 · 이벤트 등록"] AppState["AppState.ts 페이지 · 검색 상태"] SearchHandler["SearchHandler.ts 검색 · 무한스크롤"] ModalHandler["ModalHandler.ts 모달 열기 · 닫기"] scrollObserver["scrollObserver.ts IntersectionObserver"] movieRenderer["movieRenderer.ts 목록 · 배너 렌더링"] movieDetailRenderer["movieDetailRenderer.ts 상세 모달 렌더링"] movieAPIResponse["movieAPIResponse.ts TMDB API 호출"] StarRating["StarRating.ts 별점 UI · 상태"] LocalRatingStorage["LocalRatingStorage.ts localStorage"] RatingStorage["interface RatingStorage.ts"] App --> AppState App --> SearchHandler App --> ModalHandler App --> scrollObserver SearchHandler --> movieRenderer ModalHandler --> movieDetailRenderer movieRenderer --> movieAPIResponse movieDetailRenderer --> movieAPIResponse movieDetailRenderer --> StarRating StarRating --> LocalRatingStorage LocalRatingStorage -.-> RatingStorage✅ 리뷰어 체크 포인트