[2단계 - 영화 목록 불러오기] 포도 미션 제출합니다.#304
[2단계 - 영화 목록 불러오기] 포도 미션 제출합니다.#304goodsmell wants to merge 60 commits intowoowacourse:goodsmellfrom
Conversation
Co-authored-by: jebiyeon02 <jhje5595@naver.com>
Co-authored-by: jebiyeon02 <jhje5595@naver.com>
Co-authored-by: jebiyeon02 <jhje5595@naver.com>
Co-authored-by: jebiyeon02 <jhje5595@naver.com>
Co-authored-by: jebiyeon02 <jhje5595@naver.com>
Co-authored-by: jebiyeon02 <jhje5595@naver.com>
Co-authored-by: jebiyeon02 <jhje5595@naver.com>
Co-authored-by: jebiyeon02 <jhje5595@naver.com>
Co-authored-by: jebiyeon02 <jhje5595@naver.com>
Co-authored-by: jebiyeon02 <jhje5595@naver.com>
Co-authored-by: jebiyeon02 <jhje5595@naver.com>
Co-authored-by: JeongYeonJun <jebiyeon02@users.noreply.github.com>
Co-authored-by: jebiyeon02 <jhje5595@naver.com>
Co-authored-by: jebiyeon02 <jhje5595@naver.com>
Co-authored-by: jebiyeon02 <jhje5595@naver.com>
Co-authored-by: jebiyeon02 <jhje5595@naver.com>
Co-authored-by: jebiyeon02 <jhje5595@naver.com>
Co-authored-by: jebiyeon02 <jhje5595@naver.com>
Co-authored-by: jebiyeon02 <jhje5595@naver.com>
|
Important Review skippedAuto incremental reviews are disabled on this repository. Please check the settings in the CodeRabbit UI or the ⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Run ID: You can disable this status message by setting the Use the checkbox below for a quick retry:
Walkthrough이 변경사항은 영화 검색 및 리뷰 애플리케이션의 전체 구현을 추가합니다. 새로운 GitHub Actions CI/CD 워크플로우, 완전히 재구성된 HTML 레이아웃, 여러 개의 TypeScript 컴포넌트(검색 폼, 모달, 평점 UI), API 페칭 로직, 클라이언트 상태 관리, 스타일시트들, 그리고 종합적인 Vitest 단위 테스트 및 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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 20
🧹 Nitpick comments (12)
src/utils/getYear.ts (1)
1-4: 날짜 포맷 예외 처리 기준을 먼저 정해보면 좋겠습니다.
YYYY-MM-DD가 아닌 값(빈 문자열 포함)이 들어올 때, UI에서 어떤 값을 보여줄지(fallback) 먼저 결정하고 그 규칙을 이 유틸에 명시하면 안정성이 올라갑니다.As per coding guidelines "Break problems into smaller, manageable steps to help think through them logically."
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/utils/getYear.ts` around lines 1 - 4, The getYear function currently assumes date is "YYYY-MM-DD"; add explicit input validation in getYear (and use the parseDate variable) so non-conforming values (empty string, malformed strings) return a defined fallback; implement a simple validation (e.g. regex for four-digit year with optional -MM-DD) and return the decided fallback (e.g. '' or 'Unknown') when validation fails, otherwise extract and return the year as before.src/components/MovieItem.ts (1)
17-28:bindEvent()중복 호출 방어를 고려해 주세요.현재는 호출될 때마다 리스너가 추가됩니다. 이후 리팩터링/재초기화 시 중복 이벤트로 모달 오픈/요청이 여러 번 실행될 수 있으니, 한 번만 바인딩되도록 가드하는 구조가 더 안전합니다.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/components/MovieItem.ts` around lines 17 - 28, The bindEvent() method currently adds a click listener to this.#thumbnailList every time it's called, causing duplicate handlers; modify bindEvent() to guard so the listener is only attached once (e.g., track a boolean like this._eventsBound or store the bound handler on an instance property), check that flag or existing handler before calling addEventListener, and ensure you still reference the same handler when removing listeners if needed; update references to bindEvent(), this.#thumbnailList, and handleClick to use the single stored listener/flag approach so handleClick is invoked only once per click.__tests__/movie.test.ts (2)
29-32:afterEach에서 mock 정리 추가 권장
beforeEach에서vi.stubGlobal("fetch", vi.fn())을 설정하지만, 테스트 후 정리하는 코드가 없습니다. 테스트 간 격리를 보장하려면afterEach에서vi.restoreAllMocks()또는vi.unstubAllGlobals()를 호출하는 것이 좋습니다.제안
beforeEach(() => { vi.stubGlobal("fetch", vi.fn()); }); + +afterEach(() => { + vi.unstubAllGlobals(); +});🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@__tests__/movie.test.ts` around lines 29 - 32, Add test teardown to restore the mocked global fetch: after using vi.stubGlobal("fetch", vi.fn()) in beforeEach, add an afterEach that calls vi.restoreAllMocks() or vi.unstubAllGlobals() to clean up the stub between tests so mocks set in beforeEach are removed; reference the existing beforeEach and add afterEach accordingly.
7-26: 실제 API 호출 대신 모킹 사용 고려이 테스트들은 실제 외부 API를 호출하고 있습니다. 이로 인해 발생할 수 있는 문제점들이 있는데요:
- 네트워크 불안정 시 테스트 실패 (flaky tests)
- API 응답이 항상 정확히 20개라는 보장이 없음
- CI에서 rate limiting에 걸릴 수 있음
- 테스트 실행 속도 저하
에러 테스트(라인 29-67)에서는
fetch를 모킹하고 있는데, 성공 케이스에서도 동일하게 모킹하면 어떨까요? 어떤 이유로 실제 API 호출 방식을 선택하셨는지 궁금합니다.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@__tests__/movie.test.ts` around lines 7 - 26, These tests call external APIs directly via fetchPopularMovies and fetchSearchedMovies—make them use the same fetch mocking approach as the error tests to avoid flaky, slow, or rate-limited CI runs: locate the tests using fetchPopularMovies and fetchSearchedMovies and install a mock implementation for global.fetch (or jest.spyOn(window, "fetch")) that returns a successful Response-like object with a predictable JSON payload (e.g., 20 items) and proper status, reuse the mocking utilities or fixtures used in the error test to keep behavior consistent, and ensure you restore/clear the mock between tests so other specs are unaffected.src/main.ts (1)
91-93: 에러 로깅에console.error사용 권장
console.log(error)대신console.error(error)를 사용하면 브라우저 개발자 도구에서 에러로 분류되어 더 쉽게 식별할 수 있습니다.제안
} catch (error) { - console.log(error); + console.error(error); alert("영화 정보를 불러오는 데 실패했습니다. 잠시 후 다시 시도해주세요.");🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/main.ts` around lines 91 - 93, In the catch block handling the movie-fetch failure (the catch that currently does console.log(error) and alert(...)), replace console.log(error) with console.error(error) so the error is recorded as an actual error in browser devtools; keep the existing alert unchanged and ensure the catch continues to reference the same error variable.src/store.ts (1)
13-17:setPopularMode()메서드 사용 일관성 점검
setPopularMode()메서드가 정의되어 있지만,src/main.ts의resetToPopularView()함수(라인 66-68)에서는 이 메서드를 사용하지 않고 직접 속성을 변경하고 있습니다:PageStore.mode = "popular"; PageStore.query = ""; PageStore.page = 1;이렇게 하면 나중에
setPopularMode()로직이 변경될 경우 불일치가 발생할 수 있습니다. 왜 직접 변경 방식을 선택했는지, 또는setPopularMode()를 사용하도록 리팩터링할 계획이 있는지 고려해 보시겠어요?🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/store.ts` around lines 13 - 17, The resetToPopularView function is mutating PageStore properties directly instead of using the existing setPopularMode() method; update resetToPopularView to call PageStore.setPopularMode() (or delegate to that method) so all logic for switching to popular mode is centralized in setPopularMode(), preventing inconsistent behavior if setPopularMode() changes in the future.cypress/e2e/search.cy.ts (2)
78-88: 테스트 3: 추가 요청이 발생하지 않음을 더 명확히 검증 가능현재는 아이템 수가 30개로 유지되는지만 확인하고 있습니다. 실제로 추가 API 요청이 없었음을 검증하려면 spy나 alias 호출 횟수를 확인하는 방법도 있습니다.
예를 들어,
@searchP2가 정확히 1번만 호출되었는지 확인하거나, 마지막 스크롤 후 짧은 대기 시간을 두고 요청이 없음을 검증할 수 있습니다. 현재 접근 방식도 기능적으로는 충분하지만, 더 명시적인 검증을 원한다면 고려해 보세요.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@cypress/e2e/search.cy.ts` around lines 78 - 88, The test "3. 마지막 페이지까지 불러오면 더 이상 요청이 발생하지 않는다." should explicitly assert that no extra API calls occur after the last page: keep the existing flow (call searchMovie("인사이드"), wait for "@searchP1" and "@searchP2", scrollTo bottom) but add an assertion that the alias "@searchP2" was called exactly once (or that the request count did not increase) and/or add a short cy.wait then assert no additional calls; reference the test case name, the searchMovie helper, the "@searchP2" alias and the ".thumbnail-list li" selector to locate where to add the call-count assertion or extra wait+no-call check.
55-55: 하드코딩된 URL 대신 상대 경로 또는 baseUrl 사용 권장
cy.visit("http://localhost:5173/")에서 URL이 하드코딩되어 있습니다. Cypress 설정 파일(cypress.config.ts)에서baseUrl을 설정하면cy.visit("/")로 간단하게 사용할 수 있고, 환경별로 다른 URL을 쉽게 적용할 수 있습니다.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@cypress/e2e/search.cy.ts` at line 55, Replace the hardcoded URL in the test where cy.visit("http://localhost:5173/") is called in search.cy.ts with a relative path using the configured Cypress baseUrl (e.g., cy.visit("/") ) so the test honors environment-specific baseUrl from cypress.config.ts; update the cy.visit call accordingly so it relies on Cypress' baseUrl setting rather than a fixed host.src/components/SearchForm.ts (1)
77-86:removeSkeleton중복 호출 검토
loadMore콜백 내에서removeSkeleton()이 두 번 호출될 수 있습니다:
- Line 81: try 블록 내에서 호출
- Line 85: finally 블록에서 호출
finally는 항상 실행되므로, Line 81의 호출은 제거해도 됩니다. 현재 동작에 문제는 없지만, 불필요한 DOM 조작이 발생합니다.
왜 finally 블록만으로 충분한지 생각해 보시겠어요?
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/components/SearchForm.ts` around lines 77 - 86, In loadMore, remove the duplicate removeSkeleton() call inside the try block because the finally block already guarantees cleanup; keep renderSkeleton() at the start, await this.lodeList(PageStore.page + 1), call renderMoviesList(movies, { append: true }, loadMore) when movies exist, and rely on the finally removeSkeleton() to always run so you avoid redundant DOM manipulation.src/components/review.ts (1)
70-73:firstChild에 대한 null 체크 고려
this.#labelEl.firstChild!에서 non-null assertion(!)을 사용하고 있습니다. 만약.rating-text요소에 자식 노드가 없는 경우 런타임 에러가 발생할 수 있습니다.이 경우 어떤 상황에서 문제가 될 수 있을지, 그리고 더 안전한 접근 방식은 무엇일지 생각해 보시겠어요? 힌트:
textContent를 부모 요소에 직접 설정하는 방식과 비교해 보세요.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/components/review.ts` around lines 70 - 73, `#updateText에서` this.#labelEl.firstChild!의 non-null assertion 사용은 자식 노드가 없을 경우 런타임 에러를 유발하므로 안전하게 수정해야 합니다; 대신 this.#labelEl의 textContent를 직접 설정하거나(예: this.#labelEl.textContent = RATING_LABELS[rating] + " ") 또는 firstChild가 없을 때 새 텍스트 노드를 생성하도록 방어 코드로 처리하여 RATING_LABELS[rating] 값을 안전하게 배치하고 null 문제를 방지하세요 (참조: `#updateText`, `#labelEl`, `#scoreEl`, RATING_LABELS).src/styles/modal.css (1)
4-6: 사용되지 않는 CSS 클래스
body.modal-open클래스가 정의되어 있지만,Modal.ts에서는document.body.style.overflow = "hidden"을 인라인으로 설정하고 있습니다 (Context snippet 2, line 103).두 가지 접근 방식 중 하나를 선택해야 합니다:
- CSS 클래스를 사용하려면 Modal.ts에서
classList.add/remove("modal-open")으로 변경- 인라인 스타일을 유지하려면 이 CSS 규칙 제거
일반적으로 CSS 클래스 방식이 관심사 분리 측면에서 더 낫습니다. 어떤 접근 방식이 더 적합할지 고려해 보세요.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/styles/modal.css` around lines 4 - 6, The CSS rule body.modal-open is unused because Modal.ts sets document.body.style.overflow inline; switch to the CSS-class approach: in Modal.ts replace any direct document.body.style.overflow = "hidden" / "" usages with document.body.classList.add("modal-open") when opening and document.body.classList.remove("modal-open") when closing/unmounting (ensure cleanup on unmount/escape/close paths), and remove any remaining inline overflow manipulations; keep the body.modal-open rule in src/styles/modal.css as the single source of truth.src/components/Modal.ts (1)
59-63: ESC 키 리스너 최적화 고려현재
keydown리스너가document에 항상 등록되어 있어서, 모달이 닫혀있을 때도 모든 키 입력에 반응합니다.모달이 활성화되어 있을 때만 ESC 키에 반응하도록 하려면 어떤 방법이 있을까요? 힌트:
- 리스너 내부에서 모달 활성 상태를 체크하는 방법
- 모달 열릴 때 리스너를 추가하고 닫힐 때 제거하는 방법
각 접근 방식의 장단점을 고려해 보세요.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/components/Modal.ts` around lines 59 - 63, 현재 document에 항상 등록된 keydown 핸들러는 모달이 닫혀있을 때도 반응하므로 성능/조작 충돌 우려가 있습니다; 해결 방법으로 두 가지 중 택하거나 둘을 병행하세요: (A) 핸들러 내부에서 모달 활성 상태를 확인하도록 수정하여 document.addEventListener로 남기되 이벤트 콜백에서 this.isOpen (또는 Modal 클래스의 상태 플래그)를 검사하고 Escape 처리 시 this.close() 호출; (B, 권장) Modal의 열기/닫기 로직에 맞춰 이벤트를 등록/해제하도록 변경하여 open()/show() 내에서 document.addEventListener("keydown", handler) 추가하고 close()/hide()/destroy() 내에서 document.removeEventListener("keydown", handler)로 제거하여 불필요한 리스너 실행과 메모리 누수 방지(참조되는 핸들러는 Modal 인스턴스 메서드로 고정).
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@index.html`:
- Line 181: index.html에 삽입된 <script type="module" src="./src/main.ts"></script>
태그가 현재 </html> 바깥에 있어 문서 구조가 깨져 있으니 해당 <script> 태그(./src/main.ts 참조)를 문서의 본문 안,
즉 닫는 </body> 태그 바로 직전에 옮겨 넣어 정상적인 HTML 구조가 되도록 수정하세요.
- Around line 98-100: The close button lacks an accessible name; update the
element with id "closeModal" (class "close-modal") to provide one—either add an
aria-label (e.g., aria-label="Close modal") on the <button id="closeModal"> or
add a meaningful alt attribute on the <img> (e.g., alt="Close") so screen
readers can identify the button; prefer adding aria-label to the button for
clear semantics and keep the image alt empty (alt="") if it is purely
decorative.
In `@src/api/fetchMovies.ts`:
- Around line 44-51: The thrown error message in fetchPopularMovies doesn't
match the test's expectation; update the call inside fetchPopularMovies to pass
the exact message "영화를 불러오는 데 실패했습니다." to requestMovieResponse (replace the
current "영화 목록을 불러오는 중 에러가 발생했습니다.") so the implementation aligns with
__tests__/movie.test.ts; locate fetchPopularMovies and the requestMovieResponse
invocation to make the change.
- Line 24: Remove the debug console.log statements left in production: delete
the console.log(data) call found in src/api/fetchMovies.ts (the one logging the
fetched "data" variable) and also remove the duplicate debug console.log at the
other occurrence around line 75; ensure no other console.log debug prints remain
in the fetchMovies-related functions before committing.
- Line 48: 템플릿 문자열에서 TMDB 쿼리 파라미터 형식이 잘못되어 있으므로 fetchMovies 함수(또는 해당 URL 생성
로직)에서 URL 템플릿 `${apiUrl}/movie/popular?language=Ko®ion=ko-KR&page=${page}`를
수정하세요: language 파라미터를 소문자 로케일 형식인 `language=ko-KR`으로 바꾸고 region은 국가 코드만 사용하도록
`region=KR`으로 변경해 일관되게 적용하십시오.
In `@src/components/MovieItem.ts`:
- Around line 30-35: In handleClick(movieItem: HTMLElement) replace the thrown
Error when movieId is missing with a safe early return: detect missing movieId
from movieItem.dataset.movieId, log a warning (e.g., console.warn or the app
logger) with context (movieItem dataset) and return without throwing so the UI
flow isn't interrupted; ensure subsequent code that expects movieId (inside
handleClick) is guarded by this early-return check.
In `@src/components/SearchForm.ts`:
- Line 97: Rename the misspelled method lodeList to loadList and update all
references/call sites (e.g., this.lodeList(...), event handlers, bindings, and
unit tests) to use loadList; ensure the method signature and any visibility
modifiers remain the same and update any overrides or interface implementations
that referenced lodeList so they match the new loadList name.
In `@src/main.ts`:
- Around line 33-35: 현재 catch 블록에서 throw new Error("dd")라는 무의미한 placeholder를 던지고
있는데, 해당 catch(예: main.ts의 해당 try/catch 내 catch 블록 및 MovieItem 콜백에서의 예외 처리)를 의미
있는 메시지로 교체하고 원본 오류 정보를 포함해 재던지거나 그대로 재호출하도록 수정하세요; 예를 들어 "영화 상세 정보를 불러오는 데
실패했습니다: <원본에러메시지>" 형태로 메시지를 구성하거나 원본 error를 그대로 throw/rethrow 하여 상위에서 정상적으로 처리될
수 있게 하십시오. Ensure upstream callers (MovieItem 콜백을 호출하는 곳) have try/catch or
promise rejection handling to avoid unhandled rejections.
In `@src/render/createNotFoundElement.ts`:
- Around line 5-13: The image element notSearchFoundImg lacks an alt attribute
which hurts accessibility; update the creation of notSearchFoundImg (where
searchNotFoundIcon is assigned) to include an appropriate alt—since
notSearchFoundText already conveys "검색 결과가 없습니다.", mark the image as decorative
by setting notSearchFoundImg.alt = "" and
notSearchFoundImg.setAttribute("aria-hidden", "true"); if you decide the image
should convey meaning, instead set a descriptive alt like "검색 결과 없음" and omit
aria-hidden.
In `@src/render/movieList.ts`:
- Around line 27-41: The IntersectionObserver is created each render and never
cleaned up; modify the render logic so you only create and observe the sentinel
when onLoadNextPage is defined and ensure any previous observer is disconnected
before creating a new one — e.g., keep a persistent observer reference
(module-level or closure) and call observer.disconnect() (or
observer.unobserve(sentinal)) before assigning a new IntersectionObserver in the
render function that builds thumbnailList and uses createMovieThumbnail; also
ensure you stop observing/unobserve and remove the sentinel when loading is
finished to avoid leaks.
In `@src/render/skeleton.ts`:
- Around line 4-10: The skeleton nodes are created as a div and appended into a
ul, breaking markup semantics; change the creation to create li elements
instead: replace the div created into an li (e.g., where const skeleton =
document.createElement("div") and skeleton.className = "movie-skeleton") so that
the variable (skeleton) is an li element with the same class, then clone/append
that li inside the loop (the fragment append steps using
skeleton.cloneNode(true)) so the injected nodes match the ul > li structure and
preserve accessibility and styling.
In `@src/render/thumbnail.ts`:
- Around line 25-27: The image element created as starImg is decorative and
currently lacks an alt attribute; update the starImg element (variable name:
starImg) to explicitly mark it decorative by adding alt="" and
aria-hidden="true" (or otherwise ensure it is ignored by assistive technologies)
so screen readers won't announce it.
In `@src/styles/main.css`:
- Around line 104-126: Rename the CSS class selector top-header-conatiner to
top-header-container and update all references (HTML, JS, templates) that use
the misspelled class so they match the corrected class name; specifically change
the selector in the stylesheet and search for occurrences of
"top-header-conatiner" to replace them with "top-header-container" to keep
markup and styles consistent.
- Around line 25-26: 빈 CSS 규칙 선택자 main { } 블록이 남아 있으니 의도된 플레이스홀더면 주석으로 의도(예: /*
intentional placeholder for layout styles */)를 추가하거나, 실제 스타일을 넣거나, 블록 자체를 제거하세요;
대상 식별자는 CSS 선택자 main 및 해당 빈 블록이며 변경 시 빈 규칙을 제거하거나 주석/스타일을 반드시 추가하도록 수정하세요.
In `@src/styles/modal.css`:
- Around line 199-204: Remove the unnecessary blank line inside the .modal rule
so declarations are adjacent (i.e., eliminate the empty line between the width
declaration and the max-height declaration in the .modal CSS block) to satisfy
the declaration-empty-line-before Stylelint rule.
- Around line 128-138: The CSS uses absolute paths that break in GitHub Pages;
move star images into src/assets (alongside movie_fallback_image.svg), import
them in the component that renders .rating-star (e.g., the Modal/Rating
component) and replace the hardcoded background URLs by applying the imported
URLs (filled and empty) via inline style, CSS variables, or a
CSS-in-JS/className toggle so .rating-star and .rating-star.filled use the
imported asset URLs at runtime.
In `@src/vite-end.d.ts`:
- Around line 1-4: The ImportMetaEnv type declares VITE_TMDB_IMG_URL as required
but CI/workflow doesn't inject it; update the type to make VITE_TMDB_IMG_URL
optional (change VITE_TMDB_IMG_URL to an optional field on ImportMetaEnv) and
add a runtime validation in your app startup (e.g., a validateEnv or initConfig
routine that checks import.meta.env.VITE_TMDB_IMG_URL and logs/falls back to a
default or disables image features) so the app won’t construct "undefined..."
poster URLs; reference ImportMetaEnv and VITE_TMDB_IMG_URL when making these
changes.
In `@STEP1-README.md`:
- Around line 10-11: Documentation and tests disagree on page size: update the
README lines that currently state "검색어에 해당하는 영화를 10개 가져온다." and "더보기를 누르면 10개를
추가로 가져온다." and the E2E scenario lines referenced (47-48) so they all use the
same page size (choose either 10 or 20 consistently); make the change in the
STEP1-README.md text and any E2E scenario descriptions so wording and numbers
match the implemented behavior (and ensure the wording around "더보기" matches the
chosen increment).
In `@STEP2-README.md`:
- Line 16: 문서 STEP2-README.md에서 문장 "별점은 5개로 구성되어 있으며 한 개당 2점이댜."의 오타를 수정하세요;
텍스트에서 "이댜"를 "이다"로 바꿔 최종 문장이 "별점은 5개로 구성되어 있으며 한 개당 2점이다."가 되도록 변경하십시오.
In `@types/movie.ts`:
- Around line 1-16: The Movie interface fields don't match TMDB v3: rename
genre_idx to genre_ids (number[]), and update poster_path and backdrop_path to
be nullable (string | null) so they accept null from the API; update the Movie
interface declaration accordingly (keep other fields as-is) to prevent runtime
issues when API returns null values.
---
Nitpick comments:
In `@__tests__/movie.test.ts`:
- Around line 29-32: Add test teardown to restore the mocked global fetch: after
using vi.stubGlobal("fetch", vi.fn()) in beforeEach, add an afterEach that calls
vi.restoreAllMocks() or vi.unstubAllGlobals() to clean up the stub between tests
so mocks set in beforeEach are removed; reference the existing beforeEach and
add afterEach accordingly.
- Around line 7-26: These tests call external APIs directly via
fetchPopularMovies and fetchSearchedMovies—make them use the same fetch mocking
approach as the error tests to avoid flaky, slow, or rate-limited CI runs:
locate the tests using fetchPopularMovies and fetchSearchedMovies and install a
mock implementation for global.fetch (or jest.spyOn(window, "fetch")) that
returns a successful Response-like object with a predictable JSON payload (e.g.,
20 items) and proper status, reuse the mocking utilities or fixtures used in the
error test to keep behavior consistent, and ensure you restore/clear the mock
between tests so other specs are unaffected.
In `@cypress/e2e/search.cy.ts`:
- Around line 78-88: The test "3. 마지막 페이지까지 불러오면 더 이상 요청이 발생하지 않는다." should
explicitly assert that no extra API calls occur after the last page: keep the
existing flow (call searchMovie("인사이드"), wait for "@searchP1" and "@searchP2",
scrollTo bottom) but add an assertion that the alias "@searchP2" was called
exactly once (or that the request count did not increase) and/or add a short
cy.wait then assert no additional calls; reference the test case name, the
searchMovie helper, the "@searchP2" alias and the ".thumbnail-list li" selector
to locate where to add the call-count assertion or extra wait+no-call check.
- Line 55: Replace the hardcoded URL in the test where
cy.visit("http://localhost:5173/") is called in search.cy.ts with a relative
path using the configured Cypress baseUrl (e.g., cy.visit("/") ) so the test
honors environment-specific baseUrl from cypress.config.ts; update the cy.visit
call accordingly so it relies on Cypress' baseUrl setting rather than a fixed
host.
In `@src/components/Modal.ts`:
- Around line 59-63: 현재 document에 항상 등록된 keydown 핸들러는 모달이 닫혀있을 때도 반응하므로 성능/조작 충돌
우려가 있습니다; 해결 방법으로 두 가지 중 택하거나 둘을 병행하세요: (A) 핸들러 내부에서 모달 활성 상태를 확인하도록 수정하여
document.addEventListener로 남기되 이벤트 콜백에서 this.isOpen (또는 Modal 클래스의 상태 플래그)를 검사하고
Escape 처리 시 this.close() 호출; (B, 권장) Modal의 열기/닫기 로직에 맞춰 이벤트를 등록/해제하도록 변경하여
open()/show() 내에서 document.addEventListener("keydown", handler) 추가하고
close()/hide()/destroy() 내에서 document.removeEventListener("keydown", handler)로
제거하여 불필요한 리스너 실행과 메모리 누수 방지(참조되는 핸들러는 Modal 인스턴스 메서드로 고정).
In `@src/components/MovieItem.ts`:
- Around line 17-28: The bindEvent() method currently adds a click listener to
this.#thumbnailList every time it's called, causing duplicate handlers; modify
bindEvent() to guard so the listener is only attached once (e.g., track a
boolean like this._eventsBound or store the bound handler on an instance
property), check that flag or existing handler before calling addEventListener,
and ensure you still reference the same handler when removing listeners if
needed; update references to bindEvent(), this.#thumbnailList, and handleClick
to use the single stored listener/flag approach so handleClick is invoked only
once per click.
In `@src/components/review.ts`:
- Around line 70-73: `#updateText에서` this.#labelEl.firstChild!의 non-null assertion
사용은 자식 노드가 없을 경우 런타임 에러를 유발하므로 안전하게 수정해야 합니다; 대신 this.#labelEl의 textContent를 직접
설정하거나(예: this.#labelEl.textContent = RATING_LABELS[rating] + " ") 또는 firstChild가
없을 때 새 텍스트 노드를 생성하도록 방어 코드로 처리하여 RATING_LABELS[rating] 값을 안전하게 배치하고 null 문제를
방지하세요 (참조: `#updateText`, `#labelEl`, `#scoreEl`, RATING_LABELS).
In `@src/components/SearchForm.ts`:
- Around line 77-86: In loadMore, remove the duplicate removeSkeleton() call
inside the try block because the finally block already guarantees cleanup; keep
renderSkeleton() at the start, await this.lodeList(PageStore.page + 1), call
renderMoviesList(movies, { append: true }, loadMore) when movies exist, and rely
on the finally removeSkeleton() to always run so you avoid redundant DOM
manipulation.
In `@src/main.ts`:
- Around line 91-93: In the catch block handling the movie-fetch failure (the
catch that currently does console.log(error) and alert(...)), replace
console.log(error) with console.error(error) so the error is recorded as an
actual error in browser devtools; keep the existing alert unchanged and ensure
the catch continues to reference the same error variable.
In `@src/store.ts`:
- Around line 13-17: The resetToPopularView function is mutating PageStore
properties directly instead of using the existing setPopularMode() method;
update resetToPopularView to call PageStore.setPopularMode() (or delegate to
that method) so all logic for switching to popular mode is centralized in
setPopularMode(), preventing inconsistent behavior if setPopularMode() changes
in the future.
In `@src/styles/modal.css`:
- Around line 4-6: The CSS rule body.modal-open is unused because Modal.ts sets
document.body.style.overflow inline; switch to the CSS-class approach: in
Modal.ts replace any direct document.body.style.overflow = "hidden" / "" usages
with document.body.classList.add("modal-open") when opening and
document.body.classList.remove("modal-open") when closing/unmounting (ensure
cleanup on unmount/escape/close paths), and remove any remaining inline overflow
manipulations; keep the body.modal-open rule in src/styles/modal.css as the
single source of truth.
In `@src/utils/getYear.ts`:
- Around line 1-4: The getYear function currently assumes date is "YYYY-MM-DD";
add explicit input validation in getYear (and use the parseDate variable) so
non-conforming values (empty string, malformed strings) return a defined
fallback; implement a simple validation (e.g. regex for four-digit year with
optional -MM-DD) and return the decided fallback (e.g. '' or 'Unknown') when
validation fails, otherwise extract and return the year as before.
🪄 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: e8fdb412-3e95-4dc9-a71f-edb50bfd77f4
⛔ Files ignored due to path filters (3)
src/assets/movie_fallback_image.svgis excluded by!**/*.svgsrc/assets/searchNotFoundIcon.pngis excluded by!**/*.pngsrc/assets/serchIcon.pngis excluded by!**/*.png
📒 Files selected for processing (39)
.github/workflows/main.yml.gitignore.gitmessage.txtREADME.mdSTEP1-README.mdSTEP2-README.md__tests__/movie.test.tscypress/e2e/main.cy.tscypress/e2e/search.cy.tscypress/e2e/spec.cy.tsindex.htmlpackage.jsonsrc/api/fetchMovies.tssrc/components/Logo.tssrc/components/Modal.tssrc/components/MovieItem.tssrc/components/SearchForm.tssrc/components/review.tssrc/main.tssrc/render/banner.tssrc/render/createNotFoundElement.tssrc/render/movieList.tssrc/render/skeleton.tssrc/render/thumbnail.tssrc/storage/RatingStorage.tssrc/store.tssrc/styles/colors.csssrc/styles/index.csssrc/styles/main.csssrc/styles/modal.csssrc/styles/reset.csssrc/styles/tab.csssrc/styles/thumbnail.csssrc/utils/getYear.tssrc/utils/mapToThumbnailInfo.tssrc/vite-end.d.tstemplates/styles/main.csstemplates/styles/modal.csstypes/movie.ts
💤 Files with no reviewable changes (4)
- README.md
- cypress/e2e/spec.cy.ts
- templates/styles/main.css
- templates/styles/modal.css
| <button class="close-modal" id="closeModal"> | ||
| <img src="./templates/images/modal_button_close.png" /> | ||
| </button> |
There was a problem hiding this comment.
모달 닫기 버튼에 접근 가능한 이름이 필요합니다.
아이콘 이미지 버튼인데 버튼 자체의 aria-label 또는 이미지의 적절한 alt가 없어 스크린리더에서 용도를 파악하기 어렵습니다.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@index.html` around lines 98 - 100, The close button lacks an accessible name;
update the element with id "closeModal" (class "close-modal") to provide
one—either add an aria-label (e.g., aria-label="Close modal") on the <button
id="closeModal"> or add a meaningful alt attribute on the <img> (e.g.,
alt="Close") so screen readers can identify the button; prefer adding aria-label
to the button for clear semantics and keep the image alt empty (alt="") if it is
purely decorative.
| </body> | ||
| </html> | ||
|
|
||
| <script type="module" src="./src/main.ts"></script> |
There was a problem hiding this comment.
스크립트 태그 위치를 문서 내부로 옮겨주세요.
Line 181의 <script type="module" ...>가 </html> 바깥에 있어 문서 구조가 비정상입니다. </body> 직전으로 이동하는 것이 안전합니다.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@index.html` at line 181, index.html에 삽입된 <script type="module"
src="./src/main.ts"></script> 태그가 현재 </html> 바깥에 있어 문서 구조가 깨져 있으니 해당 <script>
태그(./src/main.ts 참조)를 문서의 본문 안, 즉 닫는 </body> 태그 바로 직전에 옮겨 넣어 정상적인 HTML 구조가 되도록
수정하세요.
|
|
||
| try { | ||
| const data = await response.json(); | ||
| console.log(data); |
There was a problem hiding this comment.
🛠️ Refactor suggestion | 🟠 Major
디버그용 console.log 제거 필요
프로덕션 코드에 디버그용 console.log가 남아있습니다. 배포 전에 제거해 주세요.
Line 75에도 동일한 이슈가 있습니다.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/api/fetchMovies.ts` at line 24, Remove the debug console.log statements
left in production: delete the console.log(data) call found in
src/api/fetchMovies.ts (the one logging the fetched "data" variable) and also
remove the duplicate debug console.log at the other occurrence around line 75;
ensure no other console.log debug prints remain in the fetchMovies-related
functions before committing.
| export const fetchPopularMovies = async ( | ||
| page: number, | ||
| ): Promise<MovieResponse> => { | ||
| return requestMovieResponse( | ||
| `${apiUrl}/movie/popular?language=Ko®ion=ko-KR&page=${page}`, | ||
| "영화 목록을 불러오는 중 에러가 발생했습니다.", | ||
| ); | ||
| }; |
There was a problem hiding this comment.
테스트와 에러 메시지 불일치
__tests__/movie.test.ts(Context snippet 1, line 43)에서 "영화를 불러오는 데 실패했습니다."를 기대하지만, 실제 구현은 "영화 목록을 불러오는 중 에러가 발생했습니다."를 throw합니다. 테스트가 실패할 것입니다.
구현과 테스트 중 어느 쪽을 수정할지 결정해 보세요. 어떤 메시지가 사용자에게 더 명확할까요?
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/api/fetchMovies.ts` around lines 44 - 51, The thrown error message in
fetchPopularMovies doesn't match the test's expectation; update the call inside
fetchPopularMovies to pass the exact message "영화를 불러오는 데 실패했습니다." to
requestMovieResponse (replace the current "영화 목록을 불러오는 중 에러가 발생했습니다.") so the
implementation aligns with __tests__/movie.test.ts; locate fetchPopularMovies
and the requestMovieResponse invocation to make the change.
src/api/fetchMovies.ts
Outdated
| page: number, | ||
| ): Promise<MovieResponse> => { | ||
| return requestMovieResponse( | ||
| `${apiUrl}/movie/popular?language=Ko®ion=ko-KR&page=${page}`, |
There was a problem hiding this comment.
🧩 Analysis chain
🌐 Web query:
TMDB API language and region parameter format
💡 Result:
TMDB API language parameter uses ISO 639-1 codes, typically combined with ISO 3166-1 country codes in the format language-COUNTRY (e.g., en-US, pt-BR, de-DE). The region parameter uses ISO 3166-1 country codes (e.g., DE, US), uppercase, to filter by release dates or availability in specific countries. Examples: - Language: ?language=en-US for English (US variant). - Region: ?region=DE for Germany. These parameters are query strings in GET requests, supported across most endpoints like search, discover, movie/TV details. Default language is en-US if unspecified. For images, language filters results but regional variants (-COUNTRY) are not fully supported yet.
Citations:
- 1: https://developer.themoviedb.org/docs/languages
- 2: https://developer.themoviedb.org/docs/region-support
- 3: https://developer.themoviedb.org/reference/discover-movie
- 4: https://developer.themoviedb.org/docs/image-languages
- 5: https://ileolami.mintlify.app/parameters/languages
- 6: https://ileolami.mintlify.app/parameters/region-support
- 7: https://tmdbapis.kometa.wiki/en/latest/rawapi.html
🏁 Script executed:
# Check the fetchMovies.ts file to see lines around 48, 58, and 65
sed -n '45,70p' src/api/fetchMovies.tsRepository: woowacourse/javascript-movie-review
Length of output: 820
API 파라미터 형식 수정 필요
48번 라인의 파라미터가 TMDB API 명세와 맞지 않습니다:
language=Ko→language=ko-KR형식으로 수정 (다른 라인들에서 올바르게 사용 중)region=ko-KR→region=KR형식으로 수정 (region은 국가 코드만 사용)
TMDB 공식 문서(https://developer.themoviedb.org/docs/languages, https://developer.themoviedb.org/docs/region-support)에서 정확한 파라미터 형식을 확인하고 적용해 주세요.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/api/fetchMovies.ts` at line 48, 템플릿 문자열에서 TMDB 쿼리 파라미터 형식이 잘못되어 있으므로
fetchMovies 함수(또는 해당 URL 생성 로직)에서 URL 템플릿
`${apiUrl}/movie/popular?language=Ko®ion=ko-KR&page=${page}`를 수정하세요: language
파라미터를 소문자 로케일 형식인 `language=ko-KR`으로 바꾸고 region은 국가 코드만 사용하도록 `region=KR`으로 변경해
일관되게 적용하십시오.
| .modal { | ||
| width: 100%; | ||
|
|
||
| max-height: 80vh; | ||
| overflow-y: auto; | ||
| } |
There was a problem hiding this comment.
Stylelint 오류 수정
Line 202에 불필요한 빈 줄이 있습니다. declaration-empty-line-before 규칙 위반입니다.
.modal {
width: 100%;
-
max-height: 80vh;📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| .modal { | |
| width: 100%; | |
| max-height: 80vh; | |
| overflow-y: auto; | |
| } | |
| .modal { | |
| width: 100%; | |
| max-height: 80vh; | |
| overflow-y: auto; | |
| } |
🧰 Tools
🪛 Stylelint (17.6.0)
[error] 202-202: Unexpected empty line before declaration (declaration-empty-line-before)
(declaration-empty-line-before)
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/styles/modal.css` around lines 199 - 204, Remove the unnecessary blank
line inside the .modal rule so declarations are adjacent (i.e., eliminate the
empty line between the width declaration and the max-height declaration in the
.modal CSS block) to satisfy the declaration-empty-line-before Stylelint rule.
| interface ImportMetaEnv { | ||
| readonly VITE_API_BASE_URL: string; | ||
| readonly VITE_ACCESS_TOKEN: string; | ||
| readonly VITE_TMDB_IMG_URL: string; |
There was a problem hiding this comment.
이미지 URL 환경변수 주입 누락 가능성이 큽니다.
타입에서는 VITE_TMDB_IMG_URL을 필수로 선언했지만, 제공된 CI 워크플로우에는 해당 값 주입이 없습니다. 이 상태면 포스터 URL이 undefined... 형태로 만들어져 배너/썸네일 이미지가 깨질 수 있습니다. 배포 파이프라인 주입 또는 앱 시작 시 런타임 검증을 같이 확인해 주세요.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/vite-end.d.ts` around lines 1 - 4, The ImportMetaEnv type declares
VITE_TMDB_IMG_URL as required but CI/workflow doesn't inject it; update the type
to make VITE_TMDB_IMG_URL optional (change VITE_TMDB_IMG_URL to an optional
field on ImportMetaEnv) and add a runtime validation in your app startup (e.g.,
a validateEnv or initConfig routine that checks
import.meta.env.VITE_TMDB_IMG_URL and logs/falls back to a default or disables
image features) so the app won’t construct "undefined..." poster URLs; reference
ImportMetaEnv and VITE_TMDB_IMG_URL when making these changes.
| - 검색어에 해당하는 영화를 10개 가져온다. | ||
| - 더보기를 누르면 10개를 추가로 가져온다. |
There was a problem hiding this comment.
검색 결과 개수 기준이 문서 내에서 충돌합니다.
요구사항은 10개 단위인데, E2E 시나리오는 20개 단위로 적혀 있어 구현/테스트 기준이 갈릴 수 있습니다. 한 기준으로 통일해 주세요.
Also applies to: 47-48
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@STEP1-README.md` around lines 10 - 11, Documentation and tests disagree on
page size: update the README lines that currently state "검색어에 해당하는 영화를 10개
가져온다." and "더보기를 누르면 10개를 추가로 가져온다." and the E2E scenario lines referenced
(47-48) so they all use the same page size (choose either 10 or 20
consistently); make the change in the STEP1-README.md text and any E2E scenario
descriptions so wording and numbers match the implemented behavior (and ensure
the wording around "더보기" matches the chosen increment).
STEP2-README.md
Outdated
| ## 2. 별점 매기기 | ||
|
|
||
| - 영화 상세정보 모달에서 별을 눌러 별점을 설정할 수 있다. | ||
| - 별점은 5개로 구성되어 있으며 한 개당 2점이댜. |
There was a problem hiding this comment.
오타 수정 필요
"이댜" → "이다"로 수정해 주세요.
- - 별점은 5개로 구성되어 있으며 한 개당 2점이댜.
+ - 별점은 5개로 구성되어 있으며 한 개당 2점이다.📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| - 별점은 5개로 구성되어 있으며 한 개당 2점이댜. | |
| - 별점은 5개로 구성되어 있으며 한 개당 2점이다. |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@STEP2-README.md` at line 16, 문서 STEP2-README.md에서 문장 "별점은 5개로 구성되어 있으며 한 개당
2점이댜."의 오타를 수정하세요; 텍스트에서 "이댜"를 "이다"로 바꿔 최종 문장이 "별점은 5개로 구성되어 있으며 한 개당 2점이다."가
되도록 변경하십시오.
| export interface Movie { | ||
| adult: boolean; | ||
| backdrop_path: string; | ||
| genre_idx: number[]; | ||
| id: number; | ||
| original_language: string; | ||
| original_title: string; | ||
| overview: string; | ||
| popularity: number; | ||
| poster_path: string; | ||
| release_date: string; | ||
| title: string; | ||
| video: boolean; | ||
| vote_average: number; | ||
| vote_count: number; | ||
| } |
There was a problem hiding this comment.
🧩 Analysis chain
🌐 Web query:
In TMDB v3 movie list responses (e.g., /movie/popular), what are the exact field names and nullability for genre ids, poster_path, and backdrop_path?
💡 Result:
In TMDB v3 movie list responses (e.g., /movie/popular), the fields are: - genre_ids: array of integers (genre ID numbers), always present (may be empty array), high confidence from official docs examples showing [28,12,878]. - poster_path: string (relative path to poster image, e.g., "/ngl2FKBlU4fhbdsrtdom9LVLBXw.jpg"), nullable (can be null if no poster), high confidence from examples and third-party schemas. - backdrop_path: string (relative path to backdrop image, e.g., "/gMJngTNfaqCSCqGD4y8lVMZXKDn.jpg"), nullable (can be null if no backdrop), high confidence from examples and third-party schemas. These fields appear consistently in the "results" array of paginated list responses across /movie/popular, /discover/movie, /search/movie, and similar endpoints, as shown in official OpenAPI examples and documentation snippets.
Citations:
- 1: https://developer.themoviedb.org/reference/movie-details
- 2: https://www.mintlify.com/otniel0820/connect-world-v1/api/tmdb
- 3: https://notes.nicolasdeville.com/python/tmdb-api/
- 4: https://developer.themoviedb.org/reference/movie-popular-list
- 5: https://developer.themoviedb.org/docs/search-and-query-for-details
- 6: https://developer.themoviedb.org/reference/discover-movie
- 7: https://www.v3solutions.in
TMDB API 응답 스키마와 타입 선언이 일치하지 않습니다.
TMDB v3 API 응답에서:
- 필드명은
genre_ids(정수 배열)인데, Line 4에서genre_idx로 선언되어 있음 poster_path와backdrop_path는 null 값을 가질 수 있으나, 현재 non-nullable string으로 선언됨
타입 인터페이스를 실제 TMDB API 응답 구조에 맞게 수정해야 합니다. 특히 nullable인 필드들을 정확히 반영하지 않으면 API 응답에서 null을 받을 때 조용한 런타임 오류가 발생할 수 있습니다.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@types/movie.ts` around lines 1 - 16, The Movie interface fields don't match
TMDB v3: rename genre_idx to genre_ids (number[]), and update poster_path and
backdrop_path to be nullable (string | null) so they accept null from the API;
update the Movie interface declaration accordingly (keep other fields as-is) to
prevent runtime issues when API returns null values.
jw-r
left a comment
jw-r
left a comment
There was a problem hiding this comment.
포도 안녕하세요!
미션 진행하시느라 고생하셨습니다 👍
1단계 대비 구조 개선이 눈에 띕니다! components, render, storage, api, utils로 역할을 분리하고, RatingStorage 인터페이스로 저장소를 추상화한 설계가 인상적입니다. 무한 스크롤을 IntersectionObserver로 전환한 것도 좋은 판단이고, 모달의 openEmpty() → fill() 패턴으로 이전 데이터 노출 문제를 해결한 점도 잘 고민한 흔적이 보입니다.
다만, 비동기 에러 핸들링에서 unhandled promise rejection이 발생할 수 있는 구조가 있어서 수정이 필요합니다.
아래 내용 확인하시고 수정 부탁드려요!
🎯 수정 필요 사항
1. [필수] 영화 상세 조회 시 에러 처리
파일: main.ts, MovieItem.ts
현재 movieItem 콜백에서 에러를 throw하고 있지만, MovieItem.handleClick()에서 이 콜백의 반환값(Promise)을 await하지 않아 unhandled promise rejection이 발생합니다. 또한 에러 시 모달이 빈 상태로 열려있게 됩니다.
// ❌ 현재 코드 - throw해도 아무도 catch하지 않음
const movieItem = new MovieItem(async (movieId: string) => {
try {
modal.openEmpty();
const movieDetail = await fetchMoviesDetail(Number(movieId));
// ...
} catch (error) {
throw new Error("영화 상세 정보를 불러오는 중 오류가 발생했습니다.");
}
});2. [필수] loadMore 함수에 catch 블록 추가
파일: main.ts, SearchForm.ts
loadMore 함수가 IntersectionObserver 콜백에서 호출되므로, 에러가 전파되지 않고 삼켜집니다. 또한 마지막 페이지인지 체크하는 가드가 없어 불필요한 API 호출이 발생할 수 있습니다.
// ❌ 현재 코드 - catch 블록 없음
const loadMore = async () => {
try {
renderSkeleton();
const movies = await onLoadNextPage(PageStore.page + 1);
removeSkeleton();
if (!movies.length) return;
renderMoviesList(movies, { append: true }, loadMore);
} finally {
removeSkeleton();
}
};➡️ SearchForm.ts의 loadMore도 동일하게 수정해주세요.
3. [필수] PageStore 상태 변경 일관성
파일: main.ts, store.ts
PageStore에 setPopularMode() 메서드가 있는데, resetToPopularView()에서는 직접 프로퍼티를 수정하고 있습니다.
// ❌ 현재 코드 - 직접 수정
PageStore.mode = "popular";
PageStore.query = "";
PageStore.page = 1;상태 변경 방식을 통일하면 "상태가 어디서 바뀌었지?" 추적이 쉬워집니다.
📝 Q&A
Q1. RatingStorage 분리 수준이 충분한가요?
별점 저장 로직에서 RatingStorage 인터페이스를 통해 localStorage 구현을 분리했는데, UI가 여전히 저장소 호출 시점을 알고 있는 구조입니다. 현재 정도의 분리 수준이면 충분한지 궁금합니다.
현재 수준이면 충분합니다. ✅
현재 구조를 정리하면:
main.ts: new Review(new LocalRatingStorage()) ← 주입
Review: this.#storage.save() / this.#storage.get() ← 인터페이스로 호출
UI(Review)가 저장 시점(클릭 시 저장, 모달 열 때 로드)을 아는 것은 자연스럽습니다. 중요한 건 **어떻게 저장하는지(how)**를 모르는 것이고, 현재 구조에서는 그게 잘 분리되어 있습니다.
서버 API로 교체할 때의 코드 변경:
// 현재
const review = new Review(new LocalRatingStorage());
// 서버 API로 교체 시 - Review 코드 변경 없음!
const review = new Review(new ServerRatingStorage());한 단계 더 분리가 필요한 경우:
- 저장이 비동기(서버 API)가 되어
save()가Promise를 반환해야 할 때 - 이 경우 인터페이스를
Promise<void>로 변경하고,Review에서await처리가 필요합니다
// 현재 인터페이스
export interface RatingStorage {
get(movieId: number): number;
save(movieId: number, rating: number): void;
}
// 서버 API 대응 시
export interface RatingStorage {
get(movieId: number): Promise<number>;
save(movieId: number, rating: number): Promise<void>;
}이 정도만 고려하면 됩니다. 지금 단계에서 과도하게 추상화하면 오히려 복잡해집니다.
Q2. IntersectionObserver가 실제 서비스에서 적절한가요?
현재 무한 스크롤을 IntersectionObserver로 구현했는데, 실제 서비스 환경에서도 이 방식이 적절한지 궁금합니다.
적절합니다. 실제 서비스에서도 가장 권장되는 방식입니다. ✅
| 비교 항목 | scroll 이벤트 + getBoundingClientRect | IntersectionObserver |
|---|---|---|
| 성능 | 스크롤마다 layout 재계산 | 브라우저 최적화, 비동기 |
| 코드 복잡도 | throttle/debounce 필요 | 불필요 |
| 브라우저 지원 | 모든 브라우저 | IE 제외 모든 브라우저 |
추가로 고려할 점:
-
Observer 정리: 현재
renderMoviesList호출마다 새 observer를 생성하지만disconnect()를 하지 않습니다. 모듈 레벨에서 하나의 observer를 재사용하거나, 이전 observer를 정리하는 로직이 있으면 좋습니다. -
마지막 페이지 가드: sentinel이 화면에 보이면 무조건 다음 페이지를 요청합니다.
PageStore.page >= PageStore.totalPages이면 요청을 건너뛰는 가드를 추가해주세요. -
빠른 스크롤 시 중복 요청: 현재
observer.unobserve(entry.target)로 처리하고 있어 기본적으로는 괜찮지만, 네트워크가 느릴 때 이전 요청이 완료되기 전에 다음 sentinel이 관찰될 수 있습니다.isLoading플래그를 추가하면 더 안전합니다.
let isLoading = false;
const loadMore = async () => {
if (isLoading || PageStore.page >= PageStore.totalPages) return;
isLoading = true;
try {
// ...
} finally {
isLoading = false;
removeSkeleton();
}
};Q3. components와 render의 경계가 명확하지 않은데, 조언 부탁드립니다.
현재 파일 구조를 components, render, storage, api 등으로 나누어 보았는데, 역할 분리가 적절한지 궁금합니다.
현재 구조가 상당히 좋습니다. 다만 components와 render의 경계를 더 명확히 할 수 있습니다.
현재 구분 기준:
| 폴더 | 역할 | 예시 |
|---|---|---|
components/ |
상태 + 이벤트를 가진 인터랙티브 요소 | Modal, SearchForm, Review |
render/ |
순수 렌더링 (상태 없음) | thumbnail, skeleton, banner |
이 기준은 합리적입니다! 정리하면:
- components: "행동(behavior)이 있는 것" - 이벤트 리스너, 내부 상태, 생명주기
- render: "그리기만 하는 것" - 데이터를 받아서 DOM을 만들거나 업데이트
개선 포인트:
renderMoviesList는 IntersectionObserver를 생성하고 콜백을 관리하므로 단순 렌더 함수를 넘어섭니다. 이 부분을 components로 옮기거나, observer 관련 로직을 별도로 분리하면 경계가 더 명확해집니다.
render/ → 순수 DOM 생성/조작만 (부수효과 없음)
components/ → 이벤트 + 상태 + 생명주기 관리
하지만 지금 단계에서는 현재 구조를 유지하면서 위 필수 수정사항만 반영하면 충분합니다. 구조 리팩토링은 코드가 더 커졌을 때 진행해도 늦지 않습니다.
자세한 내용은 코멘트를 확인해주세요!
확인하시고 수정 후 다시 리뷰 요청 부탁드립니다 🙏
| genres: [{ name: "애니메이션" }, { name: "가족" }], | ||
| }; | ||
|
|
||
| describe("최초 진입 시나리오 테스트", () => { |
There was a problem hiding this comment.
초기 로딩, 무한스크롤, 모달, 반응형 레이아웃까지 매우 잘 커버하고 있네요👍
| // 마지막 페이지(2페이지)를 불러오도록 버튼 클릭 | ||
| cy.get(".more-button").click(); | ||
| cy.wait("@getPopularP2"); | ||
| describe("6. 반응형 레이아웃", () => { |
| }); | ||
| review.load(Number(movieId)); | ||
| } catch (error) { | ||
| throw new Error("영화 상세 정보를 불러오는 중 오류가 발생했습니다."); |
There was a problem hiding this comment.
throw new Error()로 다시 던지면 이 콜백을 호출하는 MovieItem.handleClick()에서
await 없이 호출하기 때문에 unhandled promise rejection이 발생할 수 있어요
에러를 여기서 처리(alert 등)하거나, MovieItem에서 콜백을 await하고 try-catch로 감싸면 좋습니다
또한 모달이 빈 상태로 열려있게 되므로 에러 시 modal.close()를 호출해도 좋겠네요
| PageStore.mode = "popular"; | ||
| PageStore.query = ""; | ||
| PageStore.page = 1; |
There was a problem hiding this comment.
PageStore에 setPopularMode() 메서드가 있는데 직접 프로퍼티를 수정하고 있네요
| renderMoviesList(movies, { append: false }); | ||
| renderTopRatedMovie(thumbnails[0]); | ||
| } catch (error) { | ||
| console.log(error); |
| document.addEventListener("keydown", (e) => { | ||
| if (e.key === "Escape") { | ||
| this.close(); | ||
| } | ||
| }); |
There was a problem hiding this comment.
ESC 리스너가 document에 상시 등록되어 있어서 모달이 닫힘 상태에서도 불필요하게 동작합니다
모달이 열릴 때 등록하고 닫힐 때 제거하는 방식을 고려해봐도 좋을 것 같네요!
| loadImageWithFallback(image, movie.posterPath, fallbackImg); | ||
| } | ||
|
|
||
| openEmpty() { |
There was a problem hiding this comment.
openEmpty() → fill() 패턴으로 이전 데이터 노출 문제를 해결한 점이 인상적이네요!
| handleClick(movieItem: HTMLElement) { | ||
| const movieId = movieItem.dataset.movieId; | ||
|
|
||
| if (!movieId) { | ||
| throw new Error("movieId를 찾을 수 없습니다."); | ||
| } |
| const observer = new IntersectionObserver((entries) => { | ||
| entries.forEach((entry) => { | ||
| if (entry.isIntersecting && onLoadNextPage) { | ||
| observer.unobserve(entry.target); | ||
| onLoadNextPage(); | ||
| } | ||
| }); | ||
| }); |
There was a problem hiding this comment.
IntersectionObserver 활용이 좋습니다! getBoundingClientRect 대비 성능 장점이 있습니다
다만, 매 호출마다 새 observer를 생성하고 disconnect()를 하지 않아 이전 observer가 메모리에 남습니다
observer.disconnect()를 적절한 시점에 호출하거나, 모듈 레벨에서 하나의 observer를 재사용하는 방식을 고려해보세요.
| @@ -0,0 +1,4 @@ | |||
| export const getYear = (date: string) => { | |||
There was a problem hiding this comment.
date가 빈 문자열이거나 예상과 다른 형식이면 빈 문자열을 반환합니다
방어 코드를 추가하거나, new Date(date).getFullYear() 사용도 고려해보세요
2 participants
🎯 미션 소개
🕵️ 셀프 리뷰(Self-Review)
제출 전 체크 리스트
리뷰 요청 & 논의하고 싶은 내용
1) 이번 단계에서 가장 많이 고민했던 문제와 해결 과정에서 배운 점
무한 스크롤 구현
초기에는 getBoundingClientRect()를 사용하여 스크롤 시 요소의 위치를 직접 계산하는 방식으로 구현했습니다.
하지만 이 방식은 스크롤 이벤트마다 layout 계산을 유발할 수 있고, 연산이 반복되면서 성능 저하로 이어질 수 있다는 점을 인지하게 되었습니다.
이러한 문제를 개선하기 위해 IntersectionObserver를 도입했습니다.
스크롤 이벤트마다 요소의 위치를 직접 계산하지 않고, 필요한 시점에 다음 데이터를 불러오는 방식으로 구현할 수 있었습니다.
모달 렌더링 안정성 개선
모달을 표시하는 과정에서, 이전에 선택한 영화 정보가 잠시 남아 보이거나
이미지가 올바르게 표시되지 않는 문제가 발생했습니다.
이를 해결하기 위해 모달을 열 때 바로 상세 데이터를 렌더링하지 않고,
먼저 빈 상태의 모달을 표시한 뒤 비동기로 데이터를 받아와 채우는 방식으로 개선했습니다.
또한 이미지가 완전히 로드되기 전까지는 보이지 않도록 처리하고,
로드가 완료된 이후에만 표시되도록 하여 이전 이미지가 남아 보이는 문제를 줄였습니다.
UI는 데이터 변경만으로 안정적으로 갱신되는 것이 아니라 렌더링 타이밍과 리소스 로딩 시점까지 함께 고려해야 한다는 점을 깨달았습니다.
데이터 저장 방식과 확장성에 대한 고민
별점 UI가 localStorage에 직접 접근하지 않도록, RatingStorage 인터페이스와 LocalRatingStorage 구현체를 분리했습니다.
이를 통해 현재는 로컬 스토리지를 사용하지만, 추후 저장 방식을 변경하더라도 UI가 localStorage 구현에 직접 의존하지 않도록 구성했습니다.
다만 별점 컴포넌트가 여전히 저장소의 호출 시점을 알고 있기 때문에, 저장 로직과 UI가 완전히 분리된 구조라고 보기는 어렵다고 느꼈습니다.
2) 이번 리뷰를 통해 논의하고 싶은 부분
별점 저장 로직에서 RatingStorage 인터페이스를 통해 localStorage 구현을 분리했는데,
UI가 여전히 저장소 호출 시점을 알고 있는 구조입니다.
현재 정도의 분리 수준이면 충분한지, 아니면 데이터 흐름을 한 단계 더 분리하는 것이 좋은지 궁금합니다.
현재 무한 스크롤을 IntersectionObserver로 구현했는데,
실제 서비스 환경에서도 이 방식이 적절한지, 추가로 고려해야 할 성능 최적화 포인트나 예외 케이스가 있는지 궁금합니다.
현재 파일 구조를 components, render, storage, api 등으로 나누어 보았는데,
지금 단계에서 역할 분리가 적절한지 궁금합니다. 특히 components와 render의 경계가 명확하지 않은 것 같은데, 조언 부탁드립니다..
✅ 리뷰어 체크 포인트