[2단계 - 상세 정보 & UI/UX 개선하기] 지오 미션 제출합니다.#298
Open
JuHyeong424 wants to merge 60 commits intowoowacourse:juhyeong424from
Open
[2단계 - 상세 정보 & UI/UX 개선하기] 지오 미션 제출합니다.#298JuHyeong424 wants to merge 60 commits intowoowacourse:juhyeong424from
JuHyeong424 wants to merge 60 commits intowoowacourse:juhyeong424from
Conversation
요약Walkthrough이 PR은 영화 앱에 영화 상세 정보 모달, 별점 평가 기능, 무한 스크롤 페이지네이션, 그리고 에러 복구 UI를 추가합니다. "더보기" 버튼은 마지막 항목 감시를 통한 무한 스크롤로 대체되었습니다. 영화 상세 정보, 장르, 출시일, 평점, 개요를 표시하는 모달이 추가되었으며, 사용자 별점은 localStorage에 저장됩니다. 컨트롤러, 뷰, 서비스, 모델 계층이 새로 추가되고 기존 컴포넌트는 리팩토링되었습니다. 반응형 레이아웃이 CSS에 구현되었고, E2E 테스트가 새로운 기능을 검증합니다. API 타입 정의와 엔드포인트가 확장되었습니다. Estimated code review effort🎯 4 (Complex) | ⏱️ ~75 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: 18
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
cypress/support/commands.ts (1)
53-60:⚠️ Potential issue | 🟠 Major
@searchMoviesalias가 존재하지 않습니다.54번 라인에서
cy.wait("@searchMovies")를 호출하지만, 26번 라인의mockSearchMovies명령은`searchMovies${page}`로 동적 alias를 생성합니다. 따라서 정적 alias@searchMovies는 존재하지 않습니다.예를 들어, 테스트에서
cy.mockSearchMovies(query, 1, fixture)를 호출하면@searchMovies1alias가 생성되는데, 현재 코드는@searchMovies를 기다리므로 실패합니다.또한
disappearMoreButton명령은 정의되어 있지만 테스트 코드에서 사용되지 않고 있습니다. 이 명령을 사용할 예정이라면, 페이지 번호를 매개변수로 받아 동적 alias 패턴과 일치시키거나, alias를 수정해야 합니다.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@cypress/support/commands.ts` around lines 53 - 60, The custom command disappearMoreButton currently waits on a non-existent static alias "@searchMovies"; update disappearMoreButton to accept a page parameter (e.g., page) and call cy.wait(`@searchMovies${page}`) to match the dynamic alias produced by mockSearchMovies (which uses `searchMovies${page}`), or alternatively change mockSearchMovies to register a fixed alias `searchMovies` if you want a static wait; ensure the command signature and all test usages are updated to pass the page number so the alias names align with mockSearchMovies.
🧹 Nitpick comments (12)
src/view/templates/movieListsTemplates.ts (1)
21-21:id와data-movie-id를 동시에 쓰는 구조를 한쪽으로 정리하는 게 좋습니다.Line 21에서
data-movie-id를 추가했는데, 현재 클릭 처리 경로는 아직id를 읽고 있습니다. 이벤트 쪽도dataset으로 맞추면 DOMid중복 리스크를 줄이고 의도가 더 명확해집니다.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/view/templates/movieListsTemplates.ts` at line 21, The template currently sets both a DOM id and a data attribute on the list item (line with <li class="movie" id="${escapeHTML(item.id)}" data-movie-id="${item.id}">) while click handling still reads the DOM id; remove the duplicated id attribute and standardize on data-movie-id in the template, then update the click handler (the event listener that reads item id) to read event.currentTarget.dataset.movieId (or otherwise use element.dataset.movieId) instead of element.id so all code uses the dataset and avoids DOM id duplication.src/api/getMovieDetails.ts (1)
4-5: 파라미터 의미를page에서movieId로 맞춰주세요.Line 4의
page는 실제 용도가 페이지가 아니라 영화 ID입니다. 여기 이름을 맞춰두면 이후 페이지네이션 로직과 섞일 때 오해를 크게 줄일 수 있어요.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/api/getMovieDetails.ts` around lines 4 - 5, The function getMovieDetails currently uses a misleading parameter name page; rename the parameter to movieId in the function signature and all internal references (change getMovieDetails(page: number) to getMovieDetails(movieId: number)) and update the call to API_URL.details(page) to API_URL.details(movieId); also update any exported type references or JSDoc if present and search for callers of getMovieDetails to update their argument name/usage accordingly so semantics are consistent with MovieModalData fetching by movie ID.src/view/infiniteScrollView.ts (1)
42-48:updateObserver메서드에서this대신 전역 인스턴스를 참조하고 있습니다.클래스 메서드 내에서
infiniteScrollView(export된 싱글톤)를 직접 참조하는 대신this를 사용해야 합니다. 현재 코드는 동작하지만, 클래스의 재사용성을 저해하고 순환 참조 패턴을 만듭니다. 이 패턴이 의도된 것인지 확인해 보세요.♻️ 제안된 수정
updateObserver(data: MovieResponse) { if (isLastPage(data)) { - infiniteScrollView.disconnect(); + this.disconnect(); } else { - infiniteScrollView.observe(); + this.observe(); }; };🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/view/infiniteScrollView.ts` around lines 42 - 48, The updateObserver method currently references the exported singleton infiniteScrollView instead of the instance context; change the method to use this (e.g., this.disconnect() / this.observe()) so the class uses its own instance rather than a global, keeping isLastPage(data) logic the same; locate updateObserver in the class, replace calls to infiniteScrollView.disconnect() and infiniteScrollView.observe() with this.disconnect() and this.observe(), and ensure the method remains an instance method (not static) so this is valid.src/view/movieListView.ts (1)
56-60:bindThumbnailRetryClick이 여러 번 호출되면 중복 리스너가 등록될 수 있습니다.이 메서드가 여러 번 호출되면 동일한 핸들러가 중복으로 등록될 수 있습니다. 현재 코드 흐름에서 한 번만 호출된다면 문제없지만, 방어적으로 처리하고 싶다면 기존 리스너 제거 또는 플래그를 사용하는 방법이 있습니다. 현재
main.ts에서 한 번만 바인딩된다면 괜찮습니다.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/view/movieListView.ts` around lines 56 - 60, The bindThumbnailRetryClick method can register duplicate listeners; modify it to remove any previously attached listener before adding the new one by storing the handler reference on the instance (e.g., this.#thumbnailRetryHandler) and using removeEventListener on this.#thumbnailRetryButton with that stored reference, then assign and add the new listener; update bindThumbnailRetryClick and any teardown logic to use this stored handler to prevent duplicates.src/services/errorMovieList.ts (1)
6-6:console.log대신 적절한 에러 로깅 방식을 고려해 보세요.프로덕션 코드에서
console.log를 사용하면 사용자의 브라우저 콘솔에 에러 메시지가 노출됩니다. 에러 모니터링 서비스(예: Sentry)로 에러를 전송하거나, 개발 환경에서만 로깅하도록 조건부 처리하는 것이 좋습니다. 현재 단계에서는 괜찮을 수 있지만, 향후 고려해 보세요.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/services/errorMovieList.ts` at line 6, Replace the console.log("에러 원인:", errorMessage) call with a proper error reporting mechanism: remove the console.log and either call your app logger (e.g., logger.error(..., {errorMessage})) or send the error to your monitoring service (e.g., Sentry.captureException(errorMessage)); if you must log locally, wrap the log in a development-only guard (e.g., if (process.env.NODE_ENV === 'development') logger.debug(...)). Ensure you import and use the project's logging/monitoring utilities and reference the existing errorMessage variable when reporting.cypress/e2e/search.cy.ts (1)
39-58: 끝 페이지 시나리오도 하나 남겨 두면 회귀를 더 잘 잡습니다.지금은 2페이지 추가 로드까지만 검증해서, 그 다음 빈 응답에서 기존 리스트를 유지한 채 observer가 멈추는지는 놓칩니다.
handleLoadMore()에 빈 결과 분기가 생긴 만큼 마지막 페이지 다음 호출도 같이 mock해서 확인해 보세요.🤖 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 39 - 58, Add a test case for the end-of-pages scenario by mocking the next page after page 2 to return an empty result and asserting the UI keeps the existing list and no further results are appended; use the same helpers cy.mockSearchMovies to stub the final empty page (e.g., page 3), trigger scrolling the last thumbnail (".thumbnail-list li:last-child") or call the same flow that invokes handleLoadMore(), then wait on the alias for that final request (e.g., "@searchMovies3") and assert the list length remains unchanged and no new items are added to verify the observer stops.src/model/currentMovieModel.ts (1)
14-24: ID 존재 여부는 truthy 대신 null 여부로 나누는 편이 안전합니다.
#currentMovieId의 타입은number | null인데, 지금 분기면0같은 유효한 숫자 ID도 "없음"으로 취급합니다. 의도를 고정하려면 null과 숫자를 명시적으로 구분해 보세요.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/model/currentMovieModel.ts` around lines 14 - 24, The branches in saveRating and getRating incorrectly treat 0 as "no ID" by checking this.#currentMovieId truthiness; change both checks to explicitly differentiate null from a numeric ID (e.g., test this.#currentMovieId !== null or typeof this.#currentMovieId === 'number') so that valid IDs like 0 are accepted, and keep the rest of the logic in saveRating, getRating and the private field `#currentMovieId` unchanged.src/styles/main.css (1)
195-208: 선언문 앞의 빈 줄을 제거하세요.Stylelint가 201번 라인의
z-index선언 앞에 불필요한 빈 줄을 감지했습니다. 일관된 스타일을 위해 제거해주세요.💡 수정 제안
.error-banner-container { position: absolute; top: 50%; left: 50%; transform: translate(-50%, -50%); - z-index: 2; width: 100%; display: flex;🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/styles/main.css` around lines 195 - 208, Remove the extra blank line before the z-index declaration inside the .error-banner-container rule: locate the CSS selector .error-banner-container and delete the empty line immediately preceding the z-index: 2; declaration so the property lines are contiguous and conform to Stylelint expectations.cypress/e2e/modal.cy.ts (3)
28-28:Number대신number를 사용하세요.TypeScript에서 primitive 타입을 나타낼 때는 소문자
number를 사용하는 것이 올바른 컨벤션입니다.Number는 wrapper 객체를 의미합니다.💡 수정 제안
-cy.get("#modal-description-genre").should("contain", movieDetail.genres.map((item: {id: Number, name: string}) => item.name).join(", ")); +cy.get("#modal-description-genre").should("contain", movieDetail.genres.map((item: {id: number, name: string}) => item.name).join(", "));🤖 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 28, The type annotation in the genres mapping uses the boxed type `Number` instead of the primitive `number`; update the inline type in the arrow function inside movieDetail.genres.map (the `(item: {id: Number, name: string}) => ...` declaration) to use `number` for `id` (i.e., `{id: number, name: string}`) so TypeScript correctly treats it as a primitive.
164-164: 163번 라인과 동일한 타입 이슈가 있습니다.여기서도
Number대신number를 사용해야 합니다.🤖 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 164, Update the inline type annotation used in the genres mapping to use the primitive type `number` instead of the boxed `Number`: change the annotation on the `item` parameter in the `movieDetail.genres.map((item: {id: Number, name: string}) => ...)` callback to `{id: number, name: string}` so the type matches line 163 and avoids using the `Number` object type.
65-72: 중복된evaluation객체를 추출하는 것을 고려해보세요.동일한
evaluation객체가 두 테스트에서 반복 정의되고 있습니다. 이 객체를 어디에 위치시키면 중복을 줄일 수 있을지 생각해보시겠어요?힌트:
describe블록 상단이나 별도 fixture 파일을 활용할 수 있습니다.Also applies to: 97-104
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@cypress/e2e/modal.cy.ts` around lines 65 - 72, The evaluation map object (evaluation) is duplicated across tests; extract it to a single shared location to remove duplication — move the const evaluation = {0: "...",1: "...",2: "...",3: "...",4: "...",5: "..."} to the top of the surrounding describe block (so all it() tests can access it) or place it in a separate fixture/module and import it into modal.cy.ts; update references in the tests to use this shared evaluation constant instead of redefining it.cypress/support/commands.ts (1)
81-85: alias 네이밍 일관성 문제: id가 포함되지 않았습니다.
getMovieDetail명령(72번 라인)은`getMovieDetail${id}`를 사용하는 반면,getMovieDetailNetworkError는 id 없이getMovieDetailNetworkError만 사용합니다.동일 테스트 내에서 여러 영화의 에러를 테스트해야 할 경우 alias 충돌이 발생할 수 있습니다. 일관성을 위해 id를 포함하는 것이 어떨까요?
💡 수정 제안
Cypress.Commands.add("getMovieDetailNetworkError", (id: number) => { cy.intercept("GET", `**/movie/${id}*`, { forceNetworkError: true, - }).as(`getMovieDetailNetworkError`); + }).as(`getMovieDetailNetworkError${id}`); });참고: 이 변경을 적용하면
cypress/e2e/modal.cy.ts149번 라인의 wait도`@getMovieDetailNetworkError${id}`로 업데이트해야 합니다.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@cypress/support/commands.ts` around lines 81 - 85, The alias for the intercept in the Cypress command getMovieDetailNetworkError omits the movie id and can collide when testing multiple movies; update the Cypress.Commands.add implementation for getMovieDetailNetworkError so the alias passed to cy.intercept.as includes the id (matching the pattern used by getMovieDetail, e.g., append the id to the alias name) and then update any cy.wait calls that reference the old alias to use the new alias pattern with the id (for example using `@getMovieDetailNetworkError`{id}).
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In @.github/workflows/deploy.yml:
- Line 8: 현재 워크플로의 push 트리거에 "step2"가 포함되어 있어 학습/개발 브랜치(예: step2) 푸시 시에도 deploy
잡(및 github-pages 배포)이 실행됩니다; deploy 잡의 push 트리거에서 "step2"를 제거하거나 push 조건을 main
브랜치로 한정하도록 수정하세요 (예: on.push.branches에서 "step2" 삭제 또는 jobs.deploy.if /
workflow-level 조건을 사용해 github.ref == 'refs/heads/main'으로 제한). 참조할 식별자: deploy
job, on.push.branches 목록, "step2" 브랜치 이름, 그리고 main(또는 refs/heads/main) 조건을
변경하세요.
In `@index.html`:
- Around line 88-91: The <img id="modal-image"> currently has an empty src=""
which causes the browser to request the current page; remove the empty src
attribute from the <img> element (or replace it with a harmless placeholder/data
URI) and set the actual image URL at runtime from your JavaScript logic that
targets document.getElementById("modal-image") so no empty-request is made
during initial render.
- Around line 109-113: Image path usage is inconsistent: some tags use ./images/
while others (including my-star-image-1..5) use ./public/images/, causing 404s;
standardize all image src attributes to the project's canonical path (use
./public/images/ throughout) and update any tags (check elements with class
"my-star-image" and ids my-star-image-1 to my-star-image-5 as well as other
image tags) so every src points to ./public/images/<filename> to make paths
consistent.
In `@README.md`:
- Around line 98-99: The README's endpoint path `/3/movie/movie_id` reads like a
literal; change it to use a variable path parameter such as
`/3/movie/{movie_id}` (or `:movie_id` if following your style) wherever the
Endpoint is documented so it's clear this is a path parameter, and update any
accompanying description or examples that reference `movie_id` to match the new
parameter notation.
- Around line 10-11: The README currently contradicts itself by describing
infinite scroll while also testing removal of a "더보기" (load more) button; pick
one canonical UX (either infinite scroll or a load-more button), update all
related checklist items and scenario descriptions to consistently reflect that
choice, and remove or rewrite the conflicting lines that mention the opposite
behavior (update the entries currently at the spots referenced in the comment
including the occurrences around lines 10-11, 20-21 and 135-140) so every
checklist, scenario, and acceptance criterion consistently states the chosen
behavior.
In `@src/api/localStarApi.ts`:
- Around line 2-9: Wrap localStorage calls in both saveRating and getRating with
try/catch, validate inputs and parsed output: in saveRating(movieId, score)
first ensure score is a number and within 0–10, then call
window.localStorage.setItem inside a try block and catch
SecurityError/QuotaExceededError (log via console.error or a logger and rethrow
or return a failure indicator); in getRating(movieId) call
window.localStorage.getItem inside try/catch, on error return null (or a defined
failure value) and log the error, otherwise parse the value to Number, treat NaN
or out-of-range values as invalid and return a safe default (e.g., 0) or null
per your app policy; include clear error messages that mention the function
names (saveRating/getRating) and the movieId when logging.
In `@src/controller/handleLoadMore.ts`:
- Around line 21-24: The branch in handleLoadMore that calls emptyMovieList()
when response.data.results.length === 0 should not clear already-rendered
results; change it to preserve the current list and only stop further loading by
disconnecting the scroll observer (e.g., call observer.disconnect() or
observerRef.current?.disconnect()) and return. Replace the emptyMovieList() call
with logic to unregister the observer (using the existing observer variable/Ref
in your component) so the UI stays showing current movies while preventing more
load attempts.
In `@src/main.ts`:
- Around line 28-30: Before calling handleModal, validate that clickedMovieId
parses to a valid integer: in the modalView.bindMovieClick callback, parse
clickedMovieId (e.g., via Number/parseInt) and guard against NaN (or
non-integer) results; if invalid, skip calling handleModal and optionally log or
show an error, otherwise call handleModal with the validated numeric id. Ensure
you reference modalView.bindMovieClick, clickedMovieId, and handleModal in the
change.
- Around line 20-22: The implicit protection against duplicate loads relies on
observer.unobserve inside infiniteScrollView.bindBottomIntersect before calling
handleLoadMore, but because handleLoadMore is async and updateObserver
re-attaches the observer after completion, add an explicit loading guard to
prevent race conditions: introduce an isLoading boolean (e.g., module-level or
inside the scroll component), set isLoading = true at the start of
handleLoadMore and return early if already true, and set isLoading = false after
fetchCurrentModeData and updateObserver finish; reference
infiniteScrollView.bindBottomIntersect, handleLoadMore, updateObserver and
observer.unobserve when applying the change.
In `@src/styles/modal.css`:
- Around line 141-148: 모달 스타일 블록에서 z-index와 width 사이의 불필요한 빈 줄 때문에 Stylelint 에러가
발생하고 있으니 src/styles/modal.css의 해당 블록에서 "z-index: 100;" 바로 다음 줄의 공백을 제거하여 프로퍼티들이
연속되도록 정리하세요 (특히 z-index 및 width 프로퍼티를 붙여서 lint 통과하도록 수정합니다).
In `@src/styles/thumbnail.css`:
- Around line 119-138: The hover/active rules target
.error-thumbnail-retry-button but the actual button class is
.thumbnail-retry-button, so the states never apply; update the selectors to
match the real class (replace .error-thumbnail-retry-button:hover and
.error-thumbnail-retry-button:active with .thumbnail-retry-button:hover and
.thumbnail-retry-button:active) or broaden them to include both classes (e.g.,
.thumbnail-retry-button:hover, .error-thumbnail-retry-button:hover and same for
:active) so the hover/active styles take effect for the retry button.
In `@src/view/bannerView.ts`:
- Around line 17-19: The current assignments convert possibly-missing values to
strings which yields "undefined" and inserts "undefined" into the background
URL; update the guards to check for null/undefined before stringifying or using
values: for rate use a nullish check on bannerMovie.vote_average (e.g.
bannerMovie.vote_average != null) and only then call String(...) otherwise use
"0"; for title use bannerMovie.title ?? "제목 없음"; for banner only set
`#banner.style.backgroundImage` when bannerMovie.poster_path is present (otherwise
clear the style or set a safe placeholder), referencing the fields `#rateValue`,
`#title`, `#banner`, bannerMovie.vote_average, bannerMovie.title,
bannerMovie.poster_path and BANNER_IMAGE_URL to locate the code.
In `@src/view/infiniteScrollView.ts`:
- Line 31: The declaration for the DOM query variable lastMovieList has a
duplicated semicolon causing a typo; edit the const lastMovieList declaration
(const lastMovieList = document.querySelector<HTMLLIElement>(".thumbnail-list
li:last-child");;) to remove the extra semicolon and any extra spacing so it
ends with a single semicolon and preserves the HTMLLIElement generic.
In `@src/view/modalView.ts`:
- Around line 82-90: The current falsy checks cause valid values (like
vote_average = 0 or genres = []) to be treated as missing; update the logic
around this.#modalRateNumber and this.#modalDescriptionGenre to distinguish
absent (null/undefined) from valid falsy values: for data.vote_average use an
explicit null/undefined check (or Number.isFinite) so 0 is rendered as "0.0" and
only null/undefined yield "알 수 없음", and for data.genres ensure you check for
existence and non-empty array (e.g., data.genres && data.genres.length > 0)
before mapping/joining, otherwise set "알 수 없음".
- Around line 24-28: The DOM generic types used with querySelector are
incorrect: replace querySelector<HTMLTitleElement> for the fields
`#modalDescriptionTitle`, `#modalDescriptionYear`, `#modalDescriptionGenre`,
`#modalRateNumber`, and `#modalDetailDescription` with the actual element types from
index.html (e.g., .modal-description-title -> HTMLHeadingElement,
`#modal-description-year` / `#modal-description-genre` / `#modal-rate-number` ->
HTMLSpanElement, `#modal-detail-description` -> HTMLParagraphElement), updating
the generics for the corresponding properties (`#modalDescriptionTitle`,
`#modalDescriptionYear`, `#modalDescriptionGenre`, `#modalRateNumber`,
`#modalDetailDescription`) so TypeScript reflects the real element interfaces and
preserves nullability checks after you confirm tag names in index.html.
In `@src/view/myStarRatingView.ts`:
- Around line 25-31: bindHandleMyStarClick currently only listens for "click" on
elements in `#myStarImageArray` so keyboard and assistive tech users cannot
activate stars; make each star focusable (ensure tabindex="0" and optional
role="button") and add a keydown listener alongside the click listener that
detects Enter (KeyEnter) and Space (Space/ ) presses, prevents default for
Space, converts the element's dataset.score to Number, calls
this.renderRating(score) and then handler(score) so keyboard activation behaves
identically to mouse clicks. Ensure you attach these changes to the same
elements referenced in bindHandleMyStarClick/#myStarImageArray.
- Line 10: In the myStarRatingView file update the displayed label string
"최악이예요" to the correct Korean spelling "최악이에요"; locate the star-rating labels
array or constant inside myStarRatingView (the entry currently containing
"최악이예요") and replace that single string so the UI shows "최악이에요".
In `@types/api.d.ts`:
- Around line 20-24: The MovieModalData interface declares vote_average twice,
causing a TypeScript duplicate-property error; open the MovieModalData
definition and remove the redundant vote_average: number; so the interface
contains a single vote_average field (ensure you keep any intended other
properties like poster_path and overview unchanged); if one of the duplicates
was meant to be a different property, rename it to the correct identifier
instead of deleting.
---
Outside diff comments:
In `@cypress/support/commands.ts`:
- Around line 53-60: The custom command disappearMoreButton currently waits on a
non-existent static alias "@searchMovies"; update disappearMoreButton to accept
a page parameter (e.g., page) and call cy.wait(`@searchMovies${page}`) to match
the dynamic alias produced by mockSearchMovies (which uses
`searchMovies${page}`), or alternatively change mockSearchMovies to register a
fixed alias `searchMovies` if you want a static wait; ensure the command
signature and all test usages are updated to pass the page number so the alias
names align with mockSearchMovies.
---
Nitpick comments:
In `@cypress/e2e/modal.cy.ts`:
- Line 28: The type annotation in the genres mapping uses the boxed type
`Number` instead of the primitive `number`; update the inline type in the arrow
function inside movieDetail.genres.map (the `(item: {id: Number, name: string})
=> ...` declaration) to use `number` for `id` (i.e., `{id: number, name:
string}`) so TypeScript correctly treats it as a primitive.
- Line 164: Update the inline type annotation used in the genres mapping to use
the primitive type `number` instead of the boxed `Number`: change the annotation
on the `item` parameter in the `movieDetail.genres.map((item: {id: Number, name:
string}) => ...)` callback to `{id: number, name: string}` so the type matches
line 163 and avoids using the `Number` object type.
- Around line 65-72: The evaluation map object (evaluation) is duplicated across
tests; extract it to a single shared location to remove duplication — move the
const evaluation = {0: "...",1: "...",2: "...",3: "...",4: "...",5: "..."} to
the top of the surrounding describe block (so all it() tests can access it) or
place it in a separate fixture/module and import it into modal.cy.ts; update
references in the tests to use this shared evaluation constant instead of
redefining it.
In `@cypress/e2e/search.cy.ts`:
- Around line 39-58: Add a test case for the end-of-pages scenario by mocking
the next page after page 2 to return an empty result and asserting the UI keeps
the existing list and no further results are appended; use the same helpers
cy.mockSearchMovies to stub the final empty page (e.g., page 3), trigger
scrolling the last thumbnail (".thumbnail-list li:last-child") or call the same
flow that invokes handleLoadMore(), then wait on the alias for that final
request (e.g., "@searchMovies3") and assert the list length remains unchanged
and no new items are added to verify the observer stops.
In `@cypress/support/commands.ts`:
- Around line 81-85: The alias for the intercept in the Cypress command
getMovieDetailNetworkError omits the movie id and can collide when testing
multiple movies; update the Cypress.Commands.add implementation for
getMovieDetailNetworkError so the alias passed to cy.intercept.as includes the
id (matching the pattern used by getMovieDetail, e.g., append the id to the
alias name) and then update any cy.wait calls that reference the old alias to
use the new alias pattern with the id (for example using
`@getMovieDetailNetworkError`{id}).
In `@src/api/getMovieDetails.ts`:
- Around line 4-5: The function getMovieDetails currently uses a misleading
parameter name page; rename the parameter to movieId in the function signature
and all internal references (change getMovieDetails(page: number) to
getMovieDetails(movieId: number)) and update the call to API_URL.details(page)
to API_URL.details(movieId); also update any exported type references or JSDoc
if present and search for callers of getMovieDetails to update their argument
name/usage accordingly so semantics are consistent with MovieModalData fetching
by movie ID.
In `@src/model/currentMovieModel.ts`:
- Around line 14-24: The branches in saveRating and getRating incorrectly treat
0 as "no ID" by checking this.#currentMovieId truthiness; change both checks to
explicitly differentiate null from a numeric ID (e.g., test this.#currentMovieId
!== null or typeof this.#currentMovieId === 'number') so that valid IDs like 0
are accepted, and keep the rest of the logic in saveRating, getRating and the
private field `#currentMovieId` unchanged.
In `@src/services/errorMovieList.ts`:
- Line 6: Replace the console.log("에러 원인:", errorMessage) call with a proper
error reporting mechanism: remove the console.log and either call your app
logger (e.g., logger.error(..., {errorMessage})) or send the error to your
monitoring service (e.g., Sentry.captureException(errorMessage)); if you must
log locally, wrap the log in a development-only guard (e.g., if
(process.env.NODE_ENV === 'development') logger.debug(...)). Ensure you import
and use the project's logging/monitoring utilities and reference the existing
errorMessage variable when reporting.
In `@src/styles/main.css`:
- Around line 195-208: Remove the extra blank line before the z-index
declaration inside the .error-banner-container rule: locate the CSS selector
.error-banner-container and delete the empty line immediately preceding the
z-index: 2; declaration so the property lines are contiguous and conform to
Stylelint expectations.
In `@src/view/infiniteScrollView.ts`:
- Around line 42-48: The updateObserver method currently references the exported
singleton infiniteScrollView instead of the instance context; change the method
to use this (e.g., this.disconnect() / this.observe()) so the class uses its own
instance rather than a global, keeping isLastPage(data) logic the same; locate
updateObserver in the class, replace calls to infiniteScrollView.disconnect()
and infiniteScrollView.observe() with this.disconnect() and this.observe(), and
ensure the method remains an instance method (not static) so this is valid.
In `@src/view/movieListView.ts`:
- Around line 56-60: The bindThumbnailRetryClick method can register duplicate
listeners; modify it to remove any previously attached listener before adding
the new one by storing the handler reference on the instance (e.g.,
this.#thumbnailRetryHandler) and using removeEventListener on
this.#thumbnailRetryButton with that stored reference, then assign and add the
new listener; update bindThumbnailRetryClick and any teardown logic to use this
stored handler to prevent duplicates.
In `@src/view/templates/movieListsTemplates.ts`:
- Line 21: The template currently sets both a DOM id and a data attribute on the
list item (line with <li class="movie" id="${escapeHTML(item.id)}"
data-movie-id="${item.id}">) while click handling still reads the DOM id; remove
the duplicated id attribute and standardize on data-movie-id in the template,
then update the click handler (the event listener that reads item id) to read
event.currentTarget.dataset.movieId (or otherwise use element.dataset.movieId)
instead of element.id so all code uses the dataset and avoids DOM id
duplication.
🪄 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: f690c2a8-1147-4a77-aa0f-3d60203484c4
📒 Files selected for processing (36)
.github/workflows/deploy.ymlREADME.mdcypress/e2e/home.cy.tscypress/e2e/modal.cy.tscypress/e2e/search.cy.tscypress/fixtures/movieDetail.jsoncypress/fixtures/multipleSearchResults1.jsoncypress/fixtures/multipleSearchResults2.jsoncypress/support/commands.tscypress/support/index.d.tsindex.htmlpublic/delete/addButtonView.tssrc/api/getMovieDetails.tssrc/api/localStarApi.tssrc/constants/api.tssrc/controller/handleHome.tssrc/controller/handleLoadMore.tssrc/controller/handleModal.tssrc/controller/handleMyStar.tssrc/controller/handleSearch.tssrc/main.tssrc/model/currentMovieModel.tssrc/services/emptyMovieList.tssrc/services/errorMovieDetail.tssrc/services/errorMovieList.tssrc/services/fetchCurrentModeData.tssrc/styles/main.csssrc/styles/modal.csssrc/styles/thumbnail.csssrc/view/bannerView.tssrc/view/infiniteScrollView.tssrc/view/modalView.tssrc/view/movieListView.tssrc/view/myStarRatingView.tssrc/view/templates/movieListsTemplates.tstypes/api.d.ts
| branches: | ||
| - main | ||
| - step1 | ||
| - step2 |
There was a problem hiding this comment.
step2 푸시까지 Pages 실배포되면 안정 브랜치 결과물을 덮어쓸 수 있습니다
Line 8로 인해 step2 브랜치 푸시도 deploy 잡을 타며 github-pages 환경에 배포됩니다. 학습/개발 브랜치 산출물이 운영 URL에 올라갈 수 있으니, 배포는 main에서만 실행되도록 제한하는 게 안전합니다.
수정 예시
jobs:
build:
+ if: github.ref == 'refs/heads/main'
runs-on: ubuntu-latest
@@
deploy:
+ if: github.ref == 'refs/heads/main'
needs: build
runs-on: ubuntu-latest🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In @.github/workflows/deploy.yml at line 8, 현재 워크플로의 push 트리거에 "step2"가 포함되어 있어
학습/개발 브랜치(예: step2) 푸시 시에도 deploy 잡(및 github-pages 배포)이 실행됩니다; deploy 잡의 push
트리거에서 "step2"를 제거하거나 push 조건을 main 브랜치로 한정하도록 수정하세요 (예: on.push.branches에서
"step2" 삭제 또는 jobs.deploy.if / workflow-level 조건을 사용해 github.ref ==
'refs/heads/main'으로 제한). 참조할 식별자: deploy job, on.push.branches 목록, "step2" 브랜치
이름, 그리고 main(또는 refs/heads/main) 조건을 변경하세요.
Comment on lines
+88
to
+91
| <img | ||
| src="" | ||
| id="modal-image" | ||
| /> |
There was a problem hiding this comment.
빈 src 속성은 현재 페이지 URL로 요청을 발생시킵니다.
정적 분석 도구(HTMLHint)에서 지적한 대로, 빈 src="" 속성은 브라우저가 현재 페이지 URL로 HTTP 요청을 보내게 합니다. JavaScript에서 동적으로 설정할 예정이라면 placeholder 이미지나 data: URI를 사용하거나, 초기 렌더링 시 src 속성을 아예 제거하는 것을 고려하세요.
🧰 Tools
🪛 HTMLHint (1.9.2)
[error] 89-89: The attribute [ src ] of the tag [ img ] must have a value.
(src-not-empty)
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@index.html` around lines 88 - 91, The <img id="modal-image"> currently has an
empty src="" which causes the browser to request the current page; remove the
empty src attribute from the <img> element (or replace it with a harmless
placeholder/data URI) and set the actual image URL at runtime from your
JavaScript logic that targets document.getElementById("modal-image") so no
empty-request is made during initial render.
Comment on lines
+109
to
+113
| <img src="./public/images/star_empty.png" class="my-star-image" id="my-star-image-1" data-score="2" /> | ||
| <img src="./public/images/star_empty.png" class="my-star-image" id="my-star-image-2" data-score="4"/> | ||
| <img src="./public/images/star_empty.png" class="my-star-image" id="my-star-image-3" data-score="6"/> | ||
| <img src="./public/images/star_empty.png" class="my-star-image" id="my-star-image-4" data-score="8"/> | ||
| <img src="./public/images/star_empty.png" class="my-star-image" id="my-star-image-5" data-score="10"/> |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Description: 프로젝트 내 이미지 경로 구조 확인
# public/images와 images 디렉토리 존재 여부 확인
echo "=== 이미지 디렉토리 구조 ==="
fd -t d -d 3 'images'
# HTML에서 사용된 이미지 경로 패턴 확인
echo -e "\n=== HTML 내 이미지 경로 패턴 ==="
rg -o 'src="[^"]*images[^"]*"' index.html | sort | uniqRepository: woowacourse/javascript-movie-review
Length of output: 361
이미지 경로 불일치 - 통일 필요
별점 이미지는 ./public/images/star_empty.png 경로를 사용하지만, 다른 모든 이미지들(empty_icon.png, logo.png, modal_button_close.png, search.png, star_filled.png, woowacourse_logo.png)은 ./images/ 경로를 사용합니다. 프로젝트에 public/images/ 디렉토리만 존재하므로 ./images/ 경로는 404 오류를 유발할 수 있습니다. 모든 이미지 경로를 ./public/images/로 통일하거나 경로 구조를 재검토하세요.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@index.html` around lines 109 - 113, Image path usage is inconsistent: some
tags use ./images/ while others (including my-star-image-1..5) use
./public/images/, causing 404s; standardize all image src attributes to the
project's canonical path (use ./public/images/ throughout) and update any tags
(check elements with class "my-star-image" and ids my-star-image-1 to
my-star-image-5 as well as other image tags) so every src points to
./public/images/<filename> to make paths consistent.
Comment on lines
+10
to
11
| - [x] 브라우저의 화면 끝에 도달하면 20개의 검색과 관련된 영화 목록을 추가로 불러온다. | ||
| - [x] 불러올 영화 목록이 없으면 더보기 버튼이 사라진다. |
There was a problem hiding this comment.
무한 스크롤 설명과 ‘더보기 버튼’ 서술이 서로 충돌합니다.
체크리스트/시나리오에서 무한 스크롤을 설명하면서 동시에 더보기 버튼 소거를 검증하고 있어, 현재 동작 기준이 문서상 모호합니다. 한 기준으로 통일해 주세요.
Also applies to: 20-21, 135-140
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@README.md` around lines 10 - 11, The README currently contradicts itself by
describing infinite scroll while also testing removal of a "더보기" (load more)
button; pick one canonical UX (either infinite scroll or a load-more button),
update all related checklist items and scenario descriptions to consistently
reflect that choice, and remove or rewrite the conflicting lines that mention
the opposite behavior (update the entries currently at the spots referenced in
the comment including the occurrences around lines 10-11, 20-21 and 135-140) so
every checklist, scenario, and acceptance criterion consistently states the
chosen behavior.
Comment on lines
+98
to
+99
| | Endpoint | /3/movie/movie_id | | ||
| | 설명 | 사용자가 클릭한 영화에 대한 상세 정보를 가져오는 API입니다. | |
There was a problem hiding this comment.
Details endpoint의 path parameter 표기를 명확히 해주세요.
/3/movie/movie_id는 리터럴 경로처럼 읽힐 수 있어, 변수 파라미터 표기({movie_id} 등)로 명확히 적는 편이 좋습니다.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@README.md` around lines 98 - 99, The README's endpoint path
`/3/movie/movie_id` reads like a literal; change it to use a variable path
parameter such as `/3/movie/{movie_id}` (or `:movie_id` if following your style)
wherever the Endpoint is documented so it's clear this is a path parameter, and
update any accompanying description or examples that reference `movie_id` to
match the new parameter notation.
Comment on lines
+24
to
+28
| this.#modalDescriptionTitle = document.querySelector<HTMLTitleElement>(".modal-description-title"); | ||
| this.#modalDescriptionYear = document.querySelector<HTMLTitleElement>("#modal-description-year"); | ||
| this.#modalDescriptionGenre = document.querySelector<HTMLTitleElement>("#modal-description-genre"); | ||
| this.#modalRateNumber = document.querySelector<HTMLTitleElement>("#modal-rate-number"); | ||
| this.#modalDetailDescription = document.querySelector<HTMLTitleElement>("#modal-detail-description"); |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
rg -n -C1 'modal-description-title|modal-description-year|modal-description-genre|modal-rate-number|modal-detail-description' index.html src/view/modalView.tsRepository: woowacourse/javascript-movie-review
Length of output: 1745
DOM 선택자의 제네릭 타입이 실제 HTML 요소와 맞지 않습니다.
현재 코드는 querySelector<HTMLTitleElement>를 사용하고 있는데, 실제 HTML 요소들을 확인해보면:
.modal-description-title는<h2>요소#modal-description-year,#modal-description-genre,#modal-rate-number는<span>요소#modal-detail-description는<p>요소
(HTMLTitleElement는 <title> 태그용 DOM 인터페이스입니다)
각 요소가 실제로 어떤 HTML 태그인지 index.html에서 확인한 후, 각각에 맞는 타입(예: HTMLHeadingElement, HTMLSpanElement, HTMLParagraphElement 등)으로 수정하면 TypeScript의 타입 안전성이 개선되고 IDE 지원도 더 정확해질 것입니다.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/view/modalView.ts` around lines 24 - 28, The DOM generic types used with
querySelector are incorrect: replace querySelector<HTMLTitleElement> for the
fields `#modalDescriptionTitle`, `#modalDescriptionYear`, `#modalDescriptionGenre`,
`#modalRateNumber`, and `#modalDetailDescription` with the actual element types from
index.html (e.g., .modal-description-title -> HTMLHeadingElement,
`#modal-description-year` / `#modal-description-genre` / `#modal-rate-number` ->
HTMLSpanElement, `#modal-detail-description` -> HTMLParagraphElement), updating
the generics for the corresponding properties (`#modalDescriptionTitle`,
`#modalDescriptionYear`, `#modalDescriptionGenre`, `#modalRateNumber`,
`#modalDetailDescription`) so TypeScript reflects the real element interfaces and
preserves nullability checks after you confirm tag names in index.html.
Comment on lines
+82
to
+90
| if (this.#modalDescriptionGenre) { | ||
| this.#modalDescriptionGenre.innerText = data.genres | ||
| ? data.genres.map((item) => item.name).join(", ") | ||
| : "알 수 없음"; | ||
| }; | ||
|
|
||
| if (this.#modalRateNumber) { | ||
| this.#modalRateNumber.innerText = data.vote_average ? `${data.vote_average.toFixed(1)}` : "알 수 없음"; | ||
| } |
There was a problem hiding this comment.
falsy 체크 때문에 정상 값이 "알 수 없음"으로 바뀔 수 있습니다.
vote_average가 0이면 현재 분기에서 누락값처럼 처리됩니다. genres도 빈 배열이면 빈 문자열만 남습니다. 값의 부재와 유효한 falsy 값을 분리해서 처리해 주세요.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/view/modalView.ts` around lines 82 - 90, The current falsy checks cause
valid values (like vote_average = 0 or genres = []) to be treated as missing;
update the logic around this.#modalRateNumber and this.#modalDescriptionGenre to
distinguish absent (null/undefined) from valid falsy values: for
data.vote_average use an explicit null/undefined check (or Number.isFinite) so 0
is rendered as "0.0" and only null/undefined yield "알 수 없음", and for data.genres
ensure you check for existence and non-empty array (e.g., data.genres &&
data.genres.length > 0) before mapping/joining, otherwise set "알 수 없음".
| #myStarScore; | ||
| #evaluation: Record<number, string> = { | ||
| 0: "나의 별점을 눌러보세요.", | ||
| 2: "최악이예요", |
There was a problem hiding this comment.
문구 오타가 그대로 노출됩니다.
최악이예요는 최악이에요가 자연스럽습니다.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/view/myStarRatingView.ts` at line 10, In the myStarRatingView file update
the displayed label string "최악이예요" to the correct Korean spelling "최악이에요";
locate the star-rating labels array or constant inside myStarRatingView (the
entry currently containing "최악이예요") and replace that single string so the UI
shows "최악이에요".
Comment on lines
+25
to
+31
| bindHandleMyStarClick(handler: (score: number) => void) { | ||
| this.#myStarImageArray.forEach((item) => { | ||
| item?.addEventListener("click", () => { | ||
| const score = Number(item.dataset.score); | ||
| this.renderRating( score); | ||
| handler(score); | ||
| }); |
There was a problem hiding this comment.
별점 입력이 마우스에만 묶여 있습니다.
지금은 click만 처리해서 키보드 사용자나 보조기기 사용자는 평점을 줄 수 없습니다. 이 상호작용이 포커스 가능하고 Enter/Space로도 동작하는지 한번 확인해 주세요.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/view/myStarRatingView.ts` around lines 25 - 31, bindHandleMyStarClick
currently only listens for "click" on elements in `#myStarImageArray` so keyboard
and assistive tech users cannot activate stars; make each star focusable (ensure
tabindex="0" and optional role="button") and add a keydown listener alongside
the click listener that detects Enter (KeyEnter) and Space (Space/ ) presses,
prevents default for Space, converts the element's dataset.score to Number,
calls this.renderRating(score) and then handler(score) so keyboard activation
behaves identically to mouse clicks. Ensure you attach these changes to the same
elements referenced in bindHandleMyStarClick/#myStarImageArray.
Comment on lines
+20
to
+24
| vote_average: number; | ||
| poster_path: string; | ||
| overview: string; | ||
| vote_average: number; | ||
| }; |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
python - <<'PY'
from pathlib import Path
import re
from collections import Counter
path = Path("types/api.d.ts")
text = path.read_text(encoding="utf-8")
m = re.search(r'interface\s+MovieModalData\s*{(?P<body>[^}]*)}', text, re.S)
if not m:
raise SystemExit("MovieModalData interface not found")
fields = []
for line in m.group("body").splitlines():
mm = re.match(r'\s*([A-Za-z_]\w*)\s*:', line)
if mm:
fields.append(mm.group(1))
dups = [k for k, v in Counter(fields).items() if v > 1]
print("Duplicate fields in MovieModalData:", dups)
PYRepository: woowacourse/javascript-movie-review
Length of output: 131
MovieModalData 인터페이스에서 vote_average 필드가 중복 선언되어 있습니다.
Line 20과 Line 23에 동일한 vote_average: number;이 선언되어 있습니다. 인터페이스에서 같은 이름의 프로퍼티가 여러 번 선언되는 것은 TypeScript 컴파일 오류를 발생시킵니다. 두 선언 중 어느 것이 올바른 의도인지 확인하신 후 중복을 제거해주세요.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@types/api.d.ts` around lines 20 - 24, The MovieModalData interface declares
vote_average twice, causing a TypeScript duplicate-property error; open the
MovieModalData definition and remove the redundant vote_average: number; so the
interface contains a single vote_average field (ensure you keep any intended
other properties like poster_path and overview unchanged); if one of the
duplicates was meant to be a different property, rename it to the correct
identifier instead of deleting.
JuHyeong424
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
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
🎯 미션 소개
🕵️ 셀프 리뷰(Self-Review)
제출 전 체크 리스트
리뷰 요청 & 논의하고 싶은 내용
1) 이번 단계에서 가장 많이 고민했던 문제와 해결 과정에서 배운 점
1. 무한 스크롤 설정 시 비동기 타이밍과 상태 제어 (Race Condition)
무한 스크롤(Intersection Observer)을 구현하면서 가장 크게 고민했던 부분은 예외 상황에서의 옵저버 레이스 컨디션(Race Condition) 통제였습니다. 초기 데이터 로딩에 실패했을 때, 화면에 렌더링 된 리스트가 없어 뷰포트가 짧아지면서 바닥에 심어둔 감지 센서(Observer)가 즉시 노출되는 문제가 발생했습니다. 이로 인해 에러가 발생하자마자 옵저버가 다음 페이지(page=2)를 연속으로 요청하는 비동기 폭주 현상을 겪었습니다.
이를 해결하기 위해 에러 발생 시 단순히 UI만 에러 화면으로 변경하는 것에 그치지 않고,
infiniteScrollView.disconnect()를 호출해 센서의 동작을 강제로 중단시키고 데이터 복구 성공 시에만 다시 연결(observe)하는 방어 로직을 추가했습니다. 이를 통해 비동기 이벤트의 라이프사이클을 안전하게 제어하는 방법을 깊이 체감할 수 있었습니다.2. MVC 패턴을 활용한
다시 시도하기버튼의 라우팅에러 화면에서
다시 시도하기버튼을 구현할 때, 초기에는 단일 이벤트에 무조건 무한 스크롤 함수(handleLoadMore)를 매핑하는 실수를 범했습니다. 이로 인해 1페이지(초기 화면)에서 네트워크 에러가 발생한 후 버튼을 누르면, 1페이지를 재요청하는 것이 아니라 2페이지를 요청해버려 테스트가 실패하고 데이터가 꼬이는 버그를 발견했습니다.이 문제를 해결하는 과정에서 MVC 패턴을 사용하였습니다. View의 버튼 클릭 이벤트에 단일 함수를 코딩하는 대신,
Controller(main.ts)에서 Model의 현재 상태(movieModel.page === 1및isSearchMode여부)를 평가하여 알맞은 초기화 함수(handleHome,handleSearch)나 추가 로드 함수(handleLoadMore)로 똑똑하게 분기 처리(Routing)하도록 리팩토링했습니다. 컨트롤러가 모델의 상태를 문맥(Context)으로 삼아 흐름을 제어하는 구조가 왜 필요한지 알게 되었습니다.3. Cypress E2E 테스트 작성
네트워크 에러와 복구 과정을 Cypress로 테스트하면서
cy.intercept의 우선순위 덮어쓰기(forceNetworkError후 성공Mocking 덮어쓰기)와cy.wait()의 타임아웃 문제를 집중적으로 다루었습니다.특히 타임아웃 에러를 마주했을 때, 처음에는 테스트 코드의 문법 오류라고 생각했지만, 디버깅을 진행하며 오히려 테스트 코드가 프로덕션 코드에 숨어있던 논리적 허점(옵저버 폭주, 엉뚱한 페이지 요청)을 정확히 짚어내고 있었다는 사실을 깨달았습니다. 성공 케이스를 검증할 뿐만 아니라, 실패 상태를 명확히 세팅(beforeEach)하고 액션을 취한 뒤 대기(wait)하는 정석적인 테스트 흐름을 익혔습니다. E2E 테스트 코드를 작성하고 애플리케이션 아키텍처의 결함을 찾아내면서 테스트 코드가 왜 필요한지 알게 되었습니다.
2) 이번 리뷰를 통해 논의하고 싶은 부분
1. 이벤트 라우팅과 MVC 역할 분담
에러 화면의
다시 시도하기버튼을 구현하면서, 단일 함수(handleLoadMore) 하드코딩으로 인한 버그를 해결하기 위해 컨트롤러(main.ts)에서movieModel의 상태(page,mode)를 읽어와 알맞은 초기화 함수로 라우팅하도록 리팩토링했습니다.이 과정에서 "컨트롤러가 모델의 세부 상태를 직접 읽어서 분기 처리하는 현재 방식이 최선일까?"에 대한 고민이 생겼습니다. 현업에서는 이런 예외 복구 상황을 다룰 때, 컨트롤러에서 직접 라우팅 분기를 태우는 방식을 선호하는지, 아니면
View컴포넌트 자체가 자신이 어떤 컨텍스트(초기 로딩 vs 추가 로딩)에서 에러를 띄웠는지 기억하고 있다가 특정한 이벤트를 발행하는 방식을 더 권장하는지 리뷰어분들의 의견을 듣고 싶습니다.2. Intersection Observer 레이스 컨디션을 막는 UI/UX적 접근
초기 로딩 실패 시 뷰포트가 짧아져
Observer가 즉시 노출되고 2페이지를 강제 요청하는 레이스 컨디션을 방지하기 위해,infiniteScrollView.disconnect()를 명시적으로 호출하여 센서를 끄는 논리적인 방어 로드를 추가했습니다.하지만 논리적인 제어 외에도, UI/UX 적으로 이 문제를 해결하는 방법이 있는지 궁금합니다. 예를 들어, 에러 컨테이너 자체의 최소 높이(
min-height)를 화면 전체 크기(100vh)로 강제하여 애초에 바닥 센서가 화면에 들어오지 못하게 막는 방식이 실무에서 자주 쓰이는 패턴인지, 아니면 현재 구현한 JS 로직 기반의 제어가 더 안전한 방식인지 논의해 보고 싶습니다.3. 복잡한 E2E 테스트(Cypress)의 상태 관리 전략
이번 테스트 코드에서는
beforeEach에서forceNetworkError로 실패 환경을 먼저 세팅하고,it블록 안에서cy.mock...커스텀 커맨드로 다시 성공 상태를 덮어씌워다시 시도하기성공 시나리오를 검증했습니다.이러한 방식이 동작은 잘 하지만, 애플리케이션이 커지고 복구 시나리오가 많아질 경우
intercept덮어쓰기 관리가 복잡해질 것 같다는 우려가 듭니다. 테스트 시나리오가 복잡해질 때, 현업에서는 모킹 상태 관리를 어떻게 추상화하고 유지보수하는지,beforeEach를 쪼개서 테스트 파일을 분리하는 것을 더 선호하는지 팁을 얻고 싶습니다.4. 클래스(모듈) 분리의 적절성과 책임 할당에 대한 고민
이번 미션을 진행하면서 MVC 패턴을 기반으로
View(movieListView, bannerView, infiniteScrollView, searchView, modalView, myStarRatingView),Model(movieModel, currentMovieModel),Controller(main.ts, API 서비스 함수들)등으로 책임을 나누어 구현했습니다. 특히 무한 스크롤 로직이나 에러 처리 로직을infiniteScrollView와errorMovieList라는 별도의 객체(모듈)로 분리하여 응집도를 높이고자 시도했습니다.하지만 개발을 진행하면서 "지금 내가 나눈 이 기준이 너무 잘게 쪼개진 것은 아닐까?" 혹은 "Controller가 너무 많은 다른 모듈에 의존하고 있는 것은 아닐까?" 하는 결합도 측면의 고민이 생겼습니다.
현재의 클래스 분리 수준과 구조가 실무적인 관점에서 적절한 형태인지 궁금합니다. 비슷한 규모의 요구사항을 구현할 때, 현업에서는 도메인을 더 세밀하게 쪼개는 편인지, 아니면 유지보수성을 위해 조금 더 큰 단위로 묶어서 관리하는 것을 선호하는지 리뷰어분들의 아키텍처 설계 방법을 듣고 싶습니다.
✅ 리뷰어 체크 포인트