[2단계 - 상세 정보 & UI/UX 개선하기] 이현 미션 제출합니다. #295
[2단계 - 상세 정보 & UI/UX 개선하기] 이현 미션 제출합니다. #295jebiyeon02 wants to merge 38 commits intowoowacourse:jebiyeon02from
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이 프로젝트는 영화 리뷰 애플리케이션의 주요 아키텍처 리팩토링입니다. "더 보기" 버튼 기반 페이지네이션을 무한 스크롤로 변경하고, 영화 상세정보를 모달 다이얼로그로 표시하며, 5단계 별점 평가 기능을 추가했습니다. fetchMovieDetail API 함수를 신규 추가하고, MovieDetailView 및 RatingView 클래스를 구현했습니다. App 클래스에 스크롤 이벤트 리스너와 모달/별점 이벤트 핸들러를 통합하며, 기존 MoreMovieView를 제거했습니다. 모달은 localStorage에 별점을 저장하고, 다양한 E2E 및 단위 테스트가 추가되었습니다. CSS 및 레이아웃이 재구성되어 반응형 설계가 개선되었습니다. Estimated code review effort🎯 3 (Moderate) | ⏱️ ~35 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: 16
🧹 Nitpick comments (2)
src/constants/raintg.ts (1)
1-31: 파일명 오탈자(raintg)는 조기에 정리하는 편이 좋습니다.상수는 괜찮지만 파일명은
rating으로 맞춰두면 import 실수와 탐색 비용을 줄일 수 있습니다.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/constants/raintg.ts` around lines 1 - 31, Rename the file from "raintg" to "rating" to fix the typo and update all imports to the new filename; ensure the exported constant RATING stays unchanged and is referenced correctly wherever imported (search for any import of "raintg" and replace with "rating"), then run the test/build to confirm no missing-module errors; also update any related documentation or barrel exports that referenced the old filename.src/App.ts (1)
49-57:App에 화면 오케스트레이션 책임이 과하게 모였습니다.지금은 검색, 무한 스크롤, 상세 조회, 평점 저장까지 모두 여기서 묶여 있어서 기능이 더 붙으면 테스트와 상태 추적이 빠르게 어려워집니다. 스크롤 페이징, 상세 모달, 평점 저장 흐름을 별도 coordinator/service 단위로 나누고
App은 조합만 맡기면 확장성이 훨씬 좋아집니다. 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 `@src/App.ts` around lines 49 - 57, The App class currently centralizes orchestration in `#bindAllEvents` (including `#bindWindowEvent` and handlers wired to this.#views.search, logo, movieList, movieDetail, topRated, rating); split responsibilities by extracting scrolling/paging logic into a ScrollCoordinator (move infinite-scroll handlers and `#bindWindowEvent` there), move detail-modal/open-close and movie detail retrieval into a ModalCoordinator (bind movieList/topRated -> openDetail and movieDetail.close handling), and move rating persistence into a RatingService (bind rating view events to this service). In App.#bindAllEvents simply instantiate and register these coordinators/services with the relevant view instances (e.g., ScrollCoordinator.register(this.#views.movieList, this.#views.search), ModalCoordinator.register(this.#views.movieList, this.#views.topRated, this.#views.movieDetail), RatingService.register(this.#views.rating)) so App only composes collaborators and no longer owns the detailed event handlers.
🤖 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`:
- Around line 97-143: The <dialog class="modal"> block and the module script tag
(script type="module" src="./src/main.ts") are placed after the closing </html>
and must be moved inside the document body; relocate the entire dialog
(including button#closeModal, .modal-container, .modal-poster-image,
.modal-movie-title, .review-star elements, etc.) so it sits before </body> and
ensure the <script type="module" src="./src/main.ts"> tag is also placed just
before </body> (or at the end of the body) to restore correct document structure
and parsing.
- Around line 98-100: 버튼에 접근 가능한 이름이 없으니 closeModal 아이디(또는 close-modal 클래스)를 가진
버튼 요소에 aria-label="닫기"를 추가하고, 아이콘 이미지가 장식용이면 해당 img 태그의 alt 속성을 빈 문자열 alt=""로
변경해 시각 보조기기에서 불필요한 중복 설명이 나오지 않도록 수정하세요.
- Around line 122-126: The five clickable <img> elements with classes
review-star / star-first, star-second, star-third, star-fourth, star-fifth are
not keyboard-accessible; replace each <img> with a focusable control (e.g.,
<button> wrapping an <img> or using CSS background) that preserves the
data-score attribute, add an accessible name via aria-label (e.g., "Rate 2
stars"), ensure the control is focusable (native button is fine), and wire
keyboard handlers so Enter and Space trigger the same handler used by click;
also keep the existing star classes to locate styling and attach event listeners
used by your rating logic.
In `@README_step1.md`:
- Line 41: 문서의 "더 이상 보여 줄 영화가 없으면 더
보기 버튼이 사라진다." 문장에 불필요한 연속 공백이 들어가 있으니 해당 문자열(더 이상 보여
줄)을 찾아 연속 공백을 단일 공백 또는 적절한 문장 형태("더 이상 보여줄")로 정리하고 문장 전체를 "더 이상 보여줄 영화가 없으면 더 보기
버튼이 사라진다."처럼 자연스럽게 수정해 README_step1.md 내 동일 표현이 다른 곳에 있으면 동일하게 공백을 정규화하세요.
In `@README.md`:
- Line 42: Replace the misspelled rating phrase "최악이예요" with the correct wording
"최악이에요" so the README's rating text matches the UI/constants; search for the
exact string "최악이예요" and update it to "최악이에요" wherever it appears (e.g., the
rating line currently showing "2점: 최악이예요").
- Around line 85-89: The README contains a conflicting score-to-star mapping:
the earlier rule stating "별 1개 = 2점" conflicts with the later bullets that list
"1점/2점/3점/4점/5점". Update the scenario bullets to use the same scale as the
earlier rule—either convert the bullets to star-count phrasing ("별 1개", "별 2개",
... in place of "1점", "2점", ...) or convert them to the 2/4/6/8/10 point values
that correspond to "별 1개 = 2점"; ensure the text around the scoring examples (the
list items currently showing 1~5) matches the selected scheme throughout the
README so the mapping is unambiguous.
In `@src/api/fetchMovies.ts`:
- Line 10: The region query parameter in the API URL string in
src/api/fetchMovies.ts is using the wrong format (`region=ko-KR`) for the
/movie/popular request; update the URL template to use the ISO 3166-1 alpha-2
code (`region=KR`) to match TMDB spec and match the /search/movie usage, e.g.,
modify the string literal in the function that builds the popular movies request
so `region=ko-KR` becomes `region=KR`.
In `@src/App.ts`:
- Around line 175-183: The movie detail handler `#movieDetailEventHandler`
currently lets fetchMovieDetail(movieId) errors propagate and always manipulates
the view; wrap the fetch in a try/catch so you only call
this.#views.movieDetail.show(), this.#views.movieDetail.renderData(...) and
this.#views.rating.renderByRatingValue(...) after a successful await, and in the
catch branch call an appropriate error/empty-state renderer on the movieDetail
view (or show a toast) to surface network/404 errors and avoid leaving the modal
in an inconsistent state; ensure you still read localStorage (rating-{movieId})
only when rendering succeeds, and log or surface the caught error for debugging.
- Around line 84-131: The scroll handler in `#bindWindowEvent` pre-increments page
counters (this.#state.searchMoviePage / this.#state.popularMoviePage) before the
delayed request and has no in-flight guard, so multiple timers can schedule
concurrent requests or a failed request leaves page state advanced and causes
skipped pages; fix by introducing an in-flight flag (e.g., this.#isFetching)
checked at the top of the scroll handler to prevent scheduling while a request
is ongoing, do NOT pre-increment the page counters when building requestMovies
(call fetchSearchedMovies(this.#state.searchMoviePage + 1, ...) or
fetchPopularMovies(this.#state.popularMoviePage + 1) without mutating state),
set this.#isFetching = true just before awaiting requestMovies inside the
setTimeout, on success update this.#state.searchMoviePage /
this.#state.popularMoviePage from nowPage and total* from totalPages, and in
finally set this.#isFetching = false and remove skeletons via
this.#views.movieList.removeAllSkeletons so failures don't leave state
inconsistent; also ensure the scroll handler returns early if this.#isFetching
is true to avoid queuing multiple timers.
In `@src/styles/main.css`:
- Around line 7-9: 파일의 반응형 문제가 html { min-width: 250px }만으로 해결되지 않는 이유는 동일 CSS
파일의 `#wrap에` 여전히 min-width: 1440px가 남아있기 때문입니다; src/styles/main.css에서 `#wrap` 규칙을 찾아
min-width: 1440px를 제거하거나 반응형으로 바꿔(예: max-width, width:100% 및 적절한
padding/box-sizing 사용) 작은 화면에서 오버플로를 일으키지 않도록 조정하고, 필요하면 미디어 쿼리로 `#wrap의` 레이아웃을
1440px 이상일 때만 고정 폭으로 적용하도록 변경하세요.
In `@src/styles/modal.css`:
- Around line 124-194: There are stray empty lines that violate the stylelint
rule declaration-empty-line-before; remove the blank lines before property
blocks inside the media queries so declarations start immediately under their
selector—specifically clean up the extra empty lines inside the .modal block
(mobile and tablet media queries), and the nearby selector blocks such as
.modal-image, .modal-description and .overview-container so that no empty line
precedes the first declaration for each selector.
In `@src/styles/thumbnail.css`:
- Around line 28-31: The current .item:focus rule removes the browser focus
outline (outline: none) which harms keyboard accessibility; instead keep a
visible focus indicator by removing outline: none from .item:focus and add a
clear, high-contrast focus style for .item:focus-visible (or
.item:focus:not(:focus-visible) logic) that preserves the scale transform while
applying a visible ring/outline or box-shadow; update the rules around
.item:focus and .item:focus-visible so keyboard focus shows a pronounced,
high-contrast indicator while pointer-focused or non-visible focus can remain
visually subtle.
In `@src/View/MovieListView.ts`:
- Around line 27-39: In MovieListView.bindEvent change the synchronous call to
the async handler so returned Promises are awaited and errors surface: await the
result of handler(Number(movieId)) (matching TopRatedView.bindEvent behavior)
and validate the parsed id before calling (check Number(movieId) is not NaN,
otherwise throw or return with a clear error); update references inside
bindEvent (the click listener, item.getAttribute("data-movie-id"), and handler)
to use the await pattern so async `#movieDetailEventHandler` exceptions are not
lost.
In `@src/View/RatingView.ts`:
- Around line 60-70: bindEvent currently only listens for clicks which excludes
keyboard users; update bindEvent (and the setup for .review-star) to make stars
keyboard-accessible: when initializing (inside bindEvent or a small helper)
query all elements matching ".review-star" and set tabindex="0" and
role="button" so they can be focused/announced, then add a keydown listener on
this.#dom.container (or the same selector) that finds closest ".review-star"
like the click handler and on Enter/Space (keydown codes) prevents default and
reads the data-score into ratingValue and calls handler(ratingValue); keep the
existing click logic unchanged but reuse the same closest(".review-star") +
getAttribute("data-score") flow so both mouse and keyboard path use the same
validation and error handling.
In `@src/View/TopRatedView.ts`:
- Around line 27-33: In TopRatedView's click handler (the block using
this.#dom.title and calling handler), don't throw when data-movie-id is missing;
instead early-return and optionally log or show a user-friendly message; also
validate the converted id by checking Number(movieId) for NaN before calling
handler, and only call await handler(parsedId) when parsedId is a finite number.
In `@src/View/utils.ts`:
- Around line 11-14: getYearFromDate currently returns new
Date(dateString).getFullYear() without validation so an invalid release_date can
produce NaN and leak "NaN" into the DOM; update getYearFromDate to validate the
parsed date (e.g., use Date.parse or check isNaN(date.getTime())) and return a
safe fallback (empty string or null) when invalid, and adjust callers (e.g.,
MovieDetailView's usage of release_date) to handle the fallback when rendering
to the DOM. Ensure the function signature/documentation indicates the possible
fallback return and that MovieDetailView checks for that value before inserting
text into the DOM.
---
Nitpick comments:
In `@src/App.ts`:
- Around line 49-57: The App class currently centralizes orchestration in
`#bindAllEvents` (including `#bindWindowEvent` and handlers wired to
this.#views.search, logo, movieList, movieDetail, topRated, rating); split
responsibilities by extracting scrolling/paging logic into a ScrollCoordinator
(move infinite-scroll handlers and `#bindWindowEvent` there), move
detail-modal/open-close and movie detail retrieval into a ModalCoordinator (bind
movieList/topRated -> openDetail and movieDetail.close handling), and move
rating persistence into a RatingService (bind rating view events to this
service). In App.#bindAllEvents simply instantiate and register these
coordinators/services with the relevant view instances (e.g.,
ScrollCoordinator.register(this.#views.movieList, this.#views.search),
ModalCoordinator.register(this.#views.movieList, this.#views.topRated,
this.#views.movieDetail), RatingService.register(this.#views.rating)) so App
only composes collaborators and no longer owns the detailed event handlers.
In `@src/constants/raintg.ts`:
- Around line 1-31: Rename the file from "raintg" to "rating" to fix the typo
and update all imports to the new filename; ensure the exported constant RATING
stays unchanged and is referenced correctly wherever imported (search for any
import of "raintg" and replace with "rating"), then run the test/build to
confirm no missing-module errors; also update any related documentation or
barrel exports that referenced the old filename.
🪄 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: c5868df8-82c9-4680-ac7f-f03976f5f452
📒 Files selected for processing (27)
README.mdREADME_step1.md__tests__/movie.test.tscypress/e2e/main.cy.tscypress/e2e/movie-detail.cy.tscypress/e2e/rating.cy.tscypress/e2e/search.cy.tsindex.htmlsrc/App.tssrc/View/LogoView.tssrc/View/MoreMovieView.tssrc/View/MovieDetailView.tssrc/View/MovieListView.tssrc/View/RatingView.tssrc/View/TopRatedView.tssrc/View/utils.tssrc/api/fetchMovieDetail.tssrc/api/fetchMovies.tssrc/constants/raintg.tssrc/styles/colors.csssrc/styles/main.csssrc/styles/modal.csssrc/styles/thumbnail.csssrc/thumnailManager.tstemplates/index.htmltemplates/modal.htmltypes/movie.ts
💤 Files with no reviewable changes (3)
- templates/modal.html
- src/View/MoreMovieView.ts
- templates/index.html
| <dialog class="modal"> | ||
| <button class="close-modal" id="closeModal"> | ||
| <img src="./src/assets/modal_button_close.png" /> | ||
| </button> | ||
| <div class="modal-container"> | ||
| <div class="modal-image"> | ||
| <img | ||
| class="modal-poster-image" | ||
| /> | ||
| </div> | ||
| <div class="modal-description"> | ||
| <h2 class="modal-movie-title">-</h2> | ||
| <p class="category"> | ||
| - | ||
| </p> | ||
| <p class="rate"> | ||
| <img src="./src/assets/star_filled.png" class="star" /> | ||
| <span class="detail-rate-value">7.7</span | ||
| > | ||
| </p> | ||
| <hr /> | ||
| <div class="review-container"> | ||
| <h3 class="review-title">내 별점</h3> | ||
| <div class="review-star-container"> | ||
| <span> | ||
| <img class="review-star star-first" data-score="2" src="./src/assets/star_empty.png"/> | ||
| <img class="review-star star-second" data-score="4" src="./src/assets/star_empty.png"/> | ||
| <img class="review-star star-third" data-score="6" src="./src/assets/star_empty.png"/> | ||
| <img class="review-star star-fourth" data-score="8" src="./src/assets/star_empty.png"/> | ||
| <img class="review-star star-fifth " data-score="10" src="./src/assets/star_empty.png"/> | ||
| </span> | ||
| <span class="review-text">별점 없음</span><span class="review-score">(0/10)</span> | ||
|
|
||
| </div> | ||
| </div> | ||
| <hr /> | ||
| <div class="overview-container"> | ||
| <h3>줄거리</h3> | ||
| <p class="overview"> | ||
| </p> | ||
| </div> | ||
| </div> | ||
| </div> | ||
| </dialog> | ||
| </html> | ||
|
|
||
| <script type="module" src="./src/main.ts"></script> |
There was a problem hiding this comment.
dialog와 모듈 스크립트가 body/html 바깥에 있어 문서 구조가 깨집니다.
Line 97 이후와 Line 143은 </body>/</html> 내부로 이동해야 파서 보정에 의존하지 않고 안정적으로 동작합니다.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@index.html` around lines 97 - 143, The <dialog class="modal"> block and the
module script tag (script type="module" src="./src/main.ts") are placed after
the closing </html> and must be moved inside the document body; relocate the
entire dialog (including button#closeModal, .modal-container,
.modal-poster-image, .modal-movie-title, .review-star elements, etc.) so it sits
before </body> and ensure the <script type="module" src="./src/main.ts"> tag is
also placed just before </body> (or at the end of the body) to restore correct
document structure and parsing.
| <button class="close-modal" id="closeModal"> | ||
| <img src="./src/assets/modal_button_close.png" /> | ||
| </button> |
There was a problem hiding this comment.
닫기 버튼의 접근 가능한 이름이 없습니다.
Line 98의 버튼에 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, 버튼에 접근 가능한 이름이 없으니 closeModal 아이디(또는
close-modal 클래스)를 가진 버튼 요소에 aria-label="닫기"를 추가하고, 아이콘 이미지가 장식용이면 해당 img 태그의 alt
속성을 빈 문자열 alt=""로 변경해 시각 보조기기에서 불필요한 중복 설명이 나오지 않도록 수정하세요.
| <img class="review-star star-first" data-score="2" src="./src/assets/star_empty.png"/> | ||
| <img class="review-star star-second" data-score="4" src="./src/assets/star_empty.png"/> | ||
| <img class="review-star star-third" data-score="6" src="./src/assets/star_empty.png"/> | ||
| <img class="review-star star-fourth" data-score="8" src="./src/assets/star_empty.png"/> | ||
| <img class="review-star star-fifth " data-score="10" src="./src/assets/star_empty.png"/> |
There was a problem hiding this comment.
별점 인터랙션이 키보드 접근 불가능한 구조입니다.
Line 122-126의 클릭 가능한 img 대신 포커스 가능한 컨트롤(예: button)을 사용하고, 키보드 이벤트까지 지원해 주세요.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@index.html` around lines 122 - 126, The five clickable <img> elements with
classes review-star / star-first, star-second, star-third, star-fourth,
star-fifth are not keyboard-accessible; replace each <img> with a focusable
control (e.g., <button> wrapping an <img> or using CSS background) that
preserves the data-score attribute, add an accessible name via aria-label (e.g.,
"Rate 2 stars"), ensure the control is focusable (native button is fine), and
wire keyboard handlers so Enter and Space trigger the same handler used by
click; also keep the existing star classes to locate styling and attach event
listeners used by your rating logic.
| 1. 배너에 첫 번째 인기 영화의 정보가 표시된다. | ||
| 2. 최초 진입 시 인기 영화 최대 20개가 표시된다. | ||
| 3. 더 보기 버튼을 누르면 최대 20개가 추가된다. | ||
| 4. 더 이상 보여 줄 영화가 없으면 더 보기 버튼이 사라진다. |
There was a problem hiding this comment.
문장 내 공백이 비정상적으로 들어가 있습니다.
Line 41의 더 이상 보여 줄은 문서 가독성을 해치므로 공백 정리가 필요합니다.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@README_step1.md` at line 41, 문서의 "더 이상 보여
줄 영화가 없으면 더 보기 버튼이 사라진다." 문장에 불필요한 연속 공백이 들어가 있으니 해당 문자열(더 이상 보여
줄)을 찾아 연속 공백을 단일 공백 또는 적절한 문장 형태("더 이상 보여줄")로 정리하고 문장 전체를 "더 이상 보여줄 영화가 없으면 더 보기
버튼이 사라진다."처럼 자연스럽게 수정해 README_step1.md 내 동일 표현이 다른 곳에 있으면 동일하게 공백을 정규화하세요.
| - 영화 상세정보 내부에서 별을 클릭하여 별점을 매긴다. | ||
| - 별은 총 5개, 개당 2점이고 1점 단위는 고려하지 않는다. | ||
| - 별점별 나타나는 텍스트 | ||
| - 2점: 최악이예요 |
There was a problem hiding this comment.
평점 문구 오탈자가 있습니다.
Line 42의 최악이예요는 실제 UI/상수 표현과 맞추려면 최악이에요로 통일하는 편이 좋습니다.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@README.md` at line 42, Replace the misspelled rating phrase "최악이예요" with the
correct wording "최악이에요" so the README's rating text matches the UI/constants;
search for the exact string "최악이예요" and update it to "최악이에요" wherever it appears
(e.g., the rating line currently showing "2점: 최악이예요").
| .item:focus { | ||
| scale: 105%; | ||
| outline: none; | ||
| } |
There was a problem hiding this comment.
포커스 표시를 지우면 키보드 탐색성이 크게 떨어집니다.
outline: none 뒤에 scale만 두면 현재 포커스 위치가 충분히 드러나지 않습니다. 접근성 개선 PR이라면 :focus-visible에 대비가 분명한 ring/outline을 남기는 쪽이 안전합니다.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/styles/thumbnail.css` around lines 28 - 31, The current .item:focus rule
removes the browser focus outline (outline: none) which harms keyboard
accessibility; instead keep a visible focus indicator by removing outline: none
from .item:focus and add a clear, high-contrast focus style for
.item:focus-visible (or .item:focus:not(:focus-visible) logic) that preserves
the scale transform while applying a visible ring/outline or box-shadow; update
the rules around .item:focus and .item:focus-visible so keyboard focus shows a
pronounced, high-contrast indicator while pointer-focused or non-visible focus
can remain visually subtle.
src/View/MovieListView.ts
Outdated
| bindEvent(handler: (movieId: number) => void) { | ||
| this.#dom.list.addEventListener("click", (e: MouseEvent) => { | ||
| let item = (e.target as HTMLElement).closest(".thumbnail-container"); | ||
|
|
||
| if (!item) return; | ||
|
|
||
| const movieId = item.getAttribute("data-movie-id"); | ||
| if (!movieId) { | ||
| throw new Error("영화 id를 찾을 수 없습니다."); | ||
| } | ||
|
|
||
| handler(Number(movieId)); | ||
| }); |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# bindEvent 시그니처와 async 핸들러 연결 지점을 찾아
# Promise 반환 무시 패턴 여부를 확인합니다.
rg -nP --type=ts 'bindEvent\(handler:\s*\(movieId:\s*number\)\s*=>\s*void\)'
rg -nP --type=ts '#movieDetailEventHandler\s*=\s*async'
rg -nP --type=ts 'movieList\.bindEvent\(|topRated\.bindEvent\('Repository: woowacourse/javascript-movie-review
Length of output: 471
🏁 Script executed:
# bindEvent 전체 구현 확인
cat -n src/View/MovieListView.ts | head -50
# App.ts에서 `#movieDetailEventHandler` 구현 확인
rg -A 10 '#movieDetailEventHandler\s*=\s*async' src/App.ts
# TopRatedView.ts의 bindEvent도 확인
cat -n src/View/TopRatedView.ts | head -50Repository: woowacourse/javascript-movie-review
Length of output: 3982
MovieListView의 비동기 핸들러가 대기되지 않아 예외가 누락될 수 있습니다.
38번 줄에서 handler(Number(movieId))를 호출하지만 반환된 Promise를 await하지 않습니다. 반면 TopRatedView의 동일한 메서드(32번 줄)에서는 await handler(Number(movieId))로 올바르게 처리하고 있습니다.
현재 App.ts의 #movieDetailEventHandler는 async 함수이므로 Promise를 반환하는데, MovieListView에서 이를 무시하면 핸들러의 오류가 조용히 누락될 수 있습니다. TopRatedView의 구현 방식을 참고하여 MovieListView도 동일하게 수정해 주세요. 추가로 Number(movieId) 결과가 NaN이 될 가능성도 함께 검토해 주세요.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/View/MovieListView.ts` around lines 27 - 39, In MovieListView.bindEvent
change the synchronous call to the async handler so returned Promises are
awaited and errors surface: await the result of handler(Number(movieId))
(matching TopRatedView.bindEvent behavior) and validate the parsed id before
calling (check Number(movieId) is not NaN, otherwise throw or return with a
clear error); update references inside bindEvent (the click listener,
item.getAttribute("data-movie-id"), and handler) to use the await pattern so
async `#movieDetailEventHandler` exceptions are not lost.
src/View/RatingView.ts
Outdated
| bindEvent(handler: (ratingValue: string) => void) { | ||
| this.#dom.container.addEventListener("click", (e: MouseEvent) => { | ||
| let star = (e.target as HTMLElement).closest(".review-star"); | ||
|
|
||
| if (!star) return; | ||
| const ratingValue = star.getAttribute("data-score"); | ||
| if (!ratingValue) { | ||
| throw new Error("올바른 별점이 설정되지 않았습니다."); | ||
| } | ||
| handler(ratingValue); | ||
| }); |
There was a problem hiding this comment.
별점 컨트롤이 키보드로는 동작하지 않습니다.
현재 bindEvent()는 click만 처리하고 있고, 마크업의 .review-star는 <img>라 포커스/활성화가 되지 않습니다. 이 상태면 키보드 사용자와 일부 보조기기 사용자는 평점을 남길 수 없습니다.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/View/RatingView.ts` around lines 60 - 70, bindEvent currently only
listens for clicks which excludes keyboard users; update bindEvent (and the
setup for .review-star) to make stars keyboard-accessible: when initializing
(inside bindEvent or a small helper) query all elements matching ".review-star"
and set tabindex="0" and role="button" so they can be focused/announced, then
add a keydown listener on this.#dom.container (or the same selector) that finds
closest ".review-star" like the click handler and on Enter/Space (keydown codes)
prevents default and reads the data-score into ratingValue and calls
handler(ratingValue); keep the existing click logic unchanged but reuse the same
closest(".review-star") + getAttribute("data-score") flow so both mouse and
keyboard path use the same validation and error handling.
| const movieId = this.#dom.title.getAttribute("data-movie-id"); | ||
| if (!movieId) { | ||
| throw new Error("영화 id를 찾을 수 없습니다."); | ||
| } | ||
|
|
||
| await handler(Number(movieId)); | ||
| }); |
There was a problem hiding this comment.
클릭 핸들러에서 예외를 직접 던지면 사용자 흐름이 끊길 수 있습니다.
Line 28-30의 throw는 데이터 속성이 비어 있는 순간 전체 UI 오류로 이어질 수 있습니다. 여기서는 안전하게 early return하고, Number(movieId) 결과가 유효한지(NaN 여부)까지 가드하는 쪽이 더 안정적입니다.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/View/TopRatedView.ts` around lines 27 - 33, In TopRatedView's click
handler (the block using this.#dom.title and calling handler), don't throw when
data-movie-id is missing; instead early-return and optionally log or show a
user-friendly message; also validate the converted id by checking
Number(movieId) for NaN before calling handler, and only call await
handler(parsedId) when parsedId is a finite number.
| export const getYearFromDate = (dateString: string): number => { | ||
| const date = new Date(dateString); | ||
| return date.getFullYear(); | ||
| }; |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# getYearFromDate 사용 위치와 release_date 흐름을 확인해
# 호출부에서 유효성 보정이 있는지 점검합니다.
rg -nP --type=ts '\bgetYearFromDate\s*\('
rg -nP --type=ts '\brelease_date\b'Repository: woowacourse/javascript-movie-review
Length of output: 655
🏁 Script executed:
# MovieDetailView.ts에서 getYearFromDate 호출 맥락 확인
sed -n '70,95p' src/View/MovieDetailView.ts
# release_date 유효성 검증 여부 확인
grep -n "release_date" src/View/MovieDetailView.ts | head -20Repository: woowacourse/javascript-movie-review
Length of output: 901
🏁 Script executed:
# 함수 구현부 전체 확인 및 NaN 처리 검색
cat -n src/View/utils.ts
# getYearFromDate 반환값 사용 패턴 확인
rg -A 3 "getYearFromDate" src/View/MovieDetailView.tsRepository: woowacourse/javascript-movie-review
Length of output: 923
유효하지 않은 날짜 문자열 입력 시 NaN 반환 가능성
Line 11-14의 getYearFromDate 함수는 new Date(dateString).getFullYear()의 반환값을 검증하지 않습니다. 잘못된 형식의 dateString이 전달되면 NaN이 반환되어 Line 88의 DOM 텍스트로 "NaN"이 노출될 수 있습니다.
현재 호출부(MovieDetailView.ts Line 87)에서는 API 응답의 release_date 값을 직접 사용하고 있는데, 입력값이 항상 유효한 형식임을 보장할 수 있는지 확인해 주세요. 필요하면 함수 내에서 유효성을 검증하거나 호출부에서 사전 처리하는 방안을 검토해 주세요.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/View/utils.ts` around lines 11 - 14, getYearFromDate currently returns
new Date(dateString).getFullYear() without validation so an invalid release_date
can produce NaN and leak "NaN" into the DOM; update getYearFromDate to validate
the parsed date (e.g., use Date.parse or check isNaN(date.getTime())) and return
a safe fallback (empty string or null) when invalid, and adjust callers (e.g.,
MovieDetailView's usage of release_date) to handle the fallback when rendering
to the DOM. Ensure the function signature/documentation indicates the possible
fallback return and that MovieDetailView checks for that value before inserting
text into the DOM.
coolchaem
left a comment
coolchaem
left a comment
There was a problem hiding this comment.
이현 안녕하세요! 이전 단계에 이어서 미션 리뷰를 하게 된 조이입니다~
이번 단계에서는 구조적인 고민을 많이 하신 것 같았어요. 고민되는 포인트를 함께 적어주셔서, 이번에는 구조에 조금 더 집중해서 리뷰드렸습니다.
그리고 데모에서 무한 스크롤 방식으로 변경하신 점과 localStorage를 사용하신 점도 확인하였어요~
반응형 레이아웃 역시 창 크기를 조절했을 때 자연스럽게 적용되는 점이 인상적이었습니다 👏
항목별 답변 중 1, 6, 7번은 아래에 함께 남겼고, 2 ~ 5번은 관련 코드에 코멘트로 달아두었습니다~
이벤트 처리를 App에서 조합하게 되면서 여러 View의 책임이 한곳에 모이고 있다는 감각을 이미 느끼고 계신 것 같았습니다. 특히 아래에 적어주신 부분에서 그 고민이 잘 드러났고, 가장 리팩토링하고 싶은 부분으로 App.ts를 말씀해주신 점에서도 더 분명하게 확인할 수 있었어요.
그런데 기능이 늘어날수록 정말 이 목표 하나만 붙들고 가는 것이 맞는지 의문이 들었습니다. 현재는 App.ts에서 이벤트 핸들러를 등록하고 있지만, 하나의 핸들러 안에서 여러 View를 동시에 참조하는 경우가 많아졌습니다. 그럴 때마다 “이 구조를 그대로 유지해도 괜찮을까?”, “코드가 점점 복잡해지고 있는 건 아닐까?”라는 생각이 들었습니다.
지금 가장 리팩토링하고 싶은 부분은 App.ts입니다. 아마 기능이 계속 추가될수록 핸들러 함수도 많아지고 인스턴스 변수로 들고 있어야 하는 View도 많아질텐데 그에 따라 어떤 구조를 채택해야 될지는 아직 잘 모르겠습니다.
다음으로는 현재 scroll 이벤트 리스너를 추가했는데, 이것 대신 intersectionObserver를 사용해서 렌더링 성능을 개선해 보고 싶습니다.
이 문제를 인식하신 지점에는 저도 공감했고, 좋은 방향이라고 느껴졌습니다.
지금 구조에서는 View의 이벤트를 다룰 수 있는 계층이 하나 더 있으면 좋겠다고 보였어요. 예를 들어 무한 스크롤은 단순히 window scroll 이벤트를 받는 것에서 끝나지 않고, 현재 페이지 상태를 확인하고 다음 요청 여부를 판단한 뒤, 요청 결과에 따라 여러 View를 갱신하는 흐름까지 함께 가지고 있었습니다. 이런 책임이 계속 App에 쌓이기 시작하면, 말씀해주신 것처럼 App이 점점 커지고 각 View의 내부 구현까지 더 많이 알아야 하는 구조가 되기 쉬워 보여요.
그래서 무한 스크롤 로직을 별도 파일이나 객체로 분리해보거나, 말씀해주신 IntersectionObserver를 활용해 감지 책임을 따로 분리해보는 방향이 좋은 리팩토링 시작점이 될 수 있겠다고 느꼈습니다.
또한 Movie / MovieDetail 타입을 별도로 두신 것도 현재 단계에서는 크게 문제되지 않는 구조라고 생각했습니다. 말씀해주신 것처럼 공통 필드가 더 많아지고 중복이 계속 불편해진다면, movie base를 두고 세분화하거나 API 응답 기준으로 공통 타입을 추출하는 방식 등을 고려해보셔도 일리가 있다고 생각합니다.
전반적으로 이번 단계에서는 기능 구현뿐 아니라 현재 구조가 왜 복잡하게 느껴지는지를 잘 알고 설명해주신 점이 인상적이었습니다. 충분히 다음 리팩토링 방향도 잘 잡아가실 수 있을 것 같아서, 세부 수정 요청보다는 방향 위주로 코멘트 남겼습니다 👍
| import EmptyStarIcon from "../assets/star_empty.png"; | ||
| import { RATING } from "../constants/raintg"; | ||
|
|
||
| class RatingView { |
There was a problem hiding this comment.
- MovieDetailView와 RatingView를 따로 두는 것이 맞는 판단일까요?
사용자 인터랙션 기준으로 MovieDetailView와 RatingView를 나누신 점은 저는 좋다고 생각되었습니다 👍
아마 모호하다고 느끼신 부분은 RatingView를 분리해두었지만, 실제 별점 입력과 표시가 모두 상세 모달 안에서 함께 이루어지다 보니 이것을 별도 View로 두는 것이 맞는지 고민되셨던 것 같아요. 저는 같은 화면 범위 안에 있더라도 관심사나 책임이 다르다면 분리하는 방향은 충분히 가능하다고 생각해요~
대신 현재 구조에서 두 View의 조합을 App이 맡고 있다 보니, 분리했지만 여전히 강하게 연결되어 있다고 느끼셨을 수도 있을 것 같아요.
그래서 이런 부분이 실제로 복잡하게 느껴지셨다면, 파일은 분리하되 조합 책임을 MovieDetailView 쪽으로 옮겨보는 방향도 한 번 고민해보셔도 좋을 것 같습니다.
src/View/RatingView.ts
Outdated
| } | ||
|
|
||
| setRating(ratingValue: string) { | ||
| this.#dom.container.dataset.ratingValue = ratingValue; |
There was a problem hiding this comment.
- 별점 값처럼 UI에서는 문자열로 들어오지만 실제 의미는 숫자인 값은 어떤 기준으로 타입을 잡는 게 좋을까요?
이 부분은 dataset을 사용하고 있어서 string으로 다루어도 자연스럽다고 생각되었어요.
다만, 평점 자체는 의미상 number에 더 가깝고 가공 로직도 number로 인식되었어요. 그래서 프로젝트 전반에서 한 타입으로 어떤 타입으로 다룰지 한 번 정하고, DOM에 반영하는 시점에만 변환하는 방향도 전 괜찮아 보였어요. 예를 들어 내부에서는 number로 유지하고, 요 부분에서 넣을 때만 변환하는 방식도 가능할 것 같습니다.
아래 예시코드 남겨두며, 선택함에 있어서 두 방식의 장단점을 비교해보면 좋을 것 같네요. 장점을 예시로 하나 들어보면 한 타입으로 통일할 경우 여러 로직에서 타입 전환 에러로 나는 버그를 막을 수 있는 안정성을 들 수 있어보여요.
ex)
setRating(ratingValue: number) {
this.#dom.container.dataset.ratingValue = String(ratingValue);
}
src/App.ts
Outdated
|
|
||
| if (nowPage === totalPages) { | ||
| this.#views.moreMovie.hide(); | ||
| #bindWindowEvent = () => { |
There was a problem hiding this comment.
- bindEvent, bindClickEvent 같은 메서드 네이밍은 어떤 방향이 더 읽기 좋을까요?
bindEvent, bindClickEvent처럼 포괄적인 이름보다는 보통은 사용자 행위나 이벤트 목적이 드러나도록 네이밍하는 편이 더 읽기 좋다고 생각했습니다.
현재 메서드는 단순히 scroll 이벤트를 바인딩하는 수준보다는, 무한 스크롤을 통해 다음 영화 목록을 불러오는 흐름 전체에 가까워 보여서 bindInfiniteScroll, bindLoadMoreOnScroll처럼 의도가 드러나는 이름도 괜찮아 보였어요.
참고로 상황에 따라 View 내부에서만 사용하는 메서드라면, bindButtonClick, handleButtonClick처럼 대상과 이벤트가 함께 드러나는 이름도 충분히 괜찮다고 생각했습니다.
src/View/TopRatedView.ts
Outdated
| } | ||
|
|
||
| render({ title, poster_path, vote_average }: ThumbnailInfo) { | ||
| bindEvent(handler: (movieId: number) => void) { |
There was a problem hiding this comment.
- App으로부터 핸들러를 넘겨받지 않고 View 내부에서 이벤트를 직접 바인딩 해도 괜찮은 것일까요?
아마 handler를 받아 다시 값을 넘겨주는 현재 구조에서, getter를 두고 App.ts가 직접 값을 읽는 방향은 어떨지 고민하신 것 같아요. 저도 그 방식은 충분히 가능하다고 생각했습니다.
다만 그렇게 되면 App이 각 View의 DOM 구조를 조금 더 알게 되기 때문에, 현재처럼 기능이 늘어나는 단계에서는 App이 커지거나 View와의 결합이 강해진다고 느껴지실 수도 있을 것 같아요.
그래서 getter를 두고 App이 직접 값을 읽는 방향을 선택하신다면, App과 View 사이에서 이벤트와 값을 한 번 정리해주는 계층이 함께 있을 때 더 자연스러울 것 같았습니다. 그렇지 않다면 현재처럼 View가 DOM에서 필요한 값을 읽어 handler에 전달하는 방식이 오히려 더 단순할 수도 있다고 느꼈어요.
jebiyeon02
changed the title
2 participants
🎯 미션 소개
🕵️ 셀프 리뷰(Self-Review)
제출 전 체크 리스트
리뷰 요청 & 논의하고 싶은 내용
1) 이번 단계에서 가장 많이 고민했던 문제와 해결 과정에서 배운 점
이번 단계에서는 View의 책임을 어떻게 나눌지와 웹 접근성을 어떻게 더 자연스럽게 개선할지를 중심으로 고민하면서 구현했습니다.
구현이 진행될수록 어떤 이벤트는 View 내부에서 처리하는 것이 자연스럽고, 어떤 것은 App에서 제어해야 할지 기준이 조금씩 헷갈렸습니다. 특히 기능이 늘어날수록 App이 점점 비대해지는 느낌도 들어서, View에 어느 정도까지 책임을 위임할지, 그리고 App이 너무 많은 역할을 갖지 않도록 어떻게 구조를 잡아야 할지를 계속 고민하게 되었습니다.
웹 접근성 측면에서는 단순히 기능이 동작하는 수준에서 끝내지 않고, 키보드만으로도 자연스럽게 사용할 수 있는지를 함께 신경 쓰려고 했습니다. 기존에는 tab으로 포커스 할 수 없던 요소들을 역할에 맞는 button 태그로 바꿔 tab 포커스가 가능하도록 했고, 메인 로고의 tabindex도 설정하면서 포커스가 이동하도록 개선했습니다. 코드 상에서는 엄청나게 큰 변경점이 없었음에도 접근성을 꽤 개선할 수 있다는 것을 배웠습니다.
2) 이번 리뷰를 통해 논의하고 싶은 부분
안녕하세요 조이! 이번 리뷰도 잘 부탁드립니다 :)
이번 미션에서 제 가장 큰 목표는 제 코드 베이스 안에서 최적의 아키텍처를 스스로 찾아보는 것이었습니다. 구조 설계에 많은 시간을 쏟다 보니 E2E 테스트는 시나리오 작성에 집중하고, 테스트 코드 작성 자체는 AI의 도움을 일부 받았음을 미리 말씀드립니다. 코드 구조와 설계에 대한 피드백을 중점적으로 해주시면 정말 감사드리겠습니다..!
1. Movie와 MovieDetail 타입을 지금처럼 독립적으로 관리하는 방향이 괜찮을까요?
상세 정보와 일반 영화 정보가 겹치는 필드가 꽤 많아서, 중복을 줄일 수 있는 구조가 있을지 고민했습니다.
공통 타입을 하나 만들고 여기서 Pick을 하는 방식이 더 나을지, 아니면 API Response 단위로 분리하는 현재 방식이 더 나은지 궁금합니다.
2. MovieDetailView와 RatingView를 따로 두는 것이 맞는 판단일까요?
사용자 인터랙션 기준으로 뷰를 분리한다면 2개로 나누는게 맞다고 생각했는데, 상세정보 모달 내부에 별점을 매기는 기능이 있어서 기준이 모호하게 느껴지는 것 같습니다.
3. App으로부터 핸들러를 넘겨받지 않고 View 내부에서 이벤트를 직접 바인딩 해도 괜찮은 것일까요?
예를 들어 모달 닫기 버튼이나 별점 클릭 같은 동작은 View 내부에서 처리할 수도 있고, 외부에서 핸들러를 넘겨 제어할 수도 있을 것 같습니다.
사실 모달 닫기같은 로직은 뷰 내부에서 바로 이벤트 바인딩을 하는 것이 덜 복잡하게 느껴졌습니다.
정말 단순한 로직이고 외부와의 상호작용도 필요 없기 때문에 오히려 별도 메서드를 만들고 App에서 해당 메서드를 핸들러 내부에 넣어주는 과정이 더 복잡한 것 같다고 느껴서 이런 혼란을 겪고 있습니다.
또 다른 예시로는 아래와 같습니다. 제가 원했던 것은 bindEvent는 App.ts에서 넘겨주는 핸들러 메서드를 연결만 해주는 것이었습니다. 하지만 html tag에 존재하는 movie-id를 가져오기 위해서 아래와 같은 로직을 View 메서드 내부에 작성했습니다. 이 상태에서 핸들러 연결만 해주는 방향으로 가기 위해서 해당 View 내부에 movie-id를 반환하는 getter를 만들어야 하나 고민을 하고 있는데 어떤 방향이 더 좋은 방향인지 판단이 잘 안 섭니다.
4. 별점 값처럼 UI에서는 문자열로 들어오지만 실제 의미는 숫자인 값은 어떤 기준으로 타입을 잡는 게 좋을까요?
dataset에서 값을 읽어오다 보니 문자열로 다뤄지는 경우가 많았는데, 이걸 최대한 빨리 숫자로 변환해서 다루는 게 맞는지, 아니면 입력 계층에서는 문자열로 두고 내부 로직에서 숫자로 사용할때만 숫자로 다루는 게 맞는지 궁금합니다.
5. bindEvent, bindClickEvent 같은 메서드 네이밍은 어떤 방향이 더 읽기 좋을까요?
처음엔 bindEvent로 통일하고 싶었는데, View 하나에서 여러 이벤트를 바인딩하다 보니 이름만으로 의도가 잘 드러나지 않는 것 같았습니다.
메서드명이 어느 정도까지 구체적이어야 유지보수에 도움이 되는지 궁금합니다.
6. 지금 상태에서 어떤 구조적 변경을 시도해볼까요?
이 질문이 다소 포괄적이라는 점은 알고 있습니다.
다만 저는 1단계부터 DOM 접근 최소화라는 목표 하나를 중심으로 구조를 설계해왔고, class를 사용한 이유도 그와 같습니다.
그런데 기능이 늘어날수록 정말 이 목표 하나만 붙들고 가는 것이 맞는지 의문이 들었습니다. 현재는 App.ts에서 이벤트 핸들러를 등록하고 있지만, 하나의 핸들러 안에서 여러 View를 동시에 참조하는 경우가 많아졌습니다. 그럴 때마다 “이 구조를 그대로 유지해도 괜찮을까?”, “코드가 점점 복잡해지고 있는 건 아닐까?”라는 생각이 들었습니다.
반면 버튼 하나를 눌렀을 때 실제로 여러 화면 요소가 함께 변경되는 경우도 많기 때문에, 여러 View를 참조하는 것이 완전히 잘못된 것은 아닐 수도 있다고 느끼고 있습니다.
분명 더 나은 구조가 있을 것 같은데 아직 명확하게 떠오르지 않아서, 어떤 방향의 구조적 개선을 시도해볼 수 있을지 조언을 듣고 싶습니다.
7. window 이벤트는 어디서 처리해야 할까요?
무한스크롤을 구현하면서 window 객체에 스크롤 이벤트를 붙이는 작업이 필요했었습니다. App.ts에서 이벤트 바인딩을 하는 것을 지양하려고 했지만, 그렇다고 dom접근도 하지 않기 때문에 View를 만들어야 하는지도 모르겠습니다. 브라우저 View를 만들어야 할까요?
⚒️ 가장 리팩토링 하고 싶은 부분
지금 가장 리팩토링하고 싶은 부분은 App.ts입니다. 아마 기능이 계속 추가될수록 핸들러 함수도 많아지고 인스턴스 변수로 들고 있어야 하는 View도 많아질텐데 그에 따라 어떤 구조를 채택해야 될지는 아직 잘 모르겠습니다.
다음으로는 현재 scroll 이벤트 리스너를 추가했는데, 이것 대신 intersectionObserver를 사용해서 렌더링 성능을 개선해 보고 싶습니다.
✅ 리뷰어 체크 포인트