Open
Conversation
Co-authored-by: th-97 <hood3392@gmail.com>
Co-authored-by: th-97 <hood3392@gmail.com>
Co-authored-by: th-97 <hood3392@gmail.com>
Co-authored-by: th-97 <hood3392@gmail.com>
Co-authored-by: th-97 <hood3392@gmail.com>
Co-authored-by: th-97 <hood3392@gmail.com>
Co-authored-by: th-97 <hood3392@gmail.com>
Co-authored-by: th-97 <hood3392@gmail.com>
Co-authored-by: th-97 <hood3392@gmail.com>
Co-authored-by: th-97 <hood3392@gmail.com>
Co-authored-by: th-97 <hood3392@gmail.com>
- PopularMovies, SearchedMovies 클래스로 데이터와 페이지 상태 캡슐화 - movieRenderer에서 API 호출 제거, Movie[] 수신 후 렌더링만 담당 - eventListeners.ts 분리로 이벤트 등록 책임 분리 - AppMode 타입 도입으로 boolean 상태를 명시적 타입으로 변경 - API 에러 처리를 App.ts로 이동, API 레이어는 throw만 담당 - AppState 제거
Walkthrough이 PR은 영화 브라우저 애플리케이션의 전반적인 아키텍처를 재구성합니다. 기존의 상태 관리 클래스(PopularMovies, SearchedMovedMovies)를 제거하고 순수 도메인 모델(MovieBrowser)과 분리된 프레젠테이션 계층으로 대체합니다. 모달 팝업, 사용자 별점 평가, localStorage 기반 평점 유지, 그리고 무한 스크롤 기능을 추가합니다. 이벤트 리스너를 개별 초기화 함수로 재구성하고, API 인터페이스를 재설계하여 요청 취소 기능(AbortSignal)을 지원합니다. 스타일시트를 개선하여 모달 UI와 반응형 레이아웃을 구현하고, Cypress 테스트를 업데이트합니다. Estimated code review effort🎯 5 (Critical) | ⏱️ ~120 minutes 🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 inconclusive)
✅ 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: 14
🧹 Nitpick comments (5)
templates/index.html (1)
80-80: 주석 처리된 버튼 마크업은 완전 제거를 권장합니다.Line 80처럼 남겨두면 무한 스크롤 전환 의도가 흐려져 유지보수 시 혼선을 줄 수 있습니다.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@templates/index.html` at line 80, Remove the commented-out button markup for the "load-movie-button" from templates/index.html (the <!-- <button id="load-movie-button">더 보기</button> --> fragment on line 80) so the obsolete markup is fully deleted; if you need to keep intent for future reference, replace it with a short, explicit TODO comment or put the markup behind a feature-flagged template block instead of leaving a commented element.src/presentation/CustomRatingRender.ts (2)
36-42:ratingMap정의 위치 개선 권장
ratingMap이 파일 하단에 정의되어 있어 코드 가독성이 떨어집니다. 상단이나 사용되는 함수 근처로 이동하면 코드 흐름 파악이 더 쉬워집니다.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/presentation/CustomRatingRender.ts` around lines 36 - 42, The ratingMap constant is defined far from its usage which hurts readability; move the ratingMap Map declaration closer to its consumer or to the top of the module (e.g., above or immediately before the function/class that uses it) so readers see the mapping before it's referenced; ensure you export or keep its scope unchanged (rename nothing) and update any imports/usages to the same identifier ratingMap if necessary.
18-22:ratingMap.get(rate)!에서 잠재적 edge case 확인 필요
rate가ratingMap에 없는 값(예: 0 또는 홀수)인 경우undefined가 반환될 수 있습니다. 현재 별점 클릭 로직(index * 2)에서는 2, 4, 6, 8, 10만 저장되므로 정상 동작하지만, 방어적 코드를 고려해볼 수 있습니다.♻️ 선택적 개선안
- evaluateEl.textContent = rate === null ? '아직 별점을 남기지 않으셨습니다' : ratingMap.get(rate)!; + evaluateEl.textContent = rate === null ? '아직 별점을 남기지 않으셨습니다' : (ratingMap.get(rate) ?? '');🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/presentation/CustomRatingRender.ts` around lines 18 - 22, renderEvaluate currently assumes ratingMap.get(rate)! always returns a value which can be undefined for unexpected rates; update renderEvaluate to defensively handle missing entries from ratingMap by checking the result of ratingMap.get(rate) and using a sensible fallback (e.g., a localized fallback string like '알 수 없는 별점' or formatting the numeric rate) before assigning evaluateEl.textContent; reference renderEvaluate, rate, ratingMap and evaluateEl when making the change.src/presentation/ModalRenderer.ts (1)
29-30:release_date방어적 처리 고려
detail.release_date.slice(0, 4)가 빈 문자열이거나 예상과 다른 형식일 경우 UI에 빈 값이 표시될 수 있습니다. TMDB API에서release_date가 빈 문자열("")로 올 수 있으므로 fallback 처리를 고려해보세요.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/presentation/ModalRenderer.ts` around lines 29 - 30, In ModalRenderer, defend against empty or malformed release_date before slicing: when setting category.textContent (the element queried as '.modal-description .category'), compute a safe year by checking detail.release_date is a non-empty string and matches a four-digit year (or fallback to detail.first_air_date if available), otherwise use a default like 'Unknown Year'; then build the string `${year} · ${detail.genres.map(g => g.name).join(', ')}` and assign it to category.textContent so slicing of an empty or invalid release_date is avoided.cypress/e2e/spec.cy.ts (1)
216-278: 이전 더보기 버튼 시나리오는 주석 대신 정리해두는 편이 낫습니다.Line 216-278의 큰 주석 블록 때문에 현재 무한 스크롤 커버리지와 폐기된 테스트가 한 파일에 섞여 보입니다. 유지할 계획이 아니라면 이슈로 옮기고 제거하는 쪽이 테스트 의도를 읽기 쉽습니다.
🤖 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 216 - 278, Remove the large commented-out test block (the describes titled "인기 영화 더보기 버튼이 숨겨지는지 테스트" and "검색 영화 더보기 버튼이 숨겨지는지 테스트" and the subsequent TODO/describes) from spec.cy.ts and either open a tracker/issue or move them into a separate archive file; specifically target the commented tests that reference "#load-movie-button", the search flow using ".search-input", and the placeholder "무한 스크롤 테스트"/"모달 테스트" stubs, so the active infinite-scroll tests and coverage aren’t cluttered—delete the commented code or relocate it to an issue/file and leave a single concise TODO note if needed.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@CLAUDE.md`:
- Around line 37-39: Update the MovieController description to match the current
function-based implementation: replace references to private methods
`#createNewRequest()`, `#applyResult()`, `#handleError()` with the actual
exported/defined functions `createNewRequest`, `handleSuccess`, and
`handleError` as implemented in `src/presentation/MovieController.ts` (snippet
lines 17-42); also adjust wording to describe the function-based flow (event →
domain command → success/error handlers → renderer) and ensure `MovieRenderer`
responsibilities (start/stop loading called by controller, `render()` only
displays data) remain accurate.
In `@public/styles/modal.css`:
- Around line 106-145: The 480px media query values (e.g., .modal-description
margin-top: 0 and other mobile-specific rules for .modal-background, .modal,
.modal-container, .modal-image, .modal-description, .modal-description .rate,
`#customRate`) are being overwritten by the later 1024px rules; fix by ensuring
the mobile rules win: either move the entire `@media` (max-width: 480px) block
after the 1024px block, or change the queries to non-overlapping ranges (e.g.,
use `@media` (min-width: 481px) and `@media` (max-width: 480px) or `@media`
(max-width: 1024px) and `@media` (max-width: 480px) ordered smallest-first), so
that .modal-description { margin-top: 0 } and the other mobile-specific styles
persist on screens ≤480px.
In `@public/styles/thumbnail.css`:
- Around line 86-89: The current media query sets .thumbnail-list
grid-template-columns: repeat(3, 200px) which combined with gap: 70px can force
horizontal overflow on mid-size viewports; update the layout to be responsive by
either adding an additional breakpoint that switches .thumbnail-list to two
columns (e.g., use repeat(2, 1fr) or similar) for smaller tablet widths, or
replace the fixed repeat(3, 200px) with a fluid pattern using
grid-template-columns: repeat(auto-fit, minmax(180px, 1fr)) so items shrink/grow
and avoid overflow while keeping existing gap: 70px intact; adjust the media
query for (max-width: 1024px) or add one around the 480–768px range to apply the
two-column or auto-fit rule.
In `@README.md`:
- Around line 50-55: The numbering in the checklist is inconsistent: change the
duplicated "3. UI/UX 개선" and the "4 E2E 구현" entry to a continuous, properly
punctuated sequence (e.g., "3. UI/UX 개선" and "4. E2E 구현"), and ensure all
numbered headings include the trailing period so the list flows correctly;
update the README.md lines that contain the strings "3. UI/UX 개선" and "4 E2E 구현"
accordingly.
In `@src/domain/MovieBrowser.ts`:
- Around line 13-15: canLoadMore currently returns !this.isLastPage which is
true when `#totalPages` is 0, allowing requests before the first successful page;
change the logic in the getter (canLoadMore) to also require that `#totalPages` is
set (e.g., totalPages > 0) or that currentPage < totalPages so it returns false
until at least one successful response sets totalPages, referencing the existing
canLoadMore getter, the isLastPage boolean and the totalPages/currentPage
properties to locate and fix the code.
In `@src/eventListeners.ts`:
- Around line 1-2: getKeyword currently returns the raw input value so strings
with only whitespace pass the truthy check; change getKeyword to return
document.querySelector<HTMLInputElement>('.search-input')?.value.trim() ?? '' so
callers (click/enter handlers) receive a trimmed value and existing `if
(keyword)` checks will properly filter empty searches; update any related event
handlers that call getKeyword to rely on the trimmed result (e.g., the search
click and Enter key handlers referenced in this file).
- Around line 30-33: The scroll-bottom check in initLoadMore is too strict (uses
===) and should allow a small threshold; replace the exact equality with a >=
comparison against document.documentElement.scrollHeight minus a small constant
(e.g., const THRESHOLD = 100) so the handler (the callback passed as onLoadMore)
fires when the viewport is near the bottom; keep the event listener on window
with a named handler if you want to allow removal later.
In `@src/movieAPIResponse.ts`:
- Around line 21-23: apiFetch and fetchMovieDetail are leaking TMDB snake_case
fields out of the API boundary; update these functions so they normalize TMDB
responses to camelCase DTOs (apiFetch should return MoviePage with results
mapped to objects with camelCase properties and totalPages set from
json.total_pages) and fetchMovieDetail should map poster_path → posterPath,
vote_average → voteAverage, release_date → releaseDate (perform mapping only
inside src/movieAPIResponse.ts so snake_case stays contained here), and do not
add try/catch — let errors propagate as before.
In `@src/presentation/CustomRatingRepository.ts`:
- Around line 1-8: Wrap localStorage writes in saveCustomRate with a try-catch
and handle failures gracefully: in saveCustomRate(movieId, rating) validate
rating is a finite number before attempting setItem, catch any DOMException from
localStorage.setItem and either log the error and return silently or rethrow a
controlled error; also update getCustomRate(movieId) to convert the stored
string to Number and return null if Number(...) is NaN to avoid propagating NaN
into rendering. Ensure rateMovie (the caller) no longer relies on localStorage
throwing by either wrapping its call to saveCustomRate in try-catch or by
assuming saveCustomRate handles errors internally.
In `@src/presentation/ModalController.ts`:
- Around line 10-19: openModal currently issues fetchMovieDetail calls that can
complete out-of-order and overwrite modal state; make it cancel or ignore stale
requests by adding a per-module AbortController or request token and using it in
fetchMovieDetail (or wrapping the fetch) so previous requests are aborted when a
new openModal(id) runs, and/or check the token before calling selection.select,
renderModal, and renderCustomRating to ensure only the most recent request's
response is applied; update fetchMovieDetail to accept an AbortSignal (or
propagate the token) and ensure error handling distinguishes aborts from real
errors.
In `@src/presentation/ModalRenderer.ts`:
- Around line 15-39: The renderModal function declares an unused parameter
customRateNum; remove the parameter from renderModal's signature and any callers
should continue to pass only state (update places that call renderModal if they
currently pass a second arg), since custom ratings are already handled
separately by renderCustomRating in ModalController.ts; update the export line
and any imports/usages referencing renderModal to match the new single-argument
signature.
In `@src/presentation/MovieController.ts`:
- Line 44: The loadPopular export currently calls load(browser.currentPage) but
does not restore the controller's fetch strategy to popular; modify loadPopular
to explicitly replace the controller's FetchStrategy with popularStrategy (same
mechanism search() uses to set searchStrategy) before invoking
load(browser.currentPage), so loadPopular always uses popularStrategy rather
than whatever search() left in `#strategy`; reference the FetchStrategy
replacement logic used in search(), the identifiers loadPopular, load,
searchStrategy, popularStrategy, and browser.currentPage to locate where to
apply the change.
- Around line 29-40: The load function's finally block can clear isLoading for a
previous/aborted request; fix it by tying the finally logic to the specific
request started: when calling createNewRequest() capture its unique identifier
or AbortController (e.g., localRequestController/localRequestId) and compare it
to the current global controller/id before setting isLoading = false; update
createNewRequest (or the module-level current controller variable) so callers
can compare equality, and in load only call handleSuccess/handleError for the
matching request and only reset isLoading in finally when the local
controller/id === currentController/currentRequestId.
In `@src/presentation/MovieRenderer.ts`:
- Around line 41-45: renderBanner currently appends a banner every time and
assumes a movie exists, causing duplicates and runtime errors; update the render
flow so that before appending you clear the banner container
(querySelector('.top-rated-movie') and remove its children or set innerHTML =
'') and also clear the background container ('.background-container') at session
start, and guard any call that passes movies[0] into
renderBanner/createBannerHTML by checking the movies array and only calling
renderBanner when movies.length > 0 and movies[0] is truthy; adjust renderBanner
(and the code path that invokes it) to perform these checks and container resets
to avoid accumulation and null dereferences.
---
Nitpick comments:
In `@cypress/e2e/spec.cy.ts`:
- Around line 216-278: Remove the large commented-out test block (the describes
titled "인기 영화 더보기 버튼이 숨겨지는지 테스트" and "검색 영화 더보기 버튼이 숨겨지는지 테스트" and the
subsequent TODO/describes) from spec.cy.ts and either open a tracker/issue or
move them into a separate archive file; specifically target the commented tests
that reference "#load-movie-button", the search flow using ".search-input", and
the placeholder "무한 스크롤 테스트"/"모달 테스트" stubs, so the active infinite-scroll tests
and coverage aren’t cluttered—delete the commented code or relocate it to an
issue/file and leave a single concise TODO note if needed.
In `@src/presentation/CustomRatingRender.ts`:
- Around line 36-42: The ratingMap constant is defined far from its usage which
hurts readability; move the ratingMap Map declaration closer to its consumer or
to the top of the module (e.g., above or immediately before the function/class
that uses it) so readers see the mapping before it's referenced; ensure you
export or keep its scope unchanged (rename nothing) and update any
imports/usages to the same identifier ratingMap if necessary.
- Around line 18-22: renderEvaluate currently assumes ratingMap.get(rate)!
always returns a value which can be undefined for unexpected rates; update
renderEvaluate to defensively handle missing entries from ratingMap by checking
the result of ratingMap.get(rate) and using a sensible fallback (e.g., a
localized fallback string like '알 수 없는 별점' or formatting the numeric rate)
before assigning evaluateEl.textContent; reference renderEvaluate, rate,
ratingMap and evaluateEl when making the change.
In `@src/presentation/ModalRenderer.ts`:
- Around line 29-30: In ModalRenderer, defend against empty or malformed
release_date before slicing: when setting category.textContent (the element
queried as '.modal-description .category'), compute a safe year by checking
detail.release_date is a non-empty string and matches a four-digit year (or
fallback to detail.first_air_date if available), otherwise use a default like
'Unknown Year'; then build the string `${year} · ${detail.genres.map(g =>
g.name).join(', ')}` and assign it to category.textContent so slicing of an
empty or invalid release_date is avoided.
In `@templates/index.html`:
- Line 80: Remove the commented-out button markup for the "load-movie-button"
from templates/index.html (the <!-- <button id="load-movie-button">더 보기</button>
--> fragment on line 80) so the obsolete markup is fully deleted; if you need to
keep intent for future reference, replace it with a short, explicit TODO comment
or put the markup behind a feature-flagged template block instead of leaving a
commented element.
🪄 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: e1c9138b-b51d-4af3-acc1-49fa60bb944b
📒 Files selected for processing (31)
CLAUDE.mdREADME.mdcypress/e2e/spec.cy.tscypress/fixtures/movieDetail.jsoncypress/fixtures/movies.jsoncypress/fixtures/movies2.jsonpublic/styles/main.csspublic/styles/modal.csspublic/styles/thumbnail.csssrc/App.tssrc/PopularMovies.tssrc/SearchedMovies.tssrc/createHtml.tssrc/dom.tssrc/domain/MovieBrowser.tssrc/domain/MovieSelection.tssrc/eventListeners.tssrc/initTemplate.tssrc/movieAPIResponse.tssrc/movieRenderer.tssrc/presentation/CustomRatingRender.tssrc/presentation/CustomRatingRepository.tssrc/presentation/ModalController.tssrc/presentation/ModalRenderer.tssrc/presentation/MovieController.tssrc/presentation/MovieRenderer.tssrc/presentation/fetchStrategies.tstemplates/index.htmltemplates/styles/modal.csstypes/Movie.tstypes/MovieDetail.ts
💤 Files with no reviewable changes (4)
- src/PopularMovies.ts
- src/dom.ts
- src/SearchedMovies.ts
- src/movieRenderer.ts
Comment on lines
+37
to
+39
| - `src/presentation/fetchStrategies.ts` — `FetchStrategy` 타입과 `popularStrategy` / `searchStrategy`. 모드별 fetch 전략을 교체 가능하게 분리. | ||
| - `src/presentation/MovieController.ts` — 이벤트 → 도메인 명령 → 렌더 흐름 조율. `#createNewRequest()`, `#applyResult()`, `#handleError()`로 분해됨. 에러 처리와 로딩 상태 관리 담당. | ||
| - `src/presentation/MovieRenderer.ts` — `MovieBrowser` 상태를 받아 DOM 반영. 도메인 상태를 해석하지 않고 그대로 사용. `startLoading()` / `stopLoading()`은 컨트롤러가 호출하고, `render()`는 데이터 표시만 담당. |
There was a problem hiding this comment.
MovieController 설명은 현재 구현 기준으로 다시 맞춰두는 편이 좋겠습니다.
Line 38은 #createNewRequest(), #applyResult(), #handleError() 같은 private method 구조를 전제로 설명하지만, 제공된 src/presentation/MovieController.ts:17-42 스니펫은 함수 기반 createNewRequest / handleSuccess / handleError 흐름입니다. 이 문서가 작업 지침으로 쓰이는 파일이라면 현재 코드와 맞춰 두는 편이 안전합니다.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@CLAUDE.md` around lines 37 - 39, Update the MovieController description to
match the current function-based implementation: replace references to private
methods `#createNewRequest()`, `#applyResult()`, `#handleError()` with the
actual exported/defined functions `createNewRequest`, `handleSuccess`, and
`handleError` as implemented in `src/presentation/MovieController.ts` (snippet
lines 17-42); also adjust wording to describe the function-based flow (event →
domain command → success/error handlers → renderer) and ensure `MovieRenderer`
responsibilities (start/stop loading called by controller, `render()` only
displays data) remain accurate.
Comment on lines
+106
to
+145
| @media (max-width: 480px) { | ||
| .modal-background { | ||
| align-items: flex-end; | ||
| } | ||
|
|
||
| .modal { | ||
| width: 100%; | ||
| border-radius: 16px 16px 0 0; | ||
| margin: 0; | ||
| position: fixed; | ||
| bottom: 0; | ||
| left: 0; | ||
| max-height: 80vh; | ||
| overflow-y: auto; | ||
| } | ||
|
|
||
| .modal-container { | ||
| flex-direction: column; | ||
| align-items: center; | ||
| } | ||
|
|
||
| .modal-image { | ||
| display: none; | ||
| } | ||
|
|
||
| .modal-description { | ||
| margin-left: 0; | ||
| margin-top: 0; | ||
| text-align: center; | ||
| } | ||
|
|
||
| .modal-description .rate { | ||
| justify-content: center; | ||
| } | ||
|
|
||
| #customRate { | ||
| flex-direction: column; | ||
| align-items: center; | ||
| } | ||
| } |
There was a problem hiding this comment.
480px 이하에서는 뒤의 1024px 규칙이 다시 덮어써서 레이아웃이 어긋납니다.
두 미디어쿼리가 동시에 매치되는데 1024px 블록이 뒤에 있어서, 480px 전용으로 준 .modal-description의 margin-top: 0이 다시 16px으로 바뀝니다. 작은 화면 전용 값이 유지되도록 순서를 바꾸거나 조건 범위를 분리해 주세요.
Also applies to: 147-171
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@public/styles/modal.css` around lines 106 - 145, The 480px media query values
(e.g., .modal-description margin-top: 0 and other mobile-specific rules for
.modal-background, .modal, .modal-container, .modal-image, .modal-description,
.modal-description .rate, `#customRate`) are being overwritten by the later 1024px
rules; fix by ensuring the mobile rules win: either move the entire `@media`
(max-width: 480px) block after the 1024px block, or change the queries to
non-overlapping ranges (e.g., use `@media` (min-width: 481px) and `@media`
(max-width: 480px) or `@media` (max-width: 1024px) and `@media` (max-width: 480px)
ordered smallest-first), so that .modal-description { margin-top: 0 } and the
other mobile-specific styles persist on screens ≤480px.
Comment on lines
+86
to
+89
| @media (max-width: 1024px) { | ||
| .thumbnail-list { | ||
| grid-template-columns: repeat(3, 200px); | ||
| } |
There was a problem hiding this comment.
중간 뷰포트에서 가로 오버플로우가 날 가능성이 큽니다.
Line 88의 repeat(3, 200px)는 기존 gap: 70px와 합쳐 최소 폭 요구치가 커서(3열+간격), 481px~소형 태블릿 구간에서 레이아웃이 깨질 수 있습니다. 2열 브레이크포인트를 하나 더 두거나 auto-fit/minmax 접근을 검토해 주세요.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@public/styles/thumbnail.css` around lines 86 - 89, The current media query
sets .thumbnail-list grid-template-columns: repeat(3, 200px) which combined with
gap: 70px can force horizontal overflow on mid-size viewports; update the layout
to be responsive by either adding an additional breakpoint that switches
.thumbnail-list to two columns (e.g., use repeat(2, 1fr) or similar) for smaller
tablet widths, or replace the fixed repeat(3, 200px) with a fluid pattern using
grid-template-columns: repeat(auto-fit, minmax(180px, 1fr)) so items shrink/grow
and avoid overflow while keeping existing gap: 70px intact; adjust the media
query for (max-width: 1024px) or add one around the 480–768px range to apply the
two-column or auto-fit rule.
Comment on lines
+50
to
+55
| 3. UI/UX 개선 | ||
|
|
||
| - [x] Figma 시안을 기준으로 UI를 구현합니다. | ||
|
|
||
| 4 E2E 구현 | ||
|
|
There was a problem hiding this comment.
체크리스트 번호 표기를 정리해 주세요.
Line 50이 다시 3.으로 시작하고, Line 54는 4 뒤 마침표가 없어 문서 흐름이 끊겨 보입니다.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@README.md` around lines 50 - 55, The numbering in the checklist is
inconsistent: change the duplicated "3. UI/UX 개선" and the "4 E2E 구현" entry to a
continuous, properly punctuated sequence (e.g., "3. UI/UX 개선" and "4. E2E 구현"),
and ensure all numbered headings include the trailing period so the list flows
correctly; update the README.md lines that contain the strings "3. UI/UX 개선" and
"4 E2E 구현" accordingly.
Comment on lines
+13
to
+15
| get canLoadMore() { | ||
| return !this.isLastPage; | ||
| } |
There was a problem hiding this comment.
첫 페이지를 아직 못 받은 상태에서도 canLoadMore가 true입니다.
#totalPages 초기값이 0이면 isLastPage가 false라서 canLoadMore가 true가 됩니다. 그런데 src/presentation/MovieController.ts:52-56은 이 값을 그대로 무한 스크롤 가드로 쓰고 있어서, 초기 요청이나 검색 요청이 실패한 뒤 스크롤하면 1페이지 없이 바로 2페이지를 요청하게 됩니다. 첫 성공 응답 전에는 더 불러오기를 막는 상태가 필요합니다.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/domain/MovieBrowser.ts` around lines 13 - 15, canLoadMore currently
returns !this.isLastPage which is true when `#totalPages` is 0, allowing requests
before the first successful page; change the logic in the getter (canLoadMore)
to also require that `#totalPages` is set (e.g., totalPages > 0) or that
currentPage < totalPages so it returns false until at least one successful
response sets totalPages, referencing the existing canLoadMore getter, the
isLastPage boolean and the totalPages/currentPage properties to locate and fix
the code.
Comment on lines
+10
to
+19
| export const openModal = async (id: number) => { | ||
| try { | ||
| const detail = await fetchMovieDetail(id); | ||
| selection.select(detail); | ||
| const customRate = getCustomRate(id) | ||
| renderModal(selection,customRate); | ||
| renderCustomRating(customRate) | ||
| } catch (error) { | ||
| showError(error); | ||
| } |
There was a problem hiding this comment.
상세 요청을 취소하지 않아 늦게 끝난 응답이 모달 상태를 덮어쓸 수 있습니다.
사용자가 영화를 연속으로 클릭하거나 닫기 전에 요청이 남아 있으면, 먼저 보낸 응답이 나중에 도착하면서 다른 영화 상세로 바뀌거나 닫힌 모달이 다시 열릴 수 있습니다. 이전 상세 요청을 abort하거나, 가장 마지막 요청만 반영되도록 가드가 필요합니다.
Also applies to: 29-31
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/presentation/ModalController.ts` around lines 10 - 19, openModal
currently issues fetchMovieDetail calls that can complete out-of-order and
overwrite modal state; make it cancel or ignore stale requests by adding a
per-module AbortController or request token and using it in fetchMovieDetail (or
wrapping the fetch) so previous requests are aborted when a new openModal(id)
runs, and/or check the token before calling selection.select, renderModal, and
renderCustomRating to ensure only the most recent request's response is applied;
update fetchMovieDetail to accept an AbortSignal (or propagate the token) and
ensure error handling distinguishes aborts from real errors.
Comment on lines
+15
to
+39
| export const renderModal = (state: MovieSelection, customRateNum : number | null) => { | ||
| if (!state.isOpen) { | ||
| hideModal(); | ||
| return; | ||
| } | ||
|
|
||
| const detail = state.selected!; | ||
|
|
||
| const img = document.querySelector<HTMLImageElement>('.modal-image img'); | ||
| if (img) img.src = `${posterBaseURL}${detail.poster_path}`; | ||
|
|
||
| const title = document.querySelector('.modal-description h2'); | ||
| if (title) title.textContent = detail.title; | ||
|
|
||
| const category = document.querySelector('.modal-description .category'); | ||
| if (category) category.textContent = `${detail.release_date.slice(0, 4)} · ${detail.genres.map((g) => g.name).join(', ')}`; | ||
|
|
||
| const rate = document.querySelector('#average-score') | ||
| if (rate) rate.textContent = `${detail.vote_average.toFixed(1)}` | ||
|
|
||
| const description = document.querySelector('.modal-description .detail'); | ||
| if (description) description.textContent = detail.overview; | ||
|
|
||
| showModal(); | ||
| }; |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Description: Check how renderModal is called and if customRateNum is needed
rg -n "renderModal" --type=ts -C 3Repository: woowacourse/javascript-movie-review
Length of output: 1932
customRateNum 파라미터가 사용되지 않음
함수 본문에서 customRateNum 파라미터가 참조되지 않습니다. ModalController.ts에서는 맞춤 평점을 renderCustomRating()으로 별도 처리하고 있으므로, 이 파라미터는 불필요합니다.
파라미터를 제거하세요.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/presentation/ModalRenderer.ts` around lines 15 - 39, The renderModal
function declares an unused parameter customRateNum; remove the parameter from
renderModal's signature and any callers should continue to pass only state
(update places that call renderModal if they currently pass a second arg), since
custom ratings are already handled separately by renderCustomRating in
ModalController.ts; update the export line and any imports/usages referencing
renderModal to match the new single-argument signature.
Comment on lines
+29
to
+40
| const load = async (page: number, onSuccess?: () => void) => { | ||
| const signal = createNewRequest(); | ||
| // startLoading(isLoading); | ||
| isLoading = true; | ||
| try { | ||
| const data = await strategy(page, signal); | ||
| handleSuccess(data, onSuccess); | ||
| } catch (error) { | ||
| // stopLoading(isLoading); | ||
| handleError(error); | ||
| } finally { | ||
| isLoading = false; |
There was a problem hiding this comment.
isLoading가 이전 요청의 finally에서 너무 일찍 풀릴 수 있습니다.
Line 30에서 이전 요청을 abort한 뒤 새 요청을 시작해도, 중단된 이전 요청이 Line 40에서 무조건 isLoading = false를 실행합니다. 그러면 최신 요청이 아직 진행 중이어도 loadMore()가 다시 열려 중복 fetch/append가 섞일 수 있습니다. finally가 “내가 시작한 요청”에만 적용되도록 요청 식별자나 현재 AbortController 비교로 범위를 묶어보세요.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/presentation/MovieController.ts` around lines 29 - 40, The load
function's finally block can clear isLoading for a previous/aborted request; fix
it by tying the finally logic to the specific request started: when calling
createNewRequest() capture its unique identifier or AbortController (e.g.,
localRequestController/localRequestId) and compare it to the current global
controller/id before setting isLoading = false; update createNewRequest (or the
module-level current controller variable) so callers can compare equality, and
in load only call handleSuccess/handleError for the matching request and only
reset isLoading in finally when the local controller/id ===
currentController/currentRequestId.
| } | ||
| }; | ||
|
|
||
| export const loadPopular = () => load(browser.currentPage); |
There was a problem hiding this comment.
loadPopular()이 현재 전략을 popular로 되돌리지 않습니다.
지금 구현이면 search()가 한 번 strategy를 바꾼 뒤 loadPopular()이 다시 호출되어도 마지막 searchStrategy를 그대로 사용합니다. 이 함수 안에서 popular 전략을 명시적으로 복원하는 쪽이 공개 API 의미와 맞습니다. As per coding guidelines, "src/presentation/MovieController.ts: Switch modes using FetchStrategy replacement: search() replaces #strategy with searchStrategy; loadPopular() uses popularStrategy; avoid fetch branching (if mode) in controller".
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/presentation/MovieController.ts` at line 44, The loadPopular export
currently calls load(browser.currentPage) but does not restore the controller's
fetch strategy to popular; modify loadPopular to explicitly replace the
controller's FetchStrategy with popularStrategy (same mechanism search() uses to
set searchStrategy) before invoking load(browser.currentPage), so loadPopular
always uses popularStrategy rather than whatever search() left in `#strategy`;
reference the FetchStrategy replacement logic used in search(), the identifiers
loadPopular, load, searchStrategy, popularStrategy, and browser.currentPage to
locate where to apply the change.
Comment on lines
+41
to
+45
| const renderBanner = (movie: Movie) => { | ||
| const bg = document.querySelector<HTMLElement>('.background-container'); | ||
| if (bg) bg.style.backgroundImage = `url("${bannerBaseURL + movie.backdrop_path}")`; | ||
| document.querySelector('.top-rated-movie')?.appendChild(createBannerHTML(movie)); | ||
| }; |
There was a problem hiding this comment.
배너는 새 세션마다 초기화하고 첫 영화 존재 여부를 먼저 확인해주세요.
.top-rated-movie는 계속 appendChild()만 하고 있어서 첫 페이지를 다시 렌더링하면 배너가 누적됩니다. 그리고 Line 97은 movies[0]을 바로 넘기므로 인기 영화 응답이 비어 있으면 배너 렌더에서 바로 터집니다. 새 세션에서는 배너 컨테이너도 함께 비우고, 첫 영화가 있을 때만 배너를 그리는 편이 안전합니다.
Also applies to: 92-101
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/presentation/MovieRenderer.ts` around lines 41 - 45, renderBanner
currently appends a banner every time and assumes a movie exists, causing
duplicates and runtime errors; update the render flow so that before appending
you clear the banner container (querySelector('.top-rated-movie') and remove its
children or set innerHTML = '') and also clear the background container
('.background-container') at session start, and guard any call that passes
movies[0] into renderBanner/createBannerHTML by checking the movies array and
only calling renderBanner when movies.length > 0 and movies[0] is truthy; adjust
renderBanner (and the code path that invokes it) to perform these checks and
container resets to avoid accumulation and null dereferences.
JUDONGHYEOK
requested changes
Apr 13, 2026
JUDONGHYEOK
left a comment
JUDONGHYEOK
left a comment
There was a problem hiding this comment.
안녕하세요 코브! 2단계도 1단계와 마찬가지로 아주 깔끔하게 구현해주셨군요!
우선 Presentation → Domain → Data Source 레이어를 의식하면서 단방향 의존성을 유지하려 하신 것이 코드에서 잘 느껴졌어요! 특히 MovieController에서 전략 패턴으로 인기영화, 검색을 교체 가능하게 분리하신건 개인적으로 합리적인 선택이였다고 생각합니다! 도메인이 DOM이나 fetch에 의존하지 않고 순수 상태 머신으로 동작하는 것도 잘 지켜졌고요! E2E 테스트도 mock 데이터 기반으로 주요 시나리오를 넓게 커버하고 있어서 첫 제출부터 아주 완성도 있는 코드를 작성해주셨다고 생각했습니다.
Q&A
이번 프로젝트에서 Presentation → Domain → Data Source 방향으로 잡았는데, 상황에 따라 이 방향이 달라질 수 있는지,
그리고 방향이 잘못됐다는 걸 어떻게 알아챌 수 있는지 궁금합니다.
저는 이 의존성에 대한 판단을 변경이 일어나는 시점에 판단할 수 있다고 생각해요! 예를 들어API 응답 형식이 바뀌었을 때 Domain이 깨지면 방향이 잘못된 거고, Presentation만 수정하면 되면 방향이 맞게 되는 것 같습니다.
이번 단계에서 기능을 먼저 만들고 테스트를 나중에 작성했습니다. 그러다 보니 기능이
어떻게 동작해야 하는지 미리 생각하지 않고 구현하게 됐고, 결과적으로 테스트가 검증 도구가 아니라 사후 확인 도구가 된 느낌이었습니다.
테스트를 먼저 작성했다면 설계를 먼저 고민하게 됐을 것 같은데, 실제 현업에서는 어떤
순서로 테스트를 작성하는지, 그리고 항상 기능 전에 테스트를 작성하는 게 맞는 방향인지 궁금합니다.
언제나 치트키처럼 사용되는 말이지만, 테스트 작성 순서에도 은탄환은 없다고 생각합니다. 새로운 도메인 규칙을 설계한다면 TDD처럼 테스트를 먼저 쓰면서 요 입력에서는 어떻게 동작해야한다면 규칙에서 많이 벗어나지 않게 되어 설계를 명확히 잡을 수 있구요. 반대로 복잡한 UI상호작용이나 기존 코드리팩토링처럼 흐름을 한번 쭉 구현해보아야 감이 오는 작업은 기능을 먼저 만든뒤 중요한 경로를 중심으로 테스를 보완하는 방식이 맞지 않나 생각이 되어요. 코브가 생각했을 때의 작성하는 테스트의 목적을 생각하면서 작성하면 도움이 되지 않을까 생각합니다!!
시지프가 수업중에 좋은코드를 많이 못 보셔서 좋은 구조를 짤 수 없다고 말씀하셨습니다.
좋은 코드라는 것은 무엇일까요? 그리고 좋은 코드를 보는 방법이 있을까요? 지금 저의 수준에서 좋은 코드를 이해하기 위해서는 어떤 노력을 해야할까요. 또한 책만이 답이 아니라고 생각하고 AI도 답이 아니라고 생각합니다.
저만의 기준이 있어야 할 것같은데 리뷰어 분은 어떤 기준으로 자신의 좋은 코드의 기준을 잡아나가셨는지 궁금합니다.
너무 어려운 말이네요ㅎㅎ 정답은 아니지만 제 개인적인 생각을 공유드리자면 좋은 코드는 도메인에 따라 달라지게 된다고 생각합니다. 따라서 좋은 코드를 판단할 때 먼저 해야 할 일은 코드가 놓인 맥락을 파악하는 것일 것 같은데요.
이 코드가 빠르게 검증하고 버려질 수 있는 1회성 도메인인지, 아니면 장기간 여러 사람의 손을 거치며 진화해야 하는 도메인인지에 따라 좋음의 기준 자체가 달라지게 되는 것 같아요. 전자라면 최대한 빠르게 동작하는 코드를 만드는 것이 좋은 코드이고, 후자라면 변경에 유연한 코드가 좋은 코드라고 생각해요.
변경에 유연한 코드란, 앞서 말씀드렸듯이 하나의 요구사항이 바뀌었을 때 수정 범위가 한 곳에 수렴하는 코드라고 생각해요. 역할과 책임이 명확히 나뉘어 있어서 변경의 이유가 하나의 모듈에만 존재하고, 그 변경이 다른 모듈로 전파되지 않는 구조인 것이지요.
결국 좋은 코드를 쓰려면 좋은 코드가 무엇인지 아는 것보다, 지금 내가 어떤 상황에서 코드를 쓰고 있는지를 정확히 인식하는 게 먼저라고 생각합니다!
좋은 코드를 익히는 방법으로는, 우테코 과정에 있으신 만큼 같은 미션을 수행한 크루들의 코드를 읽고 리뷰어분들의 피드백을 함께 보는 걸 추천해요! 같은 요구사항에 대해 서로 다른 설계를 비교하면서 여기서 요구사항이 바뀌면 이 구조는 어디까지 수정해야 하지?를 상상해보시고, 그 반복이 자신만의 기준을 만들어가는 현재 기준으로 가장 좋지 않을까 생각이 됩니다!
첫번째 제출이신데 너무 완성도 있는 코드를 만들어주시고 코브만의 고민이 많이 담긴 코드라서 리뷰하기 너무 좋은 코드였다고 생각이 됩니다! 너무 수고많으셨고 코멘트 확인해주시면 감사하겠습니다!
Comment on lines
+27
to
+28
| cy.get('.thumbnail-list li').first().click(); | ||
| cy.get('.modal-background').should('have.class', 'active'); |
There was a problem hiding this comment.
e2e테스트를 아주 꼼꼼히 작성해주셨군요! 좋습니다 테스트 곳곳에 공통된 로직이 있어서 추상화를 시도해주셔도 좋을 것 같아요!
| }); | ||
| }); | ||
|
|
||
| // describe("인기 영화 더보기 버튼이 숨겨지는지 테스트", () => { |
Comment on lines
+1
to
+2
| export const getCustomRate = (movieId: number) => { | ||
| const rate = localStorage.getItem(String(movieId)) |
There was a problem hiding this comment.
localStorage.getItem/setItem을 직접 호출하고 있는데요. 지금은 동작하지만, 나중에 저장소를 sessionStorage나 IndexedDB, 혹은 서버 API로 바꾸려면 이 파일을 직접 수정해야 합니다. OCP를 지키기 위해서는 어떻게 개선해볼 수 있을까요?
Comment on lines
+1
to
+8
| export const getCustomRate = (movieId: number) => { | ||
| const rate = localStorage.getItem(String(movieId)) | ||
| if(rate) return Number(rate) | ||
| return null | ||
| }; | ||
|
|
||
| export const saveCustomRate = (movieId: number, rating: number): void => { | ||
| localStorage.setItem(String(movieId), String(rating)) |
Comment on lines
+10
to
+16
| export const openModal = async (id: number) => { | ||
| try { | ||
| const detail = await fetchMovieDetail(id); | ||
| selection.select(detail); | ||
| const customRate = getCustomRate(id) | ||
| renderModal(selection,customRate); | ||
| renderCustomRating(customRate) |
There was a problem hiding this comment.
ModalController의 역할이 뭔지 조금 모호한 느낌이 있습니다 API 호출, 도메인 상태 관리, localStorage 조회, 렌더링 호출을 전부 하고 있습니다. MovieController처럼 fetch와 상태 관리를 분리하는 패턴을 여기에도 적용하면 어떨까요?
Comment on lines
+13
to
+15
| get canLoadMore() { | ||
| return !this.isLastPage; | ||
| } |
| document.body.classList.remove('modal-open'); | ||
| }; | ||
|
|
||
| export const renderModal = (state: MovieSelection, customRateNum : number | null) => { |
Comment on lines
+30
to
+33
| export const initLoadMore = (onLoadMore: () => void) => { | ||
| window.addEventListener('scroll', () => { | ||
| if(window.scrollY + window.innerHeight === document.documentElement.scrollHeight)onLoadMore() | ||
| }); |
There was a problem hiding this comment.
여기 때문에 해상도 차이에 따라 무한스크롤이 정상동작하지 않습니다 수정이 필요해보입니다!
There was a problem hiding this comment.
e2e를 꼼꼼히 작성해주셨는데요! 무한스크롤기능이 추가된만큼 해당 기능에 대한 테스트코드가 추가되면 좋을 것 같아요!
Comment on lines
+3
to
+4
| export class MovieSelection { | ||
| #selected: MovieDetail | null = null; |
There was a problem hiding this comment.
개인적으로 Domain의 책임이 너무 가볍지 않나 생각합니다. 때문에 controller가 비대해지는 문제가 생기게 되는 것 같구요. "이 영화에 별점을 매긴다"는 행위는 도메인의 책임인 것 같은데 도메인으로 책임을 옮기는 것은 어떨까요?
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
3 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
🎯 미션 소개
🕵️ 셀프 리뷰(Self-Review)
제출 전 체크 리스트
리뷰 요청 & 논의하고 싶은 내용
1) 이번 단계에서 가장 많이 고민했던 문제와 해결 과정에서 배운 점
가장 많이 고민했던 부분은 의존성의 방향과 책임 분리였습니다.
처음에는 단순히 파일을 나누는 것에 집중했습니다. 그런데 막상 나누고 보니, 파일만 분리됐을 뿐 서로를 참조하는 방식은 그대로였습니다. 겉으로는 구조가 나뉜 것처럼 보였지만, 실제 의존성은 전혀 정리되지 않은 상태였습니다.
특히 App.ts에서 그 문제가 가장 크게 드러났습니다. App.ts가 너무 많은 것을 알고 있었고, 그 결과 하나의 기능을 수정하려고 하면 여러 파일을 함께 건드려야 했습니다. 이건 분리가 제대로 되지 않았다라는 느낌을 받았고 AI와 싸우면서 구조를 바꾸었습니다.
결론은 App.ts는 앱을 실행시키는 진입점 역할만 하도록 두는 게 맞다고 판단했습니다. 대신 동작 단위로 Controller를 나누고, 각 책임을 그쪽으로 옮겼습니다.
E2E 테스트를 하면서는 테스트의 목적에 대해 다시 생각하게 되었습니다.
처음에는 실제 API URL을 그대로 사용해서 테스트를 작성했습니다. 그런데 진행하다 보니 이 방식이 맞지 않다는 생각이 들었습니다. 네트워크 상태나 외부 API 응답에 따라 테스트가 실패할 수 있었고, 그럴 경우 원인이 내 코드인지 외부 환경인지 구분하기 어려웠습니다. 또한 코드를 테스트하는 것인지 네트워크를 테스트하는 것인지 기준이 모호해졌습니다.
곰곰이 생각해보니, 테스트의 목적은 네트워크를 검증하는 것이 아니라 내가 만든 기능이 제대로 동작하는지 확인하는 것이었습니다. 그래서 AI를 활용해 mock 데이터를 만들고 테스트를 진행했습니다.
2) 이번 리뷰를 통해 논의하고 싶은 부분
의존성 방향이 올바른지 판단하는 기준이 뭔가요?
이번 프로젝트에서 Presentation → Domain → Data Source 방향으로 잡았는데, 상황에 따라 이 방향이 달라질 수 있는지,
그리고 방향이 잘못됐다는 걸 어떻게 알아챌 수 있는지 궁금합니다.
아래의 이미지는 저의 의존성 방향입니다
테스트의 역할은 무엇인가?
이번 단계에서 기능을 먼저 만들고 테스트를 나중에 작성했습니다. 그러다 보니 기능이
어떻게 동작해야 하는지 미리 생각하지 않고 구현하게 됐고, 결과적으로 테스트가 검증 도구가 아니라 사후 확인 도구가 된 느낌이었습니다.
테스트를 먼저 작성했다면 설계를 먼저 고민하게 됐을 것 같은데, 실제 현업에서는 어떤
순서로 테스트를 작성하는지, 그리고 항상 기능 전에 테스트를 작성하는 게 맞는 방향인지 궁금합니다.
좋은 코드라는 것은 무엇인가?
시지프가 수업중에 좋은코드를 많이 못 보셔서 좋은 구조를 짤 수 없다고 말씀하셨습니다.
좋은 코드라는 것은 무엇일까요? 그리고 좋은 코드를 보는 방법이 있을까요? 지금 저의 수준에서 좋은 코드를 이해하기 위해서는 어떤 노력을 해야할까요. 또한 책만이 답이 아니라고 생각하고 AI도 답이 아니라고 생각합니다.
저만의 기준이 있어야 할 것같은데 리뷰어 분은 어떤 기준으로 자신의 좋은 코드의 기준을 잡아나가셨는지 궁금합니다.
✅ 리뷰어 체크 포인트