[2단계 - 상세 정보 & UI/UX 개선하기] 파라디 미션 제출합니다. #300
Open
JetProc wants to merge 49 commits intowoowacourse:jetprocfrom
Open
[2단계 - 상세 정보 & UI/UX 개선하기] 파라디 미션 제출합니다. #300JetProc wants to merge 49 commits intowoowacourse:jetprocfrom
JetProc wants to merge 49 commits intowoowacourse:jetprocfrom
Conversation
|
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이 변경사항은 영화 상세 모달 기능, 무한 스크롤 구현, 사용자 별점 시스템을 추가합니다. 기존 "더 보기" 버튼을 IntersectionObserver 기반의 무한 스크롤 센티널로 대체하고, 모달 다이얼로그에서 영화 포스터, 메타데이터, 5점 별점 평가 UI를 제공합니다. 별점은 localStorage에 지속적으로 저장되며, API 계층은 상세 영화 조회 엔드포인트를 추가합니다. 새로운 repository, controller, service 패턴으로 상태 관리 및 모달 생명주기를 관리하고, 반응형 CSS로 데스크톱/모바일 환경을 지원합니다. Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 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: 11
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/main.ts (1)
256-275:⚠️ Potential issue | 🔴 Critical치명적 버그:
bindEvents함수 중복 정의파일 내에
bindEvents함수가 두 번 정의되어 있습니다 (118번 줄과 256번 줄). JavaScript/TypeScript에서 같은 스코프 내 함수 중복 정의 시 두 번째 정의가 첫 번째를 덮어씁니다. 결과적으로 무한 스크롤, 모달 제어, 키보드 이벤트 등 대부분의 기능이 동작하지 않게 됩니다.256-275번 줄의 코드는 리팩토링 전 레거시 코드로 보이며, 제거해야 합니다.
🐛 중복 함수 제거
window.addEventListener("load", () => { void main(); }); - -const bindEvents = (elements: AppElements) => { - elements.seeMoreBtn.addEventListener("click", async (event) => { - event.preventDefault(); - await executeWithErrorHandling(elements, () => loadMovies(elements)); - }); - - elements.searchForm.addEventListener("submit", async (event) => { - event.preventDefault(); - - const query = elements.searchInput.value.trim(); - - if (!query) { - notifyEmptyQuery(); - elements.searchInput.focus(); - return; - } - - await executeWithErrorHandling(elements, () => loadMovies(elements, query, true)); - }); -};🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/main.ts` around lines 256 - 275, Remove the legacy duplicate bindEvents implementation that only registers seeMoreBtn and searchForm handlers (the duplicate definition of bindEvents), leaving the original comprehensive bindEvents in place; locate the second bindEvents declaration (the one adding event listeners for elements.seeMoreBtn and elements.searchForm) and delete it so the single remaining bindEvents handles infinite scroll, modal controls, keyboard events, etc., preserving references to AppElements and executeWithErrorHandling.
🧹 Nitpick comments (9)
tests/API/api.test.ts (1)
119-128: 테스트 훅 중복은 공통 헬퍼로 묶는 것을 고려해보세요.
beforeEach/afterEach패턴이 기존 블록과 거의 동일합니다. 다음 단계로, 어떤 공통 초기화/정리를 하나의 재사용 가능한 단위로 추출하면 수정 포인트를 줄일 수 있을지 점검해보면 좋겠습니다.
As per coding guidelines "Promote modular thinking—breaking problems into reusable components."🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/API/api.test.ts` around lines 119 - 128, Extract the repeated test hooks into a reusable helper (e.g., create and export a function like setupTestEnv or registerDefaultTestHooks) that encapsulates vi.stubEnv("VITE_TMDB_API_KEY", "test-api-key") in a beforeEach and the vi.unstubAllEnvs(), vi.unstubAllGlobals(), vi.restoreAllMocks(), vi.useRealTimers() in an afterEach; then replace the inline beforeEach/afterEach in tests/API/api.test.ts with a call to that helper (or import and invoke it in each describe) so the initialization/teardown logic is centralized and reusable across test files.src/repositories/MovieRatingRepository.ts (1)
1-4: 저장소 레이어 의존성을 분리하는 편이 좋습니다.Line 3에서 repository가 service를 참조하면 계층 경계가 약해집니다. 별점 유효성 검사는 공용 domain/type 유틸로 이동해 repository/service가 공통 참조하도록 정리해보세요.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/repositories/MovieRatingRepository.ts` around lines 1 - 4, MovieRatingRepository currently imports isMovieUserRating from MovieRatingService which breaks layer boundaries; extract the validation function into a shared domain/utility module (e.g., a new movieRating validation/utility file) and update both MovieRatingRepository and MovieRatingService to import isMovieUserRating from that new common module; ensure the repository still uses MOVIE_USER_RATING_STORAGE_KEY and MovieUserRating type but no longer references MovieRatingService, and update any references in MovieRatingRepository and MovieRatingService to the new import location for isMovieUserRating to restore proper separation.src/main.ts (1)
199-207:close이벤트와keydownESC 핸들러 중복 가능성
HTMLDialogElement는 ESC 키 입력 시 자동으로 "close" 이벤트를 발생시킵니다. 209-215번 줄의keydown핸들러가 ESC를 감지하여detailController.close()를 호출하면, dialog의 기본 ESC 동작도 발생하여 "close" 이벤트가 두 번 처리될 수 있습니다.event.preventDefault()를 추가하거나 하나의 핸들러만 사용하는 것을 고려하세요.♻️ ESC 키 처리 개선 제안
document.addEventListener("keydown", (event) => { if (event.key !== "Escape" || !elements.modalBackground.open) { return; } + event.preventDefault(); // dialog 기본 ESC 동작 방지 detailController.close(); });🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/main.ts` around lines 199 - 207, The dialog's ESC handling can trigger both the keydown listener and the native close event causing duplicate processing; update the keydown handler that listens for "Escape" (which calls detailController.close()) to call event.preventDefault() (or remove the keydown handler) so the native HTMLDialogElement close event is the single source of truth, and ensure elements.modalBackground.addEventListener("close", ...) remains the place that performs syncMovieDetailModalClosedState(elements), detailController.syncClosedState(), and clearMovieDetailTriggerFocus() to avoid double-invocation.tests/services/serviceModules.test.ts (2)
314-314:as never타입 캐스팅 사용 개선 고려여러 곳에서
as never캐스팅을 사용하고 있습니다. 이는 타입 안전성을 우회하므로, 테스트용 mock 타입을 별도로 정의하거나Partial<AppElements>같은 타입을 활용하면 더 안전합니다.Also applies to: 342-344, 363-363, 375-375, 382-382, 400-400
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/services/serviceModules.test.ts` at line 314, Replace the unsafe "as never" casts used when calling initializeMovieDetailModal (and the other test calls noted) with a properly typed mock object: define a Partial or mock interface for AppElements (e.g., Partial<AppElements> or MockAppElements) containing only the properties the tests use, construct mock element objects matching those keys, and pass that typed mock into initializeMovieDetailModal (and the other test helpers) instead of using "as never" to preserve type safety and clarity.
253-386: 단일 테스트 케이스가 너무 많은 동작을 검증함
상세 모달 요소를 초기화하고 영화 상세 정보를 렌더링한다테스트가initializeMovieDetailModal,renderMovieDetail,openMovieDetailModal,clearMovieDetailModal,closeMovieDetailModal,syncMovieDetailModalClosedState를 모두 한 번에 테스트하고 있습니다. 각 함수별로 테스트를 분리하면 실패 시 원인 파악이 쉬워지고, 테스트 의도가 명확해집니다.♻️ 테스트 분리 예시
describe("MovieDetailModalService", () => { // 공통 mock 요소 setup... it("initializeMovieDetailModal이 모달 요소를 초기 상태로 설정한다", () => { // initializeMovieDetailModal 관련 assertions만 }); it("renderMovieDetail이 영화 정보를 모달에 렌더링한다", () => { // renderMovieDetail 관련 assertions만 }); it("openMovieDetailModal이 모달을 열고 active 클래스를 추가한다", () => { // openMovieDetailModal 관련 assertions만 }); // ... 각 함수별 테스트 });🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/services/serviceModules.test.ts` around lines 253 - 386, The test "상세 모달 요소를 초기화하고 영화 상세 정보를 렌더링한다" is exercising six separate behaviors in one case; split it into focused tests for each exported function (initializeMovieDetailModal, renderMovieDetail, openMovieDetailModal, clearMovieDetailModal, closeMovieDetailModal, syncMovieDetailModalClosedState). Create shared mock setup (dialog, document.body stub, ratingButtons, elements) in a beforeEach or top-level describe block, then in each it block call only the single target function (e.g., call initializeMovieDetailModal and assert only its expected side-effects on elements and mocks), move render assertions into a renderMovieDetail test, open-related assertions into openMovieDetailModal test, etc., and remove cross-function calls between tests so failures pinpoint the specific function.src/services/MovieDetailModalService.ts (1)
56-71: 버튼 인덱스와 MOVIE_USER_RATING_OPTIONS 배열의 암묵적 결합
myRatingButtons의 인덱스가MOVIE_USER_RATING_OPTIONS배열 순서와 일치한다고 가정하고 있습니다. HTML에서 버튼 순서가 변경되거나 배열이 수정되면 별점 표시가 깨질 수 있습니다. 버튼의data-user-rating속성을 활용하여 명시적으로 매핑하는 방안을 고려해 보세요.♻️ data-user-rating 속성 활용 제안
export const renderMovieUserRating = (userRating: MovieUserRating | null, elements: AppElements) => { elements.myRatingMessage.textContent = getMovieUserRatingLabel(userRating); elements.myRatingScore.textContent = formatMovieUserRatingScore(userRating); - elements.myRatingButtons.forEach((button, index) => { + elements.myRatingButtons.forEach((button) => { const starImage = button.querySelector("img"); - const thresholdRating = MOVIE_USER_RATING_OPTIONS[index]; + const thresholdRating = Number(button.dataset.userRating); const isFilledStar = userRating !== null && thresholdRating <= userRating; button.setAttribute("aria-pressed", String(isFilledStar)); if (starImage) { starImage.src = isFilledStar ? IMAGE_URL.FILLED_STAR_IMAGE_URL : IMAGE_URL.STAR_IMAGE_URL; } }); };🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/services/MovieDetailModalService.ts` around lines 56 - 71, renderMovieUserRating assumes myRatingButtons index aligns with MOVIE_USER_RATING_OPTIONS; change it to read each button's data-user-rating attribute instead of using the index. In renderMovieUserRating, for each button in elements.myRatingButtons get const thresholdRating = Number(button.dataset.userRating) (with a safe fallback if NaN), compute isFilledStar using that threshold and userRating, then set aria-pressed and update the star image accordingly; keep existing functions getMovieUserRatingLabel/formatMovieUserRatingScore unchanged and ensure buttons without a valid data-user-rating fail safely (e.g., treat as not filled).src/services/MovieDetailController.ts (2)
74-85: 별점 저장 실패 시 에러 처리 고려
movieRatingRepository.set()호출이 실패해도 예외가 상위로 전파되어 이후 로직(상태 업데이트, 렌더링)이 실행되지 않습니다. 이것이 의도된 동작인지 확인이 필요합니다. 만약 저장 실패 시에도 UI 업데이트가 필요하다면, try-catch로 감싸거나 fire-and-forget 방식을 고려해 보세요.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/services/MovieDetailController.ts` around lines 74 - 85, The current updateUserRating function awaits movieRatingRepository.set and will abort the local state update and render if that promise rejects; wrap the persistence call in a try-catch so that on persistence failure you log/report the error but still call applyMovieUserRating(currentMovieDetail, userRating), onMovieRated(currentMovieDetailId, userRating) and renderMovieDetail(currentMovieDetail, elements) (or alternatively rethrow if stopping is desired); specifically modify updateUserRating to try { await movieRatingRepository.set(currentMovieDetailId, userRating) } catch (err) { /* log/report err */ } then continue with the existing calls to applyMovieUserRating, onMovieRated and renderMovieDetail.
92-94:syncClosedState의 역할 명확화 필요
syncClosedState는resetCurrentMovieDetailState만 호출하고 UI 상태는 건드리지 않습니다.main.ts의 "close" 이벤트 핸들러에서syncMovieDetailModalClosedState와 함께 호출되는 것을 보면 의도적인 분리로 보이나, 함수명에 비해 동작이 제한적입니다. 필요시 JSDoc 주석으로 용도를 명시하면 유지보수에 도움이 됩니다.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/services/MovieDetailController.ts` around lines 92 - 94, syncClosedState currently only calls resetCurrentMovieDetailState and does not modify UI; add a concise JSDoc above the syncClosedState function describing its exact purpose (e.g., "resets movie detail data only; UI state handled separately by syncMovieDetailModalClosedState invoked from main.ts on close") and note that it's intentionally separated from UI updates performed in syncMovieDetailModalClosedState (or optionally rename syncClosedState to resetMovieDetailStateIf you prefer clearer naming). Ensure the JSDoc references resetCurrentMovieDetailState and the main.ts "close" handler usage for future maintainers.cypress/e2e/spec.cy.ts (1)
306-313: 재시도 테스트의 타이밍 의존성
cy.wait(100)으로 Observer 반응을 대기하고 있습니다. CI 환경이나 느린 머신에서 flaky 테스트가 될 수 있습니다. 가능하다면 상태 기반 assertion으로 대체하거나, retry-ability를 활용하세요.♻️ 개선 제안
// 재시도를 위해 모킹을 다시 성공으로 변경하고 상단으로 스크롤(sentinel 화면 이탈) 후 다시 내림 mockPopularMoviePage(2, "getPopularMoviesPage2Retry"); cy.scrollTo("top"); - cy.wait(100); // Observer 반응 대기 + // sentinel이 화면에서 벗어났는지 확인 + cy.get("#infinite-scroll-sentinel").should("not.be.visible"); cy.get("#infinite-scroll-sentinel").scrollIntoView(); cy.wait("@getPopularMoviesPage2Retry"); expectMovieList(2, "인기 영화");🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@cypress/e2e/spec.cy.ts` around lines 306 - 313, The test is flaky because it uses a fixed cy.wait(100) to let the IntersectionObserver react; replace that timing-based wait with a stateful, retryable assertion: remove cy.wait(100) and instead assert the sentinel's state (e.g., cy.get('#infinite-scroll-sentinel').should('not.be.visible') after cy.scrollTo('top')) before calling cy.get('#infinite-scroll-sentinel').scrollIntoView(), then rely on the existing network alias wait (cy.wait('@getPopularMoviesPage2Retry')) and final assertion expectMovieList(2, "인기 영화"); this uses the existing mockPopularMoviePage, `#infinite-scroll-sentinel` selector, and `@getPopularMoviesPage2Retry` alias to make the test robust.
🤖 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 99: aria-label 텍스트에 오탈자가 있습니다; 위치한 문자열 aria-label="2점 - 최악이예요"를 찾아
"최악이에요"로 교정하여 aria-label="2점 - 최악이에요" → aria-label="2점 - 최악이에요"가 아니라
aria-label="2점 - 최악이에요"의 "최악이예요"를 "최악이에요"로 바꾸어 접근성 문구를 수정하세요 (검색 키워드:
aria-label="2점 - 최악이예요").
In `@README.md`:
- Around line 37-43: The README has conflicting UI/UX descriptions: section "5.
☘️ UI/UX 개선" documents an infinite-scroll implementation (IntersectionObserver
sentinel) while earlier lines (around the "더보기 버튼" mentions) still describe a
"더보기" button; update the document to consistently describe the chosen
infinite-scroll approach by removing or replacing any "더보기 버튼" references (e.g.,
the checklist items at lines ~14-15 and ~22-24), ensure the
sentinel/IntersectionObserver behavior and page-increment/fetch logic are
clearly described (reference "5. ☘️ UI/UX 개선" and its checklist), and adjust
checklist state/phrasing so the README no longer contradicts itself.
In `@src/main.ts`:
- Around line 217-231: The runtime/type mismatch arises because MovieUserRating
is declared as keyof typeof MOVIE_USER_RATING_LABELS (a string union like
"2"|"4"|"6"|"8"|"10") while isMovieUserRating checks typeof value === "number";
fix by making the compile-time type match the runtime values: either change
MovieUserRating to a numeric union derived from MOVIE_USER_RATING_OPTIONS (e.g.,
infer it from that const) or adjust MOVIE_USER_RATING_LABELS/OPTIONS to use
string keys; then update isMovieUserRating to correctly narrow (keep the number
check if MovieUserRating is numeric) and remove the unsafe cast in the click
handler so detailController.updateUserRating receives a properly typed value
(references: MovieUserRating, isMovieUserRating, MOVIE_USER_RATING_LABELS,
MOVIE_USER_RATING_OPTIONS, elements.myRatingButtons,
detailController.updateUserRating).
In `@src/repositories/MovieRatingRepository.ts`:
- Around line 53-59: The writeRatings method currently calls
this.storage.setItem(MOVIE_USER_RATING_STORAGE_KEY, JSON.stringify(ratings))
without guarding against storage errors; wrap that setItem call in a try/catch
inside writeRatings, swallow the exception (do not rethrow), log the error
(e.g., console.error or repository logger) and fall back to an in-memory cache
field so the app can continue functioning when localStorage is full or blocked;
keep the rest of behavior unchanged and ensure the catch covers
SecurityError/QuotaExceededError cases.
In `@src/services/RenderService.ts`:
- Around line 24-26: The img element created as thumbnail (variable thumbnail)
lacks an alt attribute; update the RenderService code where thumbnail is
configured (around thumbnail.className and thumbnail.src) to set thumbnail.alt
appropriately: if the card already displays the movie title elsewhere use
thumbnail.alt = "" to mark it decorative, otherwise set thumbnail.alt to a
descriptive string derived from the movie title (e.g., `${movieTitle} poster`)
using the existing posterImageUrl/movie title variable in scope.
In `@src/styles/main.css`:
- Around line 238-248: Remove the unnecessary blank lines inside the
.hero-backdrop rule to satisfy the Stylelint declaration-empty-line-before rule:
collapse the spacing so CSS declarations (width, height, background-color,
background-position, background-size, background-repeat, transition) are
contiguous without empty lines between them and keep a single newline only where
block separation is required.
- Around line 124-134: The fixed .footer is taller than the current `#app`
padding-bottom, causing content to be obscured; define a consistent footer
height (or CSS variable like --footer-height) based on the actual rendered
.footer (e.g., ~120px), set .footer to use that height (or min-height) and
update `#app`'s padding-bottom to var(--footer-height), and adjust values in the
existing mobile/desktop media queries so the padding-bottom equals the footer
height (or use a single responsive variable) to ensure no content is hidden
behind .footer; reference the .footer selector and the `#app` rule to apply these
changes.
In `@src/styles/modal.css`:
- Around line 198-205: Rename the keyframe identifier to kebab-case to satisfy
the Stylelint keyframes-name-pattern: change the `@keyframes` name from slideUp to
slide-up and update every reference to it (e.g., any animation, animation-name
or animation-name shorthand usage that currently mentions slideUp around the
block referenced at lines ~220–222) so they point to slide-up instead.
In `@src/styles/thumbnail.css`:
- Around line 48-66: The current CSS removes outlines (.item:focus { outline:
none; }) and only relies on transform for focus, which harms keyboard
visibility; restore an accessible focus indicator by removing the global outline
suppression or overriding it for keyboard focus—update the .item:focus and
.item:focus-visible rules to add a clear focus ring (e.g., a visible outline or
a high-contrast box-shadow and a visible offset when transform is applied) and
keep transform transitions intact; ensure the selector used is
.item:focus-visible (and optionally .item:focus for browsers without
:focus-visible) so keyboard users get the explicit ring while pointer users
still get hover/transform behavior.
In `@types/dom.ts`:
- Around line 14-17: The code still references elements.seeMoreBtn in bindEvents
causing a TS error because AppElements no longer defines seeMoreBtn; update
bindEvents to remove any usage of seeMoreBtn (e.g.,
elements.seeMoreBtn.addEventListener) and either delete that block or replace it
to use elements.infiniteScrollSentinel (attach intersection observer or its
handler) so all interactions now rely on infiniteScrollSentinel; ensure you also
remove any helper functions or listeners named seeMoreHandler/seeMoreClick tied
to seeMoreBtn to prevent unused references.
In `@types/movieRating.ts`:
- Line 2: Replace the misspelled Korean label "최악이예요" with the correct phrasing
"최악이에요" in the movie rating labels array/constant so the user-facing text reads
naturally; locate the string value in types/movieRating.ts and update it
accordingly.
---
Outside diff comments:
In `@src/main.ts`:
- Around line 256-275: Remove the legacy duplicate bindEvents implementation
that only registers seeMoreBtn and searchForm handlers (the duplicate definition
of bindEvents), leaving the original comprehensive bindEvents in place; locate
the second bindEvents declaration (the one adding event listeners for
elements.seeMoreBtn and elements.searchForm) and delete it so the single
remaining bindEvents handles infinite scroll, modal controls, keyboard events,
etc., preserving references to AppElements and executeWithErrorHandling.
---
Nitpick comments:
In `@cypress/e2e/spec.cy.ts`:
- Around line 306-313: The test is flaky because it uses a fixed cy.wait(100) to
let the IntersectionObserver react; replace that timing-based wait with a
stateful, retryable assertion: remove cy.wait(100) and instead assert the
sentinel's state (e.g.,
cy.get('#infinite-scroll-sentinel').should('not.be.visible') after
cy.scrollTo('top')) before calling
cy.get('#infinite-scroll-sentinel').scrollIntoView(), then rely on the existing
network alias wait (cy.wait('@getPopularMoviesPage2Retry')) and final assertion
expectMovieList(2, "인기 영화"); this uses the existing mockPopularMoviePage,
`#infinite-scroll-sentinel` selector, and `@getPopularMoviesPage2Retry` alias to
make the test robust.
In `@src/main.ts`:
- Around line 199-207: The dialog's ESC handling can trigger both the keydown
listener and the native close event causing duplicate processing; update the
keydown handler that listens for "Escape" (which calls detailController.close())
to call event.preventDefault() (or remove the keydown handler) so the native
HTMLDialogElement close event is the single source of truth, and ensure
elements.modalBackground.addEventListener("close", ...) remains the place that
performs syncMovieDetailModalClosedState(elements),
detailController.syncClosedState(), and clearMovieDetailTriggerFocus() to avoid
double-invocation.
In `@src/repositories/MovieRatingRepository.ts`:
- Around line 1-4: MovieRatingRepository currently imports isMovieUserRating
from MovieRatingService which breaks layer boundaries; extract the validation
function into a shared domain/utility module (e.g., a new movieRating
validation/utility file) and update both MovieRatingRepository and
MovieRatingService to import isMovieUserRating from that new common module;
ensure the repository still uses MOVIE_USER_RATING_STORAGE_KEY and
MovieUserRating type but no longer references MovieRatingService, and update any
references in MovieRatingRepository and MovieRatingService to the new import
location for isMovieUserRating to restore proper separation.
In `@src/services/MovieDetailController.ts`:
- Around line 74-85: The current updateUserRating function awaits
movieRatingRepository.set and will abort the local state update and render if
that promise rejects; wrap the persistence call in a try-catch so that on
persistence failure you log/report the error but still call
applyMovieUserRating(currentMovieDetail, userRating),
onMovieRated(currentMovieDetailId, userRating) and
renderMovieDetail(currentMovieDetail, elements) (or alternatively rethrow if
stopping is desired); specifically modify updateUserRating to try { await
movieRatingRepository.set(currentMovieDetailId, userRating) } catch (err) { /*
log/report err */ } then continue with the existing calls to
applyMovieUserRating, onMovieRated and renderMovieDetail.
- Around line 92-94: syncClosedState currently only calls
resetCurrentMovieDetailState and does not modify UI; add a concise JSDoc above
the syncClosedState function describing its exact purpose (e.g., "resets movie
detail data only; UI state handled separately by syncMovieDetailModalClosedState
invoked from main.ts on close") and note that it's intentionally separated from
UI updates performed in syncMovieDetailModalClosedState (or optionally rename
syncClosedState to resetMovieDetailStateIf you prefer clearer naming). Ensure
the JSDoc references resetCurrentMovieDetailState and the main.ts "close"
handler usage for future maintainers.
In `@src/services/MovieDetailModalService.ts`:
- Around line 56-71: renderMovieUserRating assumes myRatingButtons index aligns
with MOVIE_USER_RATING_OPTIONS; change it to read each button's data-user-rating
attribute instead of using the index. In renderMovieUserRating, for each button
in elements.myRatingButtons get const thresholdRating =
Number(button.dataset.userRating) (with a safe fallback if NaN), compute
isFilledStar using that threshold and userRating, then set aria-pressed and
update the star image accordingly; keep existing functions
getMovieUserRatingLabel/formatMovieUserRatingScore unchanged and ensure buttons
without a valid data-user-rating fail safely (e.g., treat as not filled).
In `@tests/API/api.test.ts`:
- Around line 119-128: Extract the repeated test hooks into a reusable helper
(e.g., create and export a function like setupTestEnv or
registerDefaultTestHooks) that encapsulates vi.stubEnv("VITE_TMDB_API_KEY",
"test-api-key") in a beforeEach and the vi.unstubAllEnvs(),
vi.unstubAllGlobals(), vi.restoreAllMocks(), vi.useRealTimers() in an afterEach;
then replace the inline beforeEach/afterEach in tests/API/api.test.ts with a
call to that helper (or import and invoke it in each describe) so the
initialization/teardown logic is centralized and reusable across test files.
In `@tests/services/serviceModules.test.ts`:
- Line 314: Replace the unsafe "as never" casts used when calling
initializeMovieDetailModal (and the other test calls noted) with a properly
typed mock object: define a Partial or mock interface for AppElements (e.g.,
Partial<AppElements> or MockAppElements) containing only the properties the
tests use, construct mock element objects matching those keys, and pass that
typed mock into initializeMovieDetailModal (and the other test helpers) instead
of using "as never" to preserve type safety and clarity.
- Around line 253-386: The test "상세 모달 요소를 초기화하고 영화 상세 정보를 렌더링한다" is exercising
six separate behaviors in one case; split it into focused tests for each
exported function (initializeMovieDetailModal, renderMovieDetail,
openMovieDetailModal, clearMovieDetailModal, closeMovieDetailModal,
syncMovieDetailModalClosedState). Create shared mock setup (dialog,
document.body stub, ratingButtons, elements) in a beforeEach or top-level
describe block, then in each it block call only the single target function
(e.g., call initializeMovieDetailModal and assert only its expected side-effects
on elements and mocks), move render assertions into a renderMovieDetail test,
open-related assertions into openMovieDetailModal test, etc., and remove
cross-function calls between tests so failures pinpoint the specific function.
🪄 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: 490abfa9-81d9-4bc9-9124-01d23d61fd84
📒 Files selected for processing (31)
.gitignoreREADME.mdcypress.config.tscypress/e2e/spec.cy.tsindex.htmlsrc/API/api.tssrc/API/api.types.tssrc/API/apiBuilder.tssrc/constants/constant.tssrc/main.tssrc/repositories/MovieRatingRepository.tssrc/services/AppElementService.tssrc/services/MovieDetailController.tssrc/services/MovieDetailModalService.tssrc/services/MoviePageStateService.tssrc/services/MovieRatingService.tssrc/services/MovieService.tssrc/services/NotifyService.tssrc/services/RenderService.tssrc/styles/main.csssrc/styles/modal.csssrc/styles/reset.csssrc/styles/thumbnail.csssrc/utils/util.tstests/API/api.test.tstests/API/apiBuilder.test.tstests/services/movieRatingModules.test.tstests/services/serviceModules.test.tstypes/dom.tstypes/movie.tstypes/movieRating.ts
Comment on lines
+217
to
+231
| elements.myRatingButtons.forEach((button) => { | ||
| button.addEventListener("click", async () => { | ||
| const ratingValue = Number(button.dataset.userRating); | ||
|
|
||
| if (!isMovieUserRating(ratingValue)) { | ||
| return; | ||
| } | ||
|
|
||
| try { | ||
| await detailController.updateUserRating(ratingValue as MovieUserRating); | ||
| } catch (error) { | ||
| notifyError(error); | ||
| } | ||
| }); | ||
| }); |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Description: isMovieUserRating 함수가 올바른 type predicate를 반환하는지 확인
ast-grep --pattern $'export const isMovieUserRating = ($$$): $_ is MovieUserRating => {
$$$
}'Repository: woowacourse/javascript-movie-review
Length of output: 373
🏁 Script executed:
# Search for MovieUserRating type definition
rg "type MovieUserRating|enum MovieUserRating|interface MovieUserRating" -A 5Repository: woowacourse/javascript-movie-review
Length of output: 1592
🏁 Script executed:
# Check the imports and type definitions in src/main.ts around the usage
rg "MovieUserRating" src/main.ts -B 2 -A 2Repository: woowacourse/javascript-movie-review
Length of output: 1307
🏁 Script executed:
# Look at the MOVIE_USER_RATING_OPTIONS constant to understand the constraint
rg "MOVIE_USER_RATING_OPTIONS" -A 3Repository: woowacourse/javascript-movie-review
Length of output: 1767
🏁 Script executed:
# Find and display MOVIE_USER_RATING_LABELS definition
rg "MOVIE_USER_RATING_LABELS" -A 10 | head -50Repository: woowacourse/javascript-movie-review
Length of output: 2180
타입 프리디케이트와 런타임 값의 타입 불일치 문제
isMovieUserRating 함수의 타입 프리디케이트가 올바르게 작동하지 않는 이유를 파악했습니다.
현재 MovieUserRating은 keyof typeof MOVIE_USER_RATING_LABELS로 정의되어 "2" | "4" | "6" | "8" | "10" (문자열 리터럴 유니온)입니다. 그러나 isMovieUserRating 함수는 typeof value === "number"를 확인하면서도 value is MovieUserRating으로 좁혀진다고 선언하고 있습니다.
TypeScript가 이 불일치를 감지하여 타입 좁혀지기를 수행하지 못하므로, ratingValue as MovieUserRating 캐스팅이 필요합니다.
다음을 검토하세요:
MOVIE_USER_RATING_OPTIONS는 숫자 배열이지만as MovieUserRating[]로 어설션되어 있습니다MovieUserRating타입 정의가 실제 런타임 값 타입(숫자)과 일치하는지 확인하세요
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/main.ts` around lines 217 - 231, The runtime/type mismatch arises because
MovieUserRating is declared as keyof typeof MOVIE_USER_RATING_LABELS (a string
union like "2"|"4"|"6"|"8"|"10") while isMovieUserRating checks typeof value ===
"number"; fix by making the compile-time type match the runtime values: either
change MovieUserRating to a numeric union derived from MOVIE_USER_RATING_OPTIONS
(e.g., infer it from that const) or adjust MOVIE_USER_RATING_LABELS/OPTIONS to
use string keys; then update isMovieUserRating to correctly narrow (keep the
number check if MovieUserRating is numeric) and remove the unsafe cast in the
click handler so detailController.updateUserRating receives a properly typed
value (references: MovieUserRating, isMovieUserRating, MOVIE_USER_RATING_LABELS,
MOVIE_USER_RATING_OPTIONS, elements.myRatingButtons,
detailController.updateUserRating).
Comment on lines
+53
to
+59
| private writeRatings(ratings: StoredMovieRatings) { | ||
| if (!this.storage) { | ||
| return; | ||
| } | ||
|
|
||
| this.storage.setItem(MOVIE_USER_RATING_STORAGE_KEY, JSON.stringify(ratings)); | ||
| } |
There was a problem hiding this comment.
localStorage.setItem 예외를 방어해주세요.
Line 58은 저장 용량 초과/브라우저 정책(SecurityError) 상황에서 throw될 수 있어 사용자 액션 전체를 실패시킬 수 있습니다. 저장 실패를 안전하게 흡수하는 방어 처리(예: try/catch + graceful fallback)가 필요합니다.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/repositories/MovieRatingRepository.ts` around lines 53 - 59, The
writeRatings method currently calls
this.storage.setItem(MOVIE_USER_RATING_STORAGE_KEY, JSON.stringify(ratings))
without guarding against storage errors; wrap that setItem call in a try/catch
inside writeRatings, swallow the exception (do not rethrow), log the error
(e.g., console.error or repository logger) and fall back to an in-memory cache
field so the app can continue functioning when localStorage is full or blocked;
keep the rest of behavior unchanged and ensure the catch covers
SecurityError/QuotaExceededError cases.
Comment on lines
+14
to
+17
| heroDetailButton: HTMLButtonElement; | ||
|
|
||
| skeletonCard: HTMLUListElement; | ||
| seeMoreBtn: HTMLButtonElement; | ||
| infiniteScrollSentinel: HTMLDivElement; |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# 제거된 AppElements 필드 참조 잔존 여부 확인
rg -nP --type=ts -C2 '\bseeMoreBtn\b|\binfiniteScrollSentinel\b'Repository: woowacourse/javascript-movie-review
Length of output: 3002
🏁 Script executed:
# types/dom.ts 전체 내용 확인
cat types/dom.tsRepository: woowacourse/javascript-movie-review
Length of output: 1010
🏁 Script executed:
# AppElementUtil.ts와 AppElementService.ts 비교 - 어느 것이 실제 사용되는지 확인
echo "=== AppElementUtil.ts ===" && cat src/utils/AppElementUtil.ts && echo -e "\n=== AppElementService.ts ===" && cat src/services/AppElementService.tsRepository: woowacourse/javascript-movie-review
Length of output: 2559
🏁 Script executed:
# src/main.ts에서 elements 객체가 어느 파일에서 가져오는지 확인
rg -n -B5 -A2 'elements\.seeMoreBtn|elements\.infiniteScrollSentinel' src/main.ts | head -30Repository: woowacourse/javascript-movie-review
Length of output: 697
seeMoreBtn 참조로 인한 타입 오류 발생 - 즉시 수정 필요
AppElements 타입에서 seeMoreBtn이 제거되었으나, src/main.ts:257의 bindEvents 함수에서 여전히 elements.seeMoreBtn.addEventListener()를 호출하고 있습니다. 이는 TypeScript 컴파일 오류를 유발합니다. infiniteScrollSentinel 기반 무한 스크롤로 전환되었으므로 seeMoreBtn 관련 코드를 제거하거나 infiniteScrollSentinel을 활용하도록 리팩토링해야 합니다.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@types/dom.ts` around lines 14 - 17, The code still references
elements.seeMoreBtn in bindEvents causing a TS error because AppElements no
longer defines seeMoreBtn; update bindEvents to remove any usage of seeMoreBtn
(e.g., elements.seeMoreBtn.addEventListener) and either delete that block or
replace it to use elements.infiniteScrollSentinel (attach intersection observer
or its handler) so all interactions now rely on infiniteScrollSentinel; ensure
you also remove any helper functions or listeners named
seeMoreHandler/seeMoreClick tied to seeMoreBtn to prevent unused references.
JetProc
changed the title
JetProc
changed the title
degurii
requested changes
Apr 13, 2026
degurii
left a comment
degurii
left a comment
There was a problem hiding this comment.
안녕하세요 파라디~
실 변경에 비해 PR 파일 체인지, 컨플릭트가 많아서 확인해보니 step1 작업들이 step2 브랜치에 그대로 포함된 채로 PR이 올라온 것 같아요.
#287 요 PR은 squash merge되어서 woowacourse:jetproc에는 471e32c 커밋 하나만 존재하거든요. 근데 step2 브랜치에서는 그 squash 이후 히스토리가 아니라, squash 전 step1의 개별 커밋들을 기준으로 이어서 작업한 상태로 보입니다.
지금 상태로는 실제로 작업하신 내용만 정확히 보긴 어려워서 베이스 브랜치 기준으로 히스토리를 정리해주시면 좋을 것 같아요. 정리 후에 다시 리뷰요청 부탁드리겠습니다!
- isMovieUserRating 타입 가드로 타입이 좁혀지므로 불안전한 캐스팅을 제거함
Author
안녕하세요 데구리! |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
🎯 미션 소개
🕵️ 셀프 리뷰(Self-Review)
제출 전 체크 리스트
리뷰 요청 & 논의하고 싶은 내용
데구리 안녕하세요! 이번 step2도 잘 부탁드립니다.
1) 이번 단계에서 가장 많이 고민했던 문제와 해결 과정에서 배운 점
비동기 통신 시 중복 요청(Race Condition) 처리
A 영화를 누르고 응답이 오기 전에 B 영화를 누르면, B 영화 모달에 A 영화 데이터가 떴다가 덮어씌워지는 문제가 생겼습니다..
이를 해결하기 위해
latestRequestId라는 상태를 만들고, fetch를 여러 번 할 때 매 요청마다 id를 1씩 증가시켜 부여했습니다.응답이 돌아왔을 때 현재 저장된 최신 id보다 작은 값이면 무시하도록 로직을 짜면서, 비동기 환경에서의 상태 동기화 문제에 대해 고민을 해봤던 것 같습니다.
무한 스크롤 옵저버 인식 버그와 UX 개선
두 가지 방식을 한 번 적용해봤습니다.
하지만 빠르게 스크롤을 내릴 땐 어쩔 수 없이 지나쳐버렸고, 재트리거를 하려면 1)번 때문에 스크롤을 훨씬 위로 올렸다가 다시 내려야 해서 오히려 불편해졌습니다.
그래서 사용자의 자연스러운 스크롤 경험을 우선하여 트리거 위치를 다시 아래로 내리기로 결정했습니다.
타입스크립트 데이터와 타입의 중복 정의 해결 (
as const)2 | 4 | 6 | 8 | 10과 같이 하드코딩된 유니온 타입으로 만들고, 이를Record의 키로 가져다 썼습니다.하지만 이렇게 하니 별점 기준이 바뀌게 된다면 타입과 객체 데이터를 두 곳 모두 수정해야 할 것 같아, 다른 유지보수에 좋은 방법이 없는지 찾아봤습니다.
그러면서
as const와keyof typeof를 사용해 객체의 키값을 Union 타입으로 자동 추출하는 방식을 찾았고,데이터를 먼저 정의하고 거기서 타입을 역산하여 중복을 제거하는 방식으로 코드를 작성해봤습니다.
2) 이번 리뷰를 통해 논의하고 싶은 부분
비동기 최신 요청 처리 방식의 적절성
latestRequestId방식이 프론트엔드 환경에서 비동기 중복 요청을 제어하는 최적화된 방식인지 궁금합니다. 혹시 실무나 더 넓은 관점에서 볼 때 권장되는 더 좋은 패턴이나 문법이 있다면 조언 부탁드립니다.함수 분리의 정도와 가독성의 트레이드오프
DOM 요소 인터페이스 관리 구조 (AppElements)
AppElements인터페이스 내부에HTMLUListElement,HTMLHeadingElement등 수많은 요소들이 쌓이게 되었습니다. 단순히 분리하는 것을 넘어서, 이렇게 하나하나 명시적으로 타입을 지정해 주는 구조가 맞는 것인지, 아니면 DOM 요소를 더 깔끔하게 관리하고 타이핑하는 노하우가 있는지 궁금합니다.E2E 테스트(Cypress) 셀렉터의 유연성 확보
cy.get("#infinite-scroll-sentinel")로 바꿔주어야 했습니다. 이 과정이 마치 하드코딩된 값을 수정하는 것처럼 느껴졌는데, UI나 기능이 변경될 때 테스트 코드 수정을 최소화할 수 있는 더 좋은 셀렉터 관리 방법이나 테스트 작성 노하우가 있을까요?3) 기타
그 외에 리팩토링을 할 수 있다면 하고 싶은 부분이 몇 군데 있지만, 우선 먼저 리뷰 요청 드립니다! 아래는 제가 고려하고 있던 리팩토링 항목입니다.
ux 관련
코드 관련
✅ 리뷰어 체크 포인트