[2단계 - 영화 목록 불러오기] 두부 미션 제출합니다.#306
Open
boboyoii wants to merge 66 commits intowoowacourse:boboyoiifrom
Open
Conversation
Co-authored-by: stardusty-lab <stardusty-lab@users.noreply.github.com>
Co-authored-by: stardusty-lab <stardusty-lab@users.noreply.github.com>
Co-authored-by: stardusty-lab stardusty-lab@users.noreply.github.com
Co-authored-by: stardusty-lab stardusty-lab@users.noreply.github.com
Co-authored-by: stardusty-lab stardusty-lab@users.noreply.github.com
Co-authored-by: stardusty-lab stardusty-lab@users.noreply.github.com
Co-authored-by: stardusty-lab <stardusty-lab@users.noreply.github.com>
- 20개 렌더링 확인 - 더보기 클릭시 영화 목록 추가 - 마지막 페이지에서 더보기 버튼 사라짐 Co-authored-by: stardusty-lab stardusty-lab@users.noreply.github.com
- 검색어 입력 후 검색 버튼 클릭하면 영화 목록 출력 - 검색어 입력 후 엔터 치면 영화 목록 출력 - 더보기 버튼 클릭 시 필터링 된 영화 목록 추가 - 더 이상 필터링 된 결과가 마지막 페이지면 더보기 버튼 출력 X - 검색 결과 없을 시 안내메시지 출력 Co-authored-by: stardusty-lab stardusty-lab@users.noreply.github.com
Co-authored-by: stardusty-lab stardusty-lab@users.noreply.github.com
Co-authored-by: stardusty-lab stardusty-lab@users.noreply.github.com
Co-authored-by: stardusty-lab <stardusty-lab@users.noreply.github.com>
Co-authored-by: stardusty-lab <stardusty-lab@users.noreply.github.com>
Co-authored-by: stardusty-lab stardusty-lab@users.noreply.github.com
Co-authored-by: stardusty-lab stardusty-lab@users.noreply.github.com
Co-authored-by: stardusty-lab stardusty-lab@users.noreply.github.com
Co-authored-by: stardusty-lab stardusty-lab@users.noreply.github.com
Co-authored-by: stardusty-lab stardusty-lab@users.noreply.github.com
Co-authored-by: stardusty-lab stardusty-lab@users.noreply.github.com
- x 버튼 누를 때 - ESC 키 누를 때
|
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이 PR은 영화 웹 앱에 영화 상세 모달, 별점 평가 기능, 무한 스크롤(scroll-sentinel 기반), 한국어 언어 지원(language=ko-KR)을 추가합니다. 기존 "더보기" 버튼을 감시 요소로 대체하고, 모달에서 영화 상세 정보를 표시하며, 5단계 별점 평가를 localStorage에 저장합니다. 또한 반응형 디자인(태블릿, 모바일)을 개선하고, 이벤트 바인딩을 재구성하며, 새로운 핸들러, 로더, 뷰 유틸리티를 추가합니다. Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes 🚥 Pre-merge checks | ✅ 3✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 15
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
src/view/movieList.ts (1)
45-50:⚠️ Potential issue | 🟡 Minorimage 경로를 코드베이스의 일관된 패턴과 맞춰야 합니다.
현재
"./public/images/mascot.png"경로는 코드베이스의 다른 모든 이미지 참조와 맞지 않습니다. HTML 파일들(templates/modal.html, templates/index.html, index.html)에서는 모두"./images/"패턴을 사용하고 있으므로, 이 파일에서도"./images/mascot.png"로 수정해야 합니다.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/view/movieList.ts` around lines 45 - 50, The image path used in the HTML fragment assigned to the constant empty (used to set noResult.innerHTML) is inconsistent with the repo's pattern; update the src from "./public/images/mascot.png" to "./images/mascot.png" in the template string assigned to empty so this component (the empty constant / noResult assignment in movieList.ts) matches other image references.public/styles/main.css (1)
67-73:⚠️ Potential issue | 🟠 Major1440px 최소 너비가 일반 노트북 해상도에서 가로 스크롤을 강제합니다.
#wrap이 이미 1440px 최소 너비를 갖고 있는데.background-container까지 같은 제약을 추가해서, 1024~1439px 구간에서는 미디어쿼리가 적용되기 전까지 레이아웃이 넘칩니다. 반응형 개선 범위라면 이 구간도 같이 고려하는 편이 좋겠습니다.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@public/styles/main.css` around lines 67 - 73, .background-container에 설정된 min-width: 1440px가 불필요하게 가로 스크롤을 유발하니 해당 속성을 제거하거나 반응형 규칙으로 대체하세요; 구체적으로 .background-container의 min-width를 삭제하고 대신 width를 100% 또는 max-width로 제어하거나, 1024–1439px 구간을 커버하는 미디어쿼리(예: `@media` (max-width:1439px) ... )를 추가해 레이아웃이 깨지지 않도록 조정하고, 이미 `#wrap에` 적용된 1440px 제약은 유지하되 중복 제약을 제거하세요.
🧹 Nitpick comments (10)
src/view/movieModal.ts (1)
1-15: 모달 열고 닫을 때 포커스 관리까지 같이 고려해 주세요.현재는 클래스 토글만 수행해서, 키보드 사용자 기준으로 모달 오픈 시 포커스가 배경에 남을 수 있습니다.
오픈 시 모달 내부 첫 포커스로 이동하고, 닫을 때 트리거 요소로 복귀시키는 흐름을 붙이면 접근성이 크게 좋아집니다.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/view/movieModal.ts` around lines 1 - 15, Update openMovieModal and closeMovieModal to manage focus: in openMovieModal capture and store the element that triggered the modal (e.g., document.activeElement) before toggling, then find the modal container (movieModal / "#modal-background"), query for the first focusable element inside the modal (buttons, links, inputs, [tabindex], etc.) and call .focus() on it after adding "active" and "modal-open"; in closeMovieModal remove "active" and "modal-open" and restore focus to the previously stored trigger element (if still in the document) and clear the stored reference; ensure these functions handle missing focusable elements gracefully and keep the movieModal reference to locate modal internals.src/services/dto.ts (1)
25-36:genres타입은 재사용 가능한 인터페이스로 분리해도 좋겠습니다.현재 인라인 타입도 동작에는 문제없지만, 다른 DTO/뷰 모델에서도 장르 구조를 쓰게 되면 중복이 생기기 쉬워요.
Genre타입을 별도 선언해 재사용하면 변경 영향 범위 관리가 쉬워집니다.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/services/dto.ts` around lines 25 - 36, The MovieDetail interface uses an inline genres type which can cause duplication; extract a reusable exported interface named Genre (e.g., interface Genre { id: number; name: string }) and replace the inline type in MovieDetail with genres: Genre[]; update any other DTOs or view models that use the same shape to reference Genre to centralize the definition and simplify future changes.src/handlers/searchHandler.ts (2)
15-17:loadSearchMovies의 비동기 처리 방식을 검토해 보세요.
loadSearchMovies는 async 함수인데await없이 호출되고 있습니다. 현재 구현에서는 문제가 없을 수 있지만, 향후 검색 완료 후 특정 동작(예: 포커스 이동, 로딩 상태 표시)이 필요할 때 고려가 필요합니다.이 방식이 의도된 것인지, 아니면
await가 필요한지 생각해 보시기 바랍니다.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/handlers/searchHandler.ts` around lines 15 - 17, The call to loadSearchMovies (an async function) is invoked without awaiting its promise after navigate(createSearchUrl(baseUrl, search)), which may cause race conditions for post-search actions; either add await before loadSearchMovies and mark the enclosing handler function async (so subsequent logic runs after movies finish loading), or explicitly handle the returned promise (e.g., loadSearchMovies({ reset: true }).then(...).catch(...)) if you intentionally want it to run concurrently—apply this change around the code that calls createSearchUrl, navigate, and loadSearchMovies to ensure the intended sequencing.
9-13: 공백만 있는 검색어 처리를 고려해 보세요.현재
!search.length체크는 빈 문자열만 처리합니다. 사용자가 공백만 입력한 경우(" ")에도 검색이 실행될 수 있습니다. 이런 경우를 방지하려면 어떤 방법이 있을까요?힌트:
trim()메서드를 활용해 보세요.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/handlers/searchHandler.ts` around lines 9 - 13, The current check uses search = searchInput.value || "" and only checks !search.length, which misses inputs that are all whitespace; update the logic in the search handler to compute a trimmed value (e.g., const trimmed = (searchInput.value || "").trim()) and use trimmed.length to decide early return: if (!trimmed.length) { searchInput.focus(); return; } then use the original or trimmed value for the actual search; adjust references to search/searchInput accordingly (look for the search and searchInput variables in the handler).src/view/topRatedMovie.ts (1)
8-8: TMDB 이미지 URL이 중복되어 있습니다.이 URL(
https://media.themoviedb.org/t/p/w1920_and_h800_multi_faces)이 여기와src/view/movieList.ts(line 20-21)에 하드코딩되어 있습니다.이런 중복을 어떻게 관리하면 좋을지 고민해 보세요. 예를 들어, 상수 파일로 추출하면 어떤 이점이 있을까요?
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/view/topRatedMovie.ts` at line 8, The background image URL is hardcoded in topRatedContainer.style.backgroundImage (using movie.backdrop_path) and is duplicated in src/view/movieList.ts; extract the shared base URL into a single exported constant (e.g., TMDB_IMAGE_BASE or TMDB_IMAGE_W1920_MULTI) in a constants module and import it into both topRatedMovie.ts and movieList.ts, then construct the full URL by concatenating that constant with movie.backdrop_path so the base URL is centralized and avoid duplication.src/eventBinder.ts (1)
49-53: ESC 키 리스너가 항상 활성화됩니다.현재 구현에서는 모달이 닫혀 있어도 ESC 키 이벤트가 처리됩니다.
handleModalEscapeKeydown내부에서 모달 상태를 체크하고 있다면 문제없지만, 그렇지 않다면 불필요한 처리가 발생합니다.모달 열림 상태를 체크하는 로직이 있는지 확인해 보세요.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/eventBinder.ts` around lines 49 - 53, The global keydown listener currently calls handleModalEscapeKeydown for every Escape press even when the modal is closed; either guard the call by checking the modal open state (e.g., a boolean like isModalOpen or modalState) inside the event listener before invoking handleModalEscapeKeydown, or move the listener attachment/removal so document.addEventListener("keydown", ...) is only registered when the modal opens and removed when it closes; update references to handleModalEscapeKeydown and any modal state variable (or add one) accordingly to avoid unnecessary handling when the modal is not visible.src/view/movieList.ts (1)
20-21: TMDB 이미지 URL 중복이 URL도
topRatedMovie.ts와 중복됩니다. 상수로 추출하는 것을 고려해 보세요.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/view/movieList.ts` around lines 20 - 21, The movie poster URL string is duplicated (assigned to thumbnail.src using the literal `https://media.themoviedb.org/t/p/w220_and_h330_face`), so extract this base image URL into a shared constant (e.g., IMAGE_BASE_URL or TMDB_IMAGE_BASE) in a common module and replace the literal in this file (the thumbnail.src assignment) and in topRatedMovie.ts to import and use that constant when constructing the full poster URL (e.g., `${IMAGE_BASE_URL}${movie.poster_path}`) to remove duplication and centralize configuration.src/services/api.ts (1)
14-31: API 함수들의 중복 패턴을 고려해 보세요.네 개의 API 함수가 거의 동일한 패턴을 따릅니다:
- URL 구성
- Bearer 토큰으로 fetch
- res.ok 체크
- 에러 시 ApiError throw
이 중복을 줄이기 위한 공통 유틸리티 함수를 만들면 어떤 이점이 있을까요? 유지보수성, 일관성 측면에서 생각해 보세요.
Also applies to: 33-46, 48-67, 69-82
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/services/api.ts` around lines 14 - 31, Extract the repeated fetch-and-error-handling pattern into a single helper (e.g., requestJson or apiFetch) that accepts endpoint path/query (or full URL) and options, uses apiUrl and apiKey to compose the request and Authorization header, awaits fetch, checks res.ok, returns res.json() on success and throws ApiError(errorBody.status_message, errorBody.status_code) on failure; then refactor getPopularMovies (and the other similar functions) to call this helper with the specific path (`/movie/popular?page=${page}&language=ko-KR`) instead of duplicating URL construction, header setup, res.ok check and error parsing.cypress/e2e/movie-list-rendering.cy.ts (1)
40-46: 추가 요청이 발생하지 않음을 더 명확히 검증할 수 있습니다.현재 테스트는 아이템 수가 40개로 유지되는지만 확인합니다. 그러나 이것만으로는 추가 API 요청이 없었음을 보장하지 못합니다(요청이 갔지만 같은 데이터가 반환될 수도 있음).
테스트의 의도를 더 명확히 하려면 어떤 방법이 있을까요? 예를 들어:
cy.wait의 timeout으로 요청이 없음을 검증- Cypress spy를 사용하여 호출 횟수 확인
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@cypress/e2e/movie-list-rendering.cy.ts` around lines 40 - 46, The test should assert no additional network calls were made to the paged API rather than only checking DOM length; after you create the intercept alias (getPopularPage2) and wait for the expected request, add an assertion on the alias call history (e.g., use cy.get('@getPopularPage2.all').should('have.length', 1) or similar Cypress alias-all API) or attach a spy via cy.intercept and assert the spy was called only once so that functions/aliases like getPopularPage2 (the intercept alias) are used to verify the call count explicitly.public/styles/main.css (1)
145-179: 같은 breakpoint 규칙이 서로 덮어써서 의도가 흐려집니다.첫 번째
@media (max-width: 1023px)의min-width: 800px설정이 바로 아래 블록의min-width: 0로 모두 무효화됩니다. 한 블록으로 정리해 두면 실제 적용값을 훨씬 읽기 쉬워집니다.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@public/styles/main.css` around lines 145 - 179, The two identical media queries `@media` (max-width: 1023px) conflict and make intent unclear; consolidate them into one block so rules for .background-container and `#wrap` are defined once (remove the duplicate media rule), decide the intended min-width (either 800px or 0) and set that value consistently for .background-container and `#wrap`, then keep the padding/height/display/flex rules (.background-container, `#wrap`, `#wrap` section) in that single `@media` block to avoid later overrides and improve readability.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@cypress/e2e/movie-modal.cy.ts`:
- Around line 21-24: The test hardcodes movie ID 640146 in the cy.intercept call
which can drift from fixtures; instead, in beforeEach grab the id from
moviesFixture[0].id (assign to a local variable like movieId), use that variable
in the intercept pattern (e.g., build the URL with movieId) and ensure
movieDetailFixture is either created from or updated to reflect that same
movieId (e.g., set movieDetailFixture.id = movieId or derive movieDetailFixture
from moviesFixture[0]) so the intercepted request URL and fixture data stay in
sync; update references to cy.intercept, moviesFixture, and movieDetailFixture
accordingly.
In `@index.html`:
- Around line 258-263: The modal markup lacks ARIA dialog semantics and
accessible naming; update the element with class "modal" (id
"modal-background"/container structure) to include role="dialog" and
aria-modal="true", add a visible heading inside the "modal-container" and give
it an id (e.g., modal-title) then set aria-labelledby on the "modal" to that id
so screen readers announce the title, and make the close control (class
"close-modal", id "close-modal") an accessible button by ensuring it is a proper
<button> element with an explicit accessible name (e.g., aria-label="Close" or a
visually hidden label) and type="button"; also ensure focus is trapped into the
dialog and returned on close by wiring focus management in the modal open/close
logic referencing these IDs.
- Line 265: 빈 src 속성을 가진 <img> 태그(img 요소 with src="")가 유효하지 않으니 해당 img 요소를 제거하거나
유효한 placeholder URL을 src에 넣거나 조건부 렌더링으로 렌더 시점에만 삽입하도록 수정하세요; 구체적으로 index.html에
존재하는 문제의 img 태그를 찾아(src="") 삭제하거나 src에 투명 SVG/data URL 또는 적절한 placeholder 경로를
지정하고, 만약 동적으로 로드되어야 하면 해당 템플릿/스크립트에서 조건부로 <img> 요소를 생성하도록 변경하세요.
In `@public/styles/thumbnail.css`:
- Around line 26-29: .item-desc strong is inline so vertical margins may be
ignored; update the CSS to either make the element block-level (e.g., set
display:inline-block or display:block on .item-desc strong) or move the spacing
to the parent (e.g., apply margin/padding on .item-desc or a wrapper element) so
the intended vertical spacing takes effect; locate the .item-desc strong rule
and change it accordingly.
In `@src/constants/rating.ts`:
- Around line 1-9: Update the RATING_TEXTS and RATING_SCORES constants to fix
the typo and make them immutable: change the string "별로에요" to "별로예요" in
RATING_TEXTS, and mark both RATING_TEXTS and RATING_SCORES as read-only (e.g.,
use readonly or as const) so their values and index mapping cannot be mutated at
runtime; ensure any dependent code expects the narrowed readonly tuple types if
necessary.
In `@src/handlers/modalHandler.ts`:
- Around line 13-14: Currently openMovieModal() is called immediately after
loadMovieDetail(movieId), causing the modal to open before details are loaded;
update the flow in modalHandler so you await the promise returned by
loadMovieDetail(movieId) (or use .then/.catch) and only call openMovieModal()
after successful completion, and handle failures by not opening the modal and
showing an error (use the loadMovieDetail and openMovieModal symbols to find and
update the logic).
In `@src/main.ts`:
- Around line 19-22: The initial load (loadMovieList) may be duplicated because
initializeMovieListObserver can immediately trigger loadMovieList again; change
the flow so the observer is registered after the first-page load or make the
observer ignore the first intersection. Concretely, either call
initializeMovieListObserver() only after awaiting loadMovieList() (and
loadTopRatedMovie()), or add a flag (e.g., isInitialPageLoaded) checked in the
IntersectionObserver callback so the first automatic intersection is skipped;
update the observer setup in initializeMovieListObserver and any callback that
currently calls loadMovieList to respect this flag.
- Around line 25-38: The IntersectionObserver can trigger multiple simultaneous
loads; add a shared boolean flag (e.g., isLoading) to guard loadMovieList and
the observer callback: in initializeMovieListObserver check if isLoading before
calling loadMovieList, and inside loadMovieList set isLoading = true before
calling either loadPopularMovies or loadSearchMovies and set isLoading = false
in a finally block (or reset after await) so concurrent triggers are ignored;
optionally pause the observer (unobserve or disconnect) while loading and
re-observe the sentinel after completion to avoid duplicate requests. Ensure the
flag is exported/accessible where initializeMovieListObserver, loadMovieList,
loadPopularMovies, and loadSearchMovies are implemented.
In `@src/movieLoader.ts`:
- Around line 104-107: loadMovieDetail currently awaits getMovieDetail and calls
renderMovieDetail without error handling, so any rejection bubbles out; wrap the
fetch/render in a try/catch inside loadMovieDetail, call showError(error) (or a
clear boolean return) on failure and ensure it returns/throws a consistent
signal so callers know not to open the modal; update loadMovieDetail to catch
errors from getMovieDetail and call showError with the caught error before
exiting, keeping renderMovieDetail only in the success path.
- Around line 37-52: loadPopularMovies lacks guards for concurrent in-flight
requests and for stopping at the last page, causing duplicate or out-of-range
calls; add a reentrancy/lock check and a last-page check at the top of the
loadPopularMovies flow (use popularPageState.getPage() and
popularPageState.getTotalPages() to compare) and set an in-flight flag (e.g.,
popularPageState.setLoading(true)) before awaiting getPopularMovies({ page })
and clear it in finally (popularPageState.setLoading(false)); only call
renderMovieList, incrementPage(), and setTotalPages() on success, and when
current page >= total_pages, stop observing or disconnect the sentinel so no
further calls occur.
In `@src/services/api.ts`:
- Line 55: The URL is built by interpolating the raw query into the string
(const url) which breaks for special characters and non-ASCII input; fix by
applying the built-in encodeURIComponent to the query variable when constructing
url (e.g., use encodeURIComponent(query)) so the query parameter is safely
encoded before creating const url in the search function.
In `@src/services/storage.ts`:
- Around line 1-3: The setLocalStorage function currently lets exceptions from
window.localStorage.setItem propagate (e.g., quota/permission/SSR issues); wrap
the setItem call in a try/catch inside setLocalStorage, guard for window
availability (typeof window !== 'undefined') and return a boolean success flag
(true on success, false on failure) so callers can show fallback UI; ensure the
function signature (setLocalStorage) and all callers are updated to handle the
returned boolean.
In `@src/utils/router.ts`:
- Around line 15-19: createSearchUrl currently always adds a search param
(producing ?search=) even for blank input, which causes hasSearchParams to treat
empty searches as present; modify createSearchUrl to trim the incoming search
string and only call searchUrl.searchParams.set("search", search) when the
trimmed value is non-empty, otherwise leave the param unset (and ensure the
returned string omits an empty query string), referencing the createSearchUrl
function and the related hasSearchParams behavior.
In `@src/view/movieDetail.ts`:
- Around line 33-38: The code reads a stored rating (ratingScore) for
movieDetail.id and uses RATING_SCORES.indexOf(ratingScore) without validating
the result, which can be -1 and break fillStars/updateRatingResult; update the
logic in this block to compute const index = RATING_SCORES.indexOf(ratingScore)
and then only call fillStars(index) and updateRatingResult(index) when index >=
0 (or map invalid index to a safe default like 0), ensuring you reference
movieDetail.id, RATING_SCORES, fillStars, and updateRatingResult when making the
change.
In `@src/view/topRatedMovie.ts`:
- Around line 3-8: renderTopRatedMovie currently assumes movie.backdrop_path is
present and will set backgroundImage to an invalid URL when it's null; update
the function (referencing renderTopRatedMovie, movie.backdrop_path and
topRatedContainer) to check for a truthy movie.backdrop_path before composing
the TMDB URL and setting topRatedContainer.style.backgroundImage, and if it's
missing use a sensible fallback (e.g., a local placeholder image URL or skip
setting the background / set a neutral background color) so the UI doesn't show
".../null".
---
Outside diff comments:
In `@public/styles/main.css`:
- Around line 67-73: .background-container에 설정된 min-width: 1440px가 불필요하게 가로 스크롤을
유발하니 해당 속성을 제거하거나 반응형 규칙으로 대체하세요; 구체적으로 .background-container의 min-width를 삭제하고
대신 width를 100% 또는 max-width로 제어하거나, 1024–1439px 구간을 커버하는 미디어쿼리(예: `@media`
(max-width:1439px) ... )를 추가해 레이아웃이 깨지지 않도록 조정하고, 이미 `#wrap에` 적용된 1440px 제약은 유지하되
중복 제약을 제거하세요.
In `@src/view/movieList.ts`:
- Around line 45-50: The image path used in the HTML fragment assigned to the
constant empty (used to set noResult.innerHTML) is inconsistent with the repo's
pattern; update the src from "./public/images/mascot.png" to
"./images/mascot.png" in the template string assigned to empty so this component
(the empty constant / noResult assignment in movieList.ts) matches other image
references.
---
Nitpick comments:
In `@cypress/e2e/movie-list-rendering.cy.ts`:
- Around line 40-46: The test should assert no additional network calls were
made to the paged API rather than only checking DOM length; after you create the
intercept alias (getPopularPage2) and wait for the expected request, add an
assertion on the alias call history (e.g., use
cy.get('@getPopularPage2.all').should('have.length', 1) or similar Cypress
alias-all API) or attach a spy via cy.intercept and assert the spy was called
only once so that functions/aliases like getPopularPage2 (the intercept alias)
are used to verify the call count explicitly.
In `@public/styles/main.css`:
- Around line 145-179: The two identical media queries `@media` (max-width:
1023px) conflict and make intent unclear; consolidate them into one block so
rules for .background-container and `#wrap` are defined once (remove the duplicate
media rule), decide the intended min-width (either 800px or 0) and set that
value consistently for .background-container and `#wrap`, then keep the
padding/height/display/flex rules (.background-container, `#wrap`, `#wrap` section)
in that single `@media` block to avoid later overrides and improve readability.
In `@src/eventBinder.ts`:
- Around line 49-53: The global keydown listener currently calls
handleModalEscapeKeydown for every Escape press even when the modal is closed;
either guard the call by checking the modal open state (e.g., a boolean like
isModalOpen or modalState) inside the event listener before invoking
handleModalEscapeKeydown, or move the listener attachment/removal so
document.addEventListener("keydown", ...) is only registered when the modal
opens and removed when it closes; update references to handleModalEscapeKeydown
and any modal state variable (or add one) accordingly to avoid unnecessary
handling when the modal is not visible.
In `@src/handlers/searchHandler.ts`:
- Around line 15-17: The call to loadSearchMovies (an async function) is invoked
without awaiting its promise after navigate(createSearchUrl(baseUrl, search)),
which may cause race conditions for post-search actions; either add await before
loadSearchMovies and mark the enclosing handler function async (so subsequent
logic runs after movies finish loading), or explicitly handle the returned
promise (e.g., loadSearchMovies({ reset: true }).then(...).catch(...)) if you
intentionally want it to run concurrently—apply this change around the code that
calls createSearchUrl, navigate, and loadSearchMovies to ensure the intended
sequencing.
- Around line 9-13: The current check uses search = searchInput.value || "" and
only checks !search.length, which misses inputs that are all whitespace; update
the logic in the search handler to compute a trimmed value (e.g., const trimmed
= (searchInput.value || "").trim()) and use trimmed.length to decide early
return: if (!trimmed.length) { searchInput.focus(); return; } then use the
original or trimmed value for the actual search; adjust references to
search/searchInput accordingly (look for the search and searchInput variables in
the handler).
In `@src/services/api.ts`:
- Around line 14-31: Extract the repeated fetch-and-error-handling pattern into
a single helper (e.g., requestJson or apiFetch) that accepts endpoint path/query
(or full URL) and options, uses apiUrl and apiKey to compose the request and
Authorization header, awaits fetch, checks res.ok, returns res.json() on success
and throws ApiError(errorBody.status_message, errorBody.status_code) on failure;
then refactor getPopularMovies (and the other similar functions) to call this
helper with the specific path (`/movie/popular?page=${page}&language=ko-KR`)
instead of duplicating URL construction, header setup, res.ok check and error
parsing.
In `@src/services/dto.ts`:
- Around line 25-36: The MovieDetail interface uses an inline genres type which
can cause duplication; extract a reusable exported interface named Genre (e.g.,
interface Genre { id: number; name: string }) and replace the inline type in
MovieDetail with genres: Genre[]; update any other DTOs or view models that use
the same shape to reference Genre to centralize the definition and simplify
future changes.
In `@src/view/movieList.ts`:
- Around line 20-21: The movie poster URL string is duplicated (assigned to
thumbnail.src using the literal
`https://media.themoviedb.org/t/p/w220_and_h330_face`), so extract this base
image URL into a shared constant (e.g., IMAGE_BASE_URL or TMDB_IMAGE_BASE) in a
common module and replace the literal in this file (the thumbnail.src
assignment) and in topRatedMovie.ts to import and use that constant when
constructing the full poster URL (e.g., `${IMAGE_BASE_URL}${movie.poster_path}`)
to remove duplication and centralize configuration.
In `@src/view/movieModal.ts`:
- Around line 1-15: Update openMovieModal and closeMovieModal to manage focus:
in openMovieModal capture and store the element that triggered the modal (e.g.,
document.activeElement) before toggling, then find the modal container
(movieModal / "#modal-background"), query for the first focusable element inside
the modal (buttons, links, inputs, [tabindex], etc.) and call .focus() on it
after adding "active" and "modal-open"; in closeMovieModal remove "active" and
"modal-open" and restore focus to the previously stored trigger element (if
still in the document) and clear the stored reference; ensure these functions
handle missing focusable elements gracefully and keep the movieModal reference
to locate modal internals.
In `@src/view/topRatedMovie.ts`:
- Line 8: The background image URL is hardcoded in
topRatedContainer.style.backgroundImage (using movie.backdrop_path) and is
duplicated in src/view/movieList.ts; extract the shared base URL into a single
exported constant (e.g., TMDB_IMAGE_BASE or TMDB_IMAGE_W1920_MULTI) in a
constants module and import it into both topRatedMovie.ts and movieList.ts, then
construct the full URL by concatenating that constant with movie.backdrop_path
so the base URL is centralized and avoid 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: c59eba18-029c-45b5-abd7-77efd8012d64
📒 Files selected for processing (33)
README.mdcypress/e2e/error-handling.cy.tscypress/e2e/movie-list-rendering.cy.tscypress/e2e/movie-modal.cy.tscypress/e2e/movie-rating.cy.tscypress/e2e/movie-search.cy.tsindex.htmlpublic/styles/main.csspublic/styles/modal.csspublic/styles/search.csspublic/styles/thumbnail.csssrc/constants/rating.tssrc/eventBinder.tssrc/handlers/modalHandler.tssrc/handlers/ratingHandler.tssrc/handlers/searchHandler.tssrc/main.tssrc/movieLoader.tssrc/renders/topRatedMovie.tssrc/services/api.tssrc/services/dto.tssrc/services/storage.tssrc/states/PageState.tssrc/utils/error.tssrc/utils/router.tssrc/view/moreButton.tssrc/view/movieDetail.tssrc/view/movieList.tssrc/view/movieModal.tssrc/view/skeleton.tssrc/view/topRatedMovie.tstest/api.test.tstest/fixtures.ts
💤 Files with no reviewable changes (1)
- src/renders/topRatedMovie.ts
Comment on lines
+21
to
+24
| cy.intercept("GET", "**/movie/640146?language=ko-KR", { | ||
| statusCode: 200, | ||
| body: movieDetailFixture, | ||
| }).as("getMovieDetail"); |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Description: Check if movie ID 640146 matches the fixture
# Expected: moviesFixture should have id: 640146 as first element
echo "=== Checking moviesFixture for movie ID ==="
rg -n "640146|id:" test/fixturesRepository: woowacourse/javascript-movie-review
Length of output: 193
🏁 Script executed:
#!/bin/bash
# Find fixture files in the repository
echo "=== Finding fixture files ==="
find . -name "*fixture*" -type f 2>/dev/null | head -20Repository: woowacourse/javascript-movie-review
Length of output: 127
🏁 Script executed:
#!/bin/bash
# Check the actual test file structure
echo "=== Checking movie-modal.cy.ts file ==="
cat -n cypress/e2e/movie-modal.cy.tsRepository: woowacourse/javascript-movie-review
Length of output: 1957
🏁 Script executed:
#!/bin/bash
# Search for moviesFixture and movie IDs throughout the codebase
echo "=== Searching for moviesFixture ==="
rg -n "moviesFixture" --type ts --type js | head -20Repository: woowacourse/javascript-movie-review
Length of output: 1045
🏁 Script executed:
#!/bin/bash
# Read the fixtures file to check the first movie's ID
echo "=== Reading test/fixtures.ts ==="
cat -n test/fixtures.ts | head -50Repository: woowacourse/javascript-movie-review
Length of output: 2629
하드코딩된 영화 ID와 fixture의 연계성을 개선하세요.
현재 640146은 moviesFixture[0].id와 일치하지만, 이 값이 하드코딩되어 있으면 나중에 fixture 데이터를 변경할 때 동기화를 유지하기 어렵습니다.
다음을 고려해 보세요:
- fixture에서 동적으로 영화 ID를 가져오는 방법이 있을까요?
beforeEach에서moviesFixture[0].id를 변수로 저장한 후 재사용할 수 있을까요?- 현재 구조에서 테스트의 의도는 무엇이며, 그 의도를 유지하면서 더 유지보수하기 좋은 방식은 무엇일까요?
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@cypress/e2e/movie-modal.cy.ts` around lines 21 - 24, The test hardcodes movie
ID 640146 in the cy.intercept call which can drift from fixtures; instead, in
beforeEach grab the id from moviesFixture[0].id (assign to a local variable like
movieId), use that variable in the intercept pattern (e.g., build the URL with
movieId) and ensure movieDetailFixture is either created from or updated to
reflect that same movieId (e.g., set movieDetailFixture.id = movieId or derive
movieDetailFixture from moviesFixture[0]) so the intercepted request URL and
fixture data stay in sync; update references to cy.intercept, moviesFixture, and
movieDetailFixture accordingly.
Comment on lines
+258
to
+263
| <div class="modal-background" id="modal-background"> | ||
| <div class="modal"> | ||
| <button class="close-modal" id="close-modal"> | ||
| <img src="./images/modal_button_close.png" /> | ||
| </button> | ||
| <div class="modal-container"> |
There was a problem hiding this comment.
모달 접근성 시맨틱이 부족해 스크린리더 사용성이 떨어집니다.
Line 258-263, 268 구간에서 대화상자 역할(role="dialog", aria-modal)과 제목 연결, 닫기 버튼의 접근 가능한 이름을 보강해 주세요. 현재 구조는 보조기기에서 맥락 파악이 어렵습니다.
개선 예시
-<div class="modal-background" id="modal-background">
- <div class="modal">
- <button class="close-modal" id="close-modal">
- <img src="./images/modal_button_close.png" />
+<div class="modal-background" id="modal-background">
+ <div class="modal" role="dialog" aria-modal="true" aria-labelledby="modal-title">
+ <button class="close-modal" id="close-modal" aria-label="모달 닫기">
+ <img src="./images/modal_button_close.png" alt="" />
</button>
@@
- <h2></h2>
+ <h2 id="modal-title"></h2>Also applies to: 268-268
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@index.html` around lines 258 - 263, The modal markup lacks ARIA dialog
semantics and accessible naming; update the element with class "modal" (id
"modal-background"/container structure) to include role="dialog" and
aria-modal="true", add a visible heading inside the "modal-container" and give
it an id (e.g., modal-title) then set aria-labelledby on the "modal" to that id
so screen readers announce the title, and make the close control (class
"close-modal", id "close-modal") an accessible button by ensuring it is a proper
<button> element with an explicit accessible name (e.g., aria-label="Close" or a
visually hidden label) and type="button"; also ensure focus is trapped into the
dialog and returned on close by wiring focus management in the modal open/close
logic referencing these IDs.
| </button> | ||
| <div class="modal-container"> | ||
| <div class="modal-image"> | ||
| <img src="" /> |
There was a problem hiding this comment.
빈 img src 값은 유효하지 않습니다.
Line 265의 src=""는 마크업 오류이며 불필요한 요청/깨진 이미지 상태를 만들 수 있습니다. 초기 placeholder src를 넣거나 렌더 시점에만 img를 주입하는 방식으로 정리해 주세요.
수정 예시
-<img src="" />
+<img
+ src="data:image/gif;base64,R0lGODlhAQABAIAAAAAAAP///ywAAAAAAQABAAACAUwAOw=="
+ alt="영화 포스터"
+/>📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| <img src="" /> | |
| <img | |
| src="data:image/gif;base64,R0lGODlhAQABAIAAAAAAAP///ywAAAAAAQABAAACAUwAOw==" | |
| alt="영화 포스터" | |
| /> |
🧰 Tools
🪛 HTMLHint (1.9.2)
[error] 265-265: 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` at line 265, 빈 src 속성을 가진 <img> 태그(img 요소 with src="")가 유효하지 않으니
해당 img 요소를 제거하거나 유효한 placeholder URL을 src에 넣거나 조건부 렌더링으로 렌더 시점에만 삽입하도록 수정하세요;
구체적으로 index.html에 존재하는 문제의 img 태그를 찾아(src="") 삭제하거나 src에 투명 SVG/data URL 또는 적절한
placeholder 경로를 지정하고, 만약 동적으로 로드되어야 하면 해당 템플릿/스크립트에서 조건부로 <img> 요소를 생성하도록 변경하세요.
Comment on lines
+26
to
+29
| .item-desc strong { | ||
| margin: 5px 0; | ||
| font-size: 1.1rem; | ||
| } |
There was a problem hiding this comment.
strong 간격 스타일이 실제로 적용되지 않을 수 있습니다.
Line 26-29에서 strong은 inline이라 상하 margin 의도가 깨질 수 있습니다. 블록화하거나 부모 레이아웃으로 간격을 주는 쪽이 안전합니다.
수정 예시
.item-desc strong {
+ display: block;
margin: 5px 0;
font-size: 1.1rem;
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| .item-desc strong { | |
| margin: 5px 0; | |
| font-size: 1.1rem; | |
| } | |
| .item-desc strong { | |
| display: block; | |
| margin: 5px 0; | |
| font-size: 1.1rem; | |
| } |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@public/styles/thumbnail.css` around lines 26 - 29, .item-desc strong is
inline so vertical margins may be ignored; update the CSS to either make the
element block-level (e.g., set display:inline-block or display:block on
.item-desc strong) or move the spacing to the parent (e.g., apply margin/padding
on .item-desc or a wrapper element) so the intended vertical spacing takes
effect; locate the .item-desc strong rule and change it accordingly.
Comment on lines
+1
to
+9
| export const RATING_TEXTS = [ | ||
| "최악이에요", | ||
| "별로에요", | ||
| "보통이에요", | ||
| "재밌어요", | ||
| "명작이에요", | ||
| ]; | ||
|
|
||
| export const RATING_SCORES = ["2", "4", "6", "8", "10"]; |
There was a problem hiding this comment.
평점 상수에 사용자 문구 오탈자와 불변성 리스크가 있습니다.
- Line 3
"별로에요"는 UI 문구 기준으로"별로예요"가 더 자연스럽습니다. RATING_TEXTS/RATING_SCORES가 가변 배열이라 런타임에서 값이 바뀌면 인덱스 매핑이 깨질 수 있습니다.readonly(또는as const)로 고정하는 방향을 고려해 주세요.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/constants/rating.ts` around lines 1 - 9, Update the RATING_TEXTS and
RATING_SCORES constants to fix the typo and make them immutable: change the
string "별로에요" to "별로예요" in RATING_TEXTS, and mark both RATING_TEXTS and
RATING_SCORES as read-only (e.g., use readonly or as const) so their values and
index mapping cannot be mutated at runtime; ensure any dependent code expects
the narrowed readonly tuple types if necessary.
| query: string; | ||
| }): Promise<Movies> => { | ||
| const url = `${apiUrl}/search/movie?page=${page}&query=${query}`; | ||
| const url = `${apiUrl}/search/movie?page=${page}&query=${query}&language=ko-KR`; |
There was a problem hiding this comment.
검색어 URL 인코딩이 필요합니다.
query 파라미터가 URL에 직접 삽입되고 있습니다. 사용자가 &, #, ? 같은 특수 문자나 한글을 포함한 검색어를 입력하면 URL이 깨질 수 있습니다.
이 문제를 해결하려면 어떤 JavaScript 내장 함수를 사용해야 할까요?
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/services/api.ts` at line 55, The URL is built by interpolating the raw
query into the string (const url) which breaks for special characters and
non-ASCII input; fix by applying the built-in encodeURIComponent to the query
variable when constructing url (e.g., use encodeURIComponent(query)) so the
query parameter is safely encoded before creating const url in the search
function.
Comment on lines
+1
to
+3
| export const setLocalStorage = (key: string, value: string) => { | ||
| window.localStorage.setItem(key, value); | ||
| }; |
There was a problem hiding this comment.
localStorage.setItem 예외를 처리하지 않아 사용자 액션이 끊길 수 있습니다.
Line 2는 브라우저 정책/용량 이슈에서 예외를 던질 수 있습니다. 현재는 전파되어 별점 클릭 흐름 전체가 중단될 수 있어요.
저장 실패를 try/catch로 흡수하고, 호출부가 fallback UI를 처리할 수 있게 성공/실패 신호를 넘기는 구조를 검토해 주세요.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/services/storage.ts` around lines 1 - 3, The setLocalStorage function
currently lets exceptions from window.localStorage.setItem propagate (e.g.,
quota/permission/SSR issues); wrap the setItem call in a try/catch inside
setLocalStorage, guard for window availability (typeof window !== 'undefined')
and return a boolean success flag (true on success, false on failure) so callers
can show fallback UI; ensure the function signature (setLocalStorage) and all
callers are updated to handle the returned boolean.
Comment on lines
+15
to
+19
| export const createSearchUrl = (baseUrl: string, search: string) => { | ||
| const searchUrl = new URL(baseUrl, window.location.origin); | ||
| searchUrl.searchParams.set("search", search); | ||
| return `${searchUrl.pathname}${searchUrl.search}`; | ||
| }; |
There was a problem hiding this comment.
빈 검색어도 search 파라미터를 강제로 붙여 빈 검색 플로우가 실행될 수 있습니다.
Line 17에서 search가 빈 문자열이어도 ?search=가 생성됩니다. 이 경우 같은 파일의 hasSearchParams(Line 10-13)가 true로 판단해, 의도치 않게 검색 로직이 타는 케이스가 생깁니다.
입력값을 trim한 뒤 빈 값이면 파라미터를 제거/미부착하는 분기를 두는 게 안전해 보여요.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/utils/router.ts` around lines 15 - 19, createSearchUrl currently always
adds a search param (producing ?search=) even for blank input, which causes
hasSearchParams to treat empty searches as present; modify createSearchUrl to
trim the incoming search string and only call
searchUrl.searchParams.set("search", search) when the trimmed value is
non-empty, otherwise leave the param unset (and ensure the returned string omits
an empty query string), referencing the createSearchUrl function and the related
hasSearchParams behavior.
src/view/movieDetail.ts
Outdated
Comment on lines
+33
to
+38
| const ratingScore = window.localStorage.getItem(String(movieDetail.id)); | ||
| if (!ratingScore) return; | ||
|
|
||
| const index = RATING_SCORES.indexOf(ratingScore); | ||
| fillStars(index); | ||
| updateRatingResult(index); |
There was a problem hiding this comment.
저장된 별점 값이 예상 범위를 벗어나면 UI가 깨집니다.
localStorage 값이 RATING_SCORES에 없으면 indexOf()가 -1을 반환하고, 그 값이 그대로 updateRatingResult()로 들어갑니다. 그러면 평점 문구와 점수가 undefined로 렌더될 수 있으니, 인덱스 유효성부터 걸러 주는 편이 안전합니다.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/view/movieDetail.ts` around lines 33 - 38, The code reads a stored rating
(ratingScore) for movieDetail.id and uses RATING_SCORES.indexOf(ratingScore)
without validating the result, which can be -1 and break
fillStars/updateRatingResult; update the logic in this block to compute const
index = RATING_SCORES.indexOf(ratingScore) and then only call fillStars(index)
and updateRatingResult(index) when index >= 0 (or map invalid index to a safe
default like 0), ensuring you reference movieDetail.id, RATING_SCORES,
fillStars, and updateRatingResult when making the change.
Comment on lines
+3
to
+8
| export const renderTopRatedMovie = (movie: Movie) => { | ||
| const topRatedContainer = document.querySelector<HTMLDivElement>( | ||
| ".background-container", | ||
| ); | ||
| if (!topRatedContainer) return; | ||
| topRatedContainer.style.backgroundImage = `url(${`https://media.themoviedb.org/t/p/w1920_and_h800_multi_faces` + movie.backdrop_path})`; |
There was a problem hiding this comment.
backdrop_path가 없는 경우를 처리하고 있나요?
TMDB API에서 일부 영화는 backdrop_path가 null일 수 있습니다. 현재 코드에서 movie.backdrop_path가 null이면 이미지 URL이 ...multi_faces/null이 됩니다.
이 엣지 케이스를 어떻게 처리할 수 있을지 생각해 보세요.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/view/topRatedMovie.ts` around lines 3 - 8, renderTopRatedMovie currently
assumes movie.backdrop_path is present and will set backgroundImage to an
invalid URL when it's null; update the function (referencing
renderTopRatedMovie, movie.backdrop_path and topRatedContainer) to check for a
truthy movie.backdrop_path before composing the TMDB URL and setting
topRatedContainer.style.backgroundImage, and if it's missing use a sensible
fallback (e.g., a local placeholder image URL or skip setting the background /
set a neutral background color) so the UI doesn't show ".../null".
JUDONGHYEOK
requested changes
Apr 13, 2026
JUDONGHYEOK
left a comment
JUDONGHYEOK
left a comment
There was a problem hiding this comment.
안녕하세요 두부!!! 2단계 미션도 수고많으셨습니다!
파일을 역할별로 잘 나누고 PageState 캡슐화, ApiError 커스텀 에러, 이벤트 핸들러 네이밍 분리 등 설계 의도가 곳곳에 보여서 좋습니다. 다만 movieLoader가 DOM을 직접 조작하고, movieDetail(view)이 localStorage를 읽고, ratingHandler가 DOM 쿼리, 저장까지 하는 등 레이어 간 경계가 군데군데 흐려져 있어서, 데이터, 상태, 뷰를 한 방향 흐름을 좀 더 의식하면 각 모듈의 테스트성과 변경 용이성이 크게 올라갈 것 같습니다!!
Q&A
1 . IntersectionObserver를 사용할 때 빠른 스크롤 상황을 어떻게 안정적으로 처리할 수 있나요?
가장 간단한 방법은 isLoading 플래그를 두는 겁니다. 해당 상태를 두면 이전 요청이 끝나기 전에 다시 트리거되는 걸 막을 수 있을 것 같습니다. 또 요청이 누락되는 건 보통 응답이 오기 전에 sentinel이 다시 뷰포트를 벗어났다 들어오는 경우인데, 로딩 완료 후 sentinel이 여전히 뷰포트에 있으면 다시 한 번 체크하는 로직을 넣으면 더 견고해질 것 같습니다.
- 모달은 정적 DOM으로 두는 방식과 템플릿/동적 생성 방식 중 어떤 방향이 더 적절한가요?
모달이 하나인 지금 상황에서는 정적 DOM 방식이 맞는 선택이라고 생각합니다! 단순하고 이벤트 바인딩도 한 번이면 되니까요. 다만 두부님도 느끼셨듯이 clearMovieDetail 같은 초기화 책임이 생기는 게 단점인데, 이건 열 때 채우고 닫을 때 비운다는 규칙만 명확히 지키면 관리할 수 있는 수준이라고 생각해요
템플릿 기반으로 전환하는 시점은 모달 종류가 2개 이상이 되거나, 모달 내부 상태가 복잡해져서 초기화 누락 버그가 반복될 때가 적절할 것 같아요. 지금 단계에서 미리 바꿀 필요는 없고, 현재 구조에서 한 가지 개선한다면 모달 내부 요소를 채우는 로직과 비우는 로직이 대칭이 되도록 관리하는 것 정도면 충분합니다.
- 브라우저 저장소를 사용하는 기능은 테스트 범위를 어디까지 가져가는 것이 좋을까요?
현재 접근이 적절하다고 생각해요. E2E 테스트에서는 사용자가 보는 결과를 검증하는 게 맞는데요. 별점을 매기고 → 모달을 닫고 → 다시 열었을 때 별점이 유지되는지, 이게 사용자 관점의 시나리오이고 E2E의 목적이 된다고 생각합니다!
- 구조 분리와 이벤트 바인딩 위치에 대한 기준이 궁금합니다.
지금처럼 역할 기준(api, view, handler, state)으로 나누는 건 좋은 시작이라고 생각해요! 다만 두부님이 느끼신 것처럼 movieLoader에 책임이 몰리는 건, 역할 기준 분리의 전형적인 한계입니다. 데이터를 가져온다와 화면에 반영한다가 결국 한 흐름 안에서 일어나니까요.
저는 개인적으로 한 모듈을 수정할 이유가 하나인가 즉 SRP를 먼저 봅니다. movieLoader가 수정되는 이유를 대강 파악해보자면요 API 스펙 변경, DOM 구조 변경, 페이지네이션 로직 변경 세 가지나 됩니다. 요런 기준으로 판단하면 좀 더 확실하게 파악이 되더라구요.
이벤트 바인딩 같은경우 답이 있는 것은 아니지만 현재구조도 적당하다고 봅니다. 지금 모달이나 별점 영역은 정적 DOM인데 페이지 로드 시점에 이미 HTML에 존재하니까 한 번만 바인딩하면 되고, 그게 main에서 호출하는 지금 방식이니 이벤트 등록 시점과 DOM 존재 시점이 일치하니까 문제가 없습니다. 다만 프로젝트가 비대해지고 컴포넌트의 분리가 많이 필요해지고 DOM이 동적으로 생성되고 삭제될 때는 다른 방식이 필요해질 것 같습니다.
2단계 미션도 너무 수고많으셨고 코멘트 확인해주시고 재요청 주시면 감사하겠습니다!
| const movieId = movieItem.dataset.movieId; | ||
| if (!movieId) return; | ||
|
|
||
| loadMovieDetail(movieId); |
There was a problem hiding this comment.
여기 await은 필요없을까요??네트워크가 느린 환경에서 사용자가 빈 모달을 보게될 것 같아서요!
| @@ -0,0 +1,7 @@ | |||
| export const setLocalStorage = (key: string, value: string) => { | |||
There was a problem hiding this comment.
window.localStorage를 그대로 감싼 수준이라 추상화의 이점을 더 할 수 있을 것 같은데요! 저장소가 sessionStorage나 서버 API로 바뀌면 호출부를 전부 수정해야 합니다. 인터페이스를 정의하고 구현체를 주입받는 구조로 만들면 OCP를 지킬 수 있을 것 같은데 어떻게 생각하세요?
Comment on lines
+75
to
+77
| const movieListTitle = document.querySelector("#movie-list-title"); | ||
| if (!movieListTitle) return null; | ||
| movieListTitle.textContent = `"${search}" 검색 결과`; |
There was a problem hiding this comment.
movieLoader는 이름 그대로 데이터를 로드하는 역할인데 DOM을 직접 건드리고 있어요. view 쪽 함수로 분리해서 데이터 레이어와 뷰 레이어의 경계를 명확히 하면 어떨까요?
Comment on lines
+56
to
+66
| const res = await fetch(url, { | ||
| method: "get", | ||
| headers: { | ||
| Authorization: `Bearer ${apiKey}`, | ||
| }, | ||
| }); | ||
|
|
||
| if (res.ok) return await res.json(); | ||
|
|
||
| const errorBody = await res.json(); | ||
| throw new ApiError(errorBody.status_message, errorBody.status_code); |
| loadTopRatedMovie(); | ||
| loadPopularMovies(); | ||
| }); | ||
| loadMovieList(); |
There was a problem hiding this comment.
무한스크롤에서는 마지막페이지를 확인하고 있지 않은 것 같은데 맞을까요?
| import { MovieDetail } from "../services/dto"; | ||
| import { getLocalStorage } from "../services/storage"; | ||
|
|
||
| export const renderMovieDetail = (movieDetail: MovieDetail) => { |
There was a problem hiding this comment.
한 함수가 DOM 세팅, 데이터 포맷팅, localStorage 조회, 별점 복원까지 전부 담당하고 있는 것 같은데요! 역할에 맞게 함수를 조금 분리해보면 어떨까요?
|
|
||
| if (modalImage) { | ||
| modalImage.src = | ||
| `https://media.themoviedb.org/t/p/w300_and_h450_face` + |
There was a problem hiding this comment.
요런 하드코딩된 이미지 url은 별도로 관리해도 좋지 않을까요? w,h만 별도로 주입받는 형태로 만들어도 좋을 것 같아요!
Comment on lines
+37
to
+52
| export const loadPopularMovies = async () => { | ||
| try { | ||
| renderSkeleton(); | ||
| const page = popularPageState.getPage() + 1; | ||
| const movies = await getPopularMovies({ page }); | ||
|
|
||
| if (movies) { | ||
| renderMovieList(movies); | ||
| popularPageState.incrementPage(); | ||
| popularPageState.setTotalPages(movies.total_pages); | ||
| } | ||
| } catch (e) { | ||
| showError(e); | ||
| } finally { | ||
| removeSkeleton(); | ||
| } |
Comment on lines
+49
to
+53
| document.addEventListener("keydown", (event) => { | ||
| if (event.key === "Escape") { | ||
| handleModalEscapeKeydown(); | ||
| } | ||
| }); |
There was a problem hiding this comment.
모달이 닫혀 있어도 ESC를 누르면 핸들러가 실행됩니다. 지금은 큰 문제 없지만, 모달이 열려 있을 때만 동작하도록 가드를 추가하는 게 의도에 맞고, 나중에 다른 ESC 동작이 추가되면 충돌할 수 있어요.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
🎯 미션 소개
🕵️ 셀프 리뷰(Self-Review)
제출 전 체크 리스트
리뷰 요청 & 논의하고 싶은 내용
안녕하세요 동키콩!
이번 미션에서는 무한 스크롤과 모달/별점 기능을 구현하면서 구조 분리와 이벤트 처리 방식, DOM 탐색 범위, E2E 테스트 범위까지 함께 고민해보려고 했습니다. 구현하면서 고민했던 지점과 리뷰를 통해 의견을 듣고 싶은 부분들을 아래에 정리해두었는데 편하게 코멘트 주시면 감사하겠습니다. 이번 리뷰도 잘 부탁드립니다 😊
1) 이번 단계에서 가장 많이 고민했던 문제와 해결 과정에서 배운 점
IntersectionObserver 중복 로드 문제
브라우저 렌더링 최적화를 주제로 한 원정대 활동에서 무한 스크롤을 구현하는 방법에 대해 고민한 적이 있습니다. 이때 스크롤 위치를 확인하기 위해
getBoundingClientRect를 반복적으로 사용하는 것은 리플로우가 자주 발생해서 좋지 않다는 점을 알게되었습니다. 그래서 이번 미션에서 브라우저가 알아서 요소를 감지하는IntersectionObserver를 사용했습니다.영화 목록 리스트 아래에
sentinel요소를 두고 요소가 감지되면 다음 영화 목록을 불러오도록 구현했습니다. 그런데sentinel요소가 감지될 때마다 로드 함수가 두 번씩 호출되는 문제가 생겼습니다.확인해보니 옵저버는 감지하는 요소가 들어올 때와 나갈 때 둘 다 감지하고 등록된 함수를 실행하는 구조였습니다. 그래서
sentinel요소가 들어올때 로드 함수를 실행하도록 조건을 추가해서 문제를 해결했습니다.이벤트 의도를 드러내는 핸들러
이벤트를 구현하면서 서로 다른 이벤트이지만 동일한 동작을 수행해야 하는 경우가 있었습니다.
예를 들어 모달을 닫는 동작은 닫기 버튼을 클릭했을 때, ESC 키를 눌렀을 때, 모달 바깥을 클릭했을 때 모두 발생합니다. 처음에는 같은 동작이라고 생각해 하나의 핸들러를 세 이벤트에서 함께 사용했습니다.
하지만 이벤트 등록 코드가 많아지면서 ESC 키 이벤트나 모달 바깥 클릭 이벤트에서 `handleModalCloseButtonClick 이 실행되는 구조가 어색하게 느껴졌습니다. 같은 동작을 수행하더라도 이벤트가 발생한 맥락과 핸들러 이름이 맞지 않으면 코드의 의도가 분명하게 드러나지 않는다고 생각했습니다.
그래서 각 이벤트마다 역할이 드러나는 별도의 핸들러를 두고 실제 모달을 닫는 함수를 참조하도록 했습니다. 이렇게 분리하니 어떤 이벤트가 어떤 동작을 트리거하는지 더 명확해졌고 이후 다른 이벤트에 추가 동작이 필요하더라도 유연하게 확장할 수 있을 것 같습니다.
DOM 탐색 범위를 제한해 책임을 명확히 하기
모달 기능 구현에서
document.querySelector로 필요한 요소들을 찾아 값을 채웠습니다. 그런데 구조가 커질수록 모달 내부 요소를 전역에서 찾는 것이 취약할 수 있다는 생각이 들었습니다. 별점이나 제목, 줄거리처럼 모달 안에서만 사용되는 요소도 전역에서 조회하게 되면 나중에 같은 클래스명이 다른 영역에 생겼을 때 의도하지 않은 요소를 참조할 수 있습니다.그래서 모달 루트 요소를 먼저 찾고 그 안에서
querySelector를 사용하는 방식으로 수정했습니다. 이 방식은 특정 UI 내부에서만 사용하는 요소를 해당 UI 기준으로 탐색할 수 있어 전역 탐색을 줄이고 구조가 변경되더라도 더 안전하게 동작할 수 있을 것 같습니다.2) 이번 리뷰를 통해 논의하고 싶은 부분
1 . IntersectionObserver를 사용할 때 빠른 스크롤 상황을 어떻게 안정적으로 처리할 수 있나요?
현재는
sentinel요소가 화면에 들어오면 다음 페이지를 불러오는 구조로 구현했습니다.하지만 빠르게 스크롤할 경우 요청이 누락되는 상황이 있었는데 이런 경우 보통 어떤 보호 장치를 두는지 궁금합니다.
2. 모달은 정적 DOM으로 두는 방식과 템플릿/동적 생성 방식 중 어떤 방향이 더 적절한가요?
이번 구현에서는 모달이 하나였기 때문에 정적으로 두고 열릴 때마다 내용을 채우고 닫힐 때 일부 상태를 초기화하는 방식으로 구현했습니다.
이 방식은 단순하게 시작하기 좋았지만 반대로 clear 책임이 생길수 있습니다.
반대로 템플릿 기반으로 매번 새로 생성하면 상태 초기화는 자연스럽지만 DOM 생성과 이벤트 연결을 다시 해야 한다는 부담이 있다고 생각합니다.
모달이 하나뿐인 상황에서도 템플릿 기반으로 관리하는 것이 더 나은 선택인지, 아니면 현재처럼 정적인 DOM을 두고 필요한 부분만 초기화하는 방식이 오히려 더 단순하고 적절한지 궁금합니다. 또한 이후 여러 모달이 생길 가능성까지 고려한다면 어느 시점부터 구조를 변경하는 것이 좋나요
3. 브라우저 저장소를 사용하는 기능은 테스트 범위를 어디까지 가져가는 것이 좋을까요?
별점 기능을 테스트하면서 현재는 별점을 저장한 뒤 모달을 닫았다가 다시 열었을 때, 이전에 선택한 값이 그대로 표시되는지를 기준으로 테스트를 작성하고 있습니다. 하지만 이 테스트는 localStorage라는 브라우저 저장소 상태에 의존하고 있는데요. 이런 경우에도 E2E 테스트에서 실제 브라우저 저장소를 그대로 사용하는 것이 적절한지 궁금합니다. 또한 이와 같은 기능은 UI 결과만 검증하면 충분한지 아니면 저장 자체도 별도로 확인해야 하는지에 대해서도 의견을 듣고 싶습니다.
4. 구조 분리와 이벤트 바인딩 위치에 대한 기준이 궁금합니다.
현재는 API 호출, 뷰, 이벤트 등 역할을 기준으로 구조를 분리하려고 했습니다. 그런데 구현을 진행하면서 API 호출과 뷰를 함께 관리하는 로더 쪽에 책임이 점점 커지고 있다고 느꼈습니다. 그러다 보니 지금처럼 역할 기준으로 구조를 나누는 방식이 적절한지, 아니면 사용자 인터랙션을 기준으로 구조를 나누는 방식이 더 나은지 고민하게 되었습니다. 리뷰어님께서는 어떤 방식을 더 선호하시는지 궁금합니다. 또한 구현하시다가 특정 부분의 책임이 너무 커진다고 느껴질 때 어떤 기준으로 분리하거나 구조를 조정하시는지도 궁금합니다.
이벤트 등록과 관련해서는 main.ts에서 미리 바인딩하는 방식이 적절한지 혹은 렌더 함수 내부에서 처리하는 것이 더 자연스러운지도 함께 의견을 듣고 싶습니다!
✅ 리뷰어 체크 포인트