[2단계 - 영화 목록 불러오기] 라바 미션 제출합니다. #303
[2단계 - 영화 목록 불러오기] 라바 미션 제출합니다. #303kojesung wants to merge 104 commits intowoowacourse:kojesungfrom
Conversation
Co-authored-by: 송아영 <gustn99@users.noreply.github.com>
Co-authored-by: 송아영 <gustn99@users.noreply.github.com>
Co-authored-by: 송아영 <gustn99@users.noreply.github.com>
Co-authored-by: 송아영 <gustn99@users.noreply.github.com>
Co-authored-by: 송아영 <gustn99@users.noreply.github.com>
Co-authored-by: 송아영 <gustn99@users.noreply.github.com>
Co-authored-by: 송아영 <gustn99@users.noreply.github.com>
Co-authored-by: 송아영 <gustn99@users.noreply.github.com>
Co-authored-by: 송아영 <gustn99@users.noreply.github.com>
Co-authored-by: 송아영 <gustn99@users.noreply.github.com>
Co-authored-by: 송아영 <gustn99@users.noreply.github.com>
Co-authored-by: 송아영 <gustn99@users.noreply.github.com>
Co-authored-by: 송아영 <gustn99@users.noreply.github.com>
Co-authored-by: 송아영 <gustn99@users.noreply.github.com>
Co-authored-by: 송아영 <gustn99@users.noreply.github.com>
Co-authored-by: 송아영 <gustn99@users.noreply.github.com>
Co-authored-by: 송아영 <gustn99@users.noreply.github.com>
Co-authored-by: 송아영 <gustn99@users.noreply.github.com>
Co-authored-by: 송아영 <gustn99@users.noreply.github.com>
Co-authored-by: 송아영 <gustn99@users.noreply.github.com>
Co-authored-by: 송아영 <gustn99@users.noreply.github.com>
Co-authored-by: 송아영 <gustn99@users.noreply.github.com>
Co-authored-by: 송아영 <gustn99@users.noreply.github.com>
Co-authored-by: 송아영 <gustn99@users.noreply.github.com>
Co-authored-by: 송아영 <gustn99@users.noreply.github.com>
Co-authored-by: 송아영 <gustn99@users.noreply.github.com>
Co-authored-by: 송아영 <gustn99@users.noreply.github.com>
Co-authored-by: 송아영 <gustn99@users.noreply.github.com>
Co-authored-by: 송아영 <gustn99@users.noreply.github.com>
Co-authored-by: 송아영 <gustn99@users.noreply.github.com>
Summary by CodeRabbit릴리스 노트
Walkthrough이 PR은 영화 상세 정보를 표시하는 모달 UI와 별점 기능을 구현하고, DOM 렌더링 로직을 절차형 함수에서 클래스 기반 UI 컴포넌트(MainUI, SearchUI, ModalUI)로 리팩토링합니다. IntersectionObserver를 활용한 무한 스크롤 페이지네이션을 추가하며, 기존의 이벤트 핸들러와 렌더링 함수를 제거합니다. RatingRepository를 통한 localStorage 기반 별점 저장, HTML 구조 업데이트, 태블릿 및 모바일을 위한 반응형 CSS 미디어 쿼리를 포함하고, 모달 상호작용과 스켈레톤 로딩 상태를 검증하는 포괄적인 Cypress E2E 테스트를 추가합니다. Estimated code review effort🎯 4 (Complex) | ⏱️ ~75 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: 20
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@index.html`:
- Around line 221-223: The close button (<button class="close-modal"
id="closeModal">) lacks an accessible name; update the element to provide one by
adding an appropriate aria-label (e.g., aria-label="Close modal") or visible
text inside the button, ensuring the label describes its action (closing the
modal) while keeping the existing <img src="./images/modal_button_close.png">
for visuals.
- Around line 284-288: 요소 id="modal-detail"에 class 속성이 중복되어 첫번째 클래스 값("detail")가
덮어씌워지는 문제입니다; id="modal-detail"를 가진 <p> 엘리먼트를 찾아 class 속성들을 하나로 합쳐서 예: "detail
modal-skeleton modal-skeleton-detail"처럼 단일 class 속성으로 통합하고 중복된 class 속성 선언을
제거하세요.
- Around line 225-226: HTML에 빈 src="" 속성이 있어 브라우저의 불필요한 문서 재요청을 유발하므로 <img>
엘리먼트의 src 속성(아이디 "modal-image" 또는 클래스 "modal-image modal-skeleton-image")을 마크업에서
제거하고, ModalUI.ts의 기존 동적 로직을 사용해 이미지가 준비된 시점에만 JavaScript에서 src를 설정하도록 변경하세요;
마크업에서는 alt는 유지하고 src는 삭제한 뒤 ModalUI.ts의 이미지 로딩/설정 코드에서
document.getElementById("modal-image") 또는 해당 셀렉터로 엘리먼트를 찾아 src를 할당하도록 수정하면 됩니다.
In `@README.md`:
- Line 145: Fix the typo in the README by changing the term "localsStorage" to
"localStorage" wherever it appears (notably the line that currently reads "API
연동 고려한 localsStorage 활용") so the document consistently and correctly uses the
web-standard identifier localStorage for clarity and searchability.
In `@src/apis/movie/api.ts`:
- Around line 24-29: The belongs_to_collection property in the movie API type is
declared as an object but can be null (per cypress/fixtures/movieDetail.json),
so update the type definition for belongs_to_collection in src/apis/movie/api.ts
to explicitly allow null (e.g., union with null) and then update any consumers
of belongs_to_collection to handle the null branch (check for null before
accessing id/name/poster_path/backdrop_path) so the runtime branching is
reflected in the type system.
In `@src/dom/render/MainUI.ts`:
- Around line 49-58: The load() method currently leads to duplicated main
thumbnails because the UI treats incoming movie data as append-only; fix by
resetting the movie list before fetching or by replacing (not appending) when
setting the "data" state: in load() call setMainState({ type: "loading", movies:
[] }) (or otherwise ensure setMainState({ type: "data", movies:
popularMovies.results, isLastPage }) performs a replace rather than merge), and
apply the same replace-vs-append change for the other data-setting path handled
around the render/update code referenced (the logic around setMainState usage at
the other location). Ensure the unique symbols load() and setMainState are the
points you modify so re-entry clears previous items instead of accumulating
them.
- Line 45: The error message element (this.errorMessageContent) is being hidden
even when the error container is shown, so update the error-rendering logic
(e.g., the method that renders error state / showError / renderError) to manage
container visibility and message visibility separately: when showing an error,
ensure you remove the "hidden" class from this.errorMessageContent (and only add
"hidden" when clearing the error), and when hiding the error ensure you hide
both container and message; conversely when showing the container do not leave
the message element hidden—adjust the classList.add/classList.remove calls
accordingly for the container and this.errorMessageContent so they are toggled
independently.
- Line 65: The additional-page request omits the language parameter causing
inconsistent language across pages; update the call to getPopularMovies in
MainUI (where getPageParam() + 1 is used) to include the same language option
used for the initial load (reuse the shared request options or a helper that
constructs options, e.g., the existing initial options factory or a
getRequestOptions/getPopularMoviesOptions function) so both initial and
subsequent requests pass identical { language, page } parameters; ensure you
locate and reuse the same symbol that builds initial options rather than
hardcoding language in this call.
In `@src/dom/render/ModalUI.ts`:
- Around line 125-136: When load(movieId) starts, ensure late responses can't
overwrite newer requests by either cancelling the previous fetch via an
AbortController stored on the instance (e.g. this.currentLoadController: abort
the previous controller before creating a new one and pass its signal into
getMovieDetail) or by stamping each request with a token/lastRequestedMovieId
(set this.currentMovieId = movieId at start, capture it in a local variable or
increment a request counter, and before any call to this.#setModalState({ type:
"data", movie }) or error handling verify the token matches the latest request);
apply the guard in the load() flow around getMovieDetail and both the success
and catch branches so only the most recent movieId updates the modal.
- Around line 33-38: The star rating images in starEls (elements with ids like
`modal-my-rate-star-{i}`) are only clickable and not keyboard accessible; make
the control focusable and respond to keyboard activation by either replacing the
IMG elements with actual <button> elements or adding tabindex="0",
role="button", appropriate ARIA state (e.g., aria-pressed/aria-label) and a
keydown handler that treats Enter and Space the same as the click handler;
update the code that attaches the click listener (the same logic used for
elements referenced by starEls and the analogous block at 62-71) to also attach
a keydown listener which calls the same selection function so keyboard users can
focus and activate stars and screen readers get accessible semantics.
- Around line 121-130: The modal background visibility is toggled but the page
scroll lock is never applied; update ModalUI.hide() to remove the body class
"modal-open" and update ModalUI.load(movieId) to add
document.body.classList.add("modal-open") when showing the modal so the CSS rule
body.modal-open { overflow: hidden; } takes effect; ensure you reference the
existing methods hide() and load(movieId) and keep the body class add/remove
paired so the background scroll is unlocked on hide().
In `@src/dom/render/SearchUI.ts`:
- Around line 61-65: The code in SearchUI is always setting search state type to
"data" even when searchResult.results is an empty array, so the "empty" UI never
appears; update both places where setSearchState is called (the blocks around
the shown diff and the similar block at lines 105-107) to inspect
searchResult.results.length and set type to "empty" when zero (and include
movies: []), otherwise set type to "data" and compute isLastPage as before;
ensure you handle undefined/null results defensively and keep the same payload
fields so downstream rendering logic (empty-container vs data view) can switch
correctly.
- Line 30: seeMore() (and the code around the referenced block) can use a stale
this.keyword set at instantiation, so ensure the latest search term is read at
the time of the "see more" request: modify seeMore() to fetch the current
keyword via getKeywordFromURL() (or accept keyword as an explicit parameter)
instead of relying on this.keyword, and update any other methods that reference
this.keyword (the methods around the 72-74 region) to either refresh
this.keyword on new searches or read getKeywordFromURL() on demand so all
outgoing requests use the current term.
- Line 47: The error message <p> can remain hidden because hide() always adds
"hidden" via this.errorMessageContent?.classList.add("hidden") but the error
render path never removes it; update the error rendering logic in SearchUI (the
methods that render errors around the existing hide()/show() behavior and the
error rendering at lines ~109-112) to explicitly remove the "hidden" class
(e.g., call classList.remove("hidden") or ensure a showError method clears the
hidden flag) or centralize visibility toggling so the error element is unhidden
when an error is rendered; search for this.errorMessageContent, hide(), and the
error render block to apply the fix.
In `@src/main.ts`:
- Around line 79-100: The infinite-scroll IntersectionObserver is permanently
disabled when the last page triggers observer.disconnect(), so further
navigations or new searches won't re-enable loading; change the disconnect
behavior and re-enable observation on list resets: replace observer.disconnect()
with observer.unobserve(sentinel) (so the observer instance stays usable) and
ensure you call observer.observe(sentinel) when the list/search state is reset
(hook into the code paths that change results — e.g., when getKeywordFromURL
changes, or in the places where mainUI or searchUI reset/initialize their lists
or after calling mainUI.seeMore/searchUI.seeMore). Keep the same observer
callback and isLoading guard but add re-observe calls in the list-reset logic so
new queries re-enable infinite scroll.
- Around line 36-45: The app must treat the URL as the source of truth: move the
current render logic that inspects getKeywordFromURL() and decides between
mainUI.load()/hide() and searchUI.load()/hide() into a reusable render()
function, call that from handleSearch (after setURLParams) and also register a
window 'popstate' listener that invokes render() so UI updates when the user
navigates back/forward; additionally, manage the IntersectionObserver lifecycle
inside the search UI (e.g., create/connect the observer whenever searchUI.load()
runs or when a new keyword is set, and only disconnect() when hasMore becomes
false but ensure a fresh observer is created on subsequent searches or route
changes) so infinite scroll is reinitialized on new searches or when returning
to search results.
In `@src/repository/RatingRepository.ts`:
- Around line 2-7: 현재 getRate와 setRate가 localStorage의 임의 문자열을 그대로 숫자로 변환해 유효하지
않은 별점("abc","0","9" 등)을 복원할 수 있습니다; getRate와 setRate에서 허용할 별점 범위(예: minRate,
maxRate)를 정의하고 setRate는 입력 rate가 범위 내에 있을 때만 저장하도록, getRate는 localStorage에서 읽은
값을 Number로 변환한 뒤 범위를 벗어나면 null로 정규화하도록 수정하세요; 관련 식별자는 getRate, setRate, movieId,
rate이며 저장키(`movie-${movieId}`)로 읽기/쓰기 모두 일관된 검증을 적용해야 합니다.
In `@src/utils/fetcher.ts`:
- Around line 1-3: parseJSON currently returns null for empty response bodies
which can later be thrown as `null`; update parseJSON (the async function taking
Response) to detect empty/non-2xx responses and normalize by throwing or
returning a consistent Error instance (e.g., new Error with HTTP status and
message) instead of null so callers never receive `null` as an error; ensure the
normalization includes response.status and a short message and adjust any
callers that expect parsed JSON to handle that Error consistently.
In `@src/utils/pageParams.ts`:
- Around line 4-5: The current parsing of the page param (const prevPage =
Number(params.get("page") || 1)) can yield NaN for inputs like "page=abc";
update the logic that reads params.get("page") to parse and validate the numeric
result using Number (or parseInt), check Number.isFinite() (or
Number.isSafeInteger()), coerce any value < 1 to 1, and fall back to the default
1 when validation fails so prevPage is always a valid integer >= 1.
In `@styles/thumbnail.css`:
- Around line 10-14: The current media query (`@media` (max-width: 1024px))
switches .thumbnail-list to 3 columns too late, causing horizontal overflow
between ~1025–1279px; update the responsive rule for .thumbnail-list by either
raising the breakpoint to 1280px (e.g. change the media query threshold) or make
the columns fluid (replace grid-template-columns: repeat(3, 1fr) with a
responsive pattern such as repeat(auto-fit, minmax(..., 1fr)) so thumbnails
reflow smoothly across 1280px and below).
🪄 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: b2477414-736c-409e-a679-f334740184dd
📒 Files selected for processing (28)
README.mdcypress/e2e/Modal.cy.tscypress/e2e/spec.tscypress/fixtures/movieDetail.jsonindex.htmlsrc/apis/movie/api.tssrc/dom/eventHandler/handleMovieSearch.tssrc/dom/eventHandler/handleSeeMore.tssrc/dom/render/MainUI.tssrc/dom/render/ModalUI.tssrc/dom/render/SearchUI.tssrc/dom/render/renderError.tssrc/dom/render/renderInitialUI.tssrc/dom/render/renderMainUI.tssrc/dom/render/renderResultSectionContent.tssrc/dom/render/renderSearchUI.tssrc/dom/render/renderThumbnailList.tssrc/main.tssrc/repository/RatingRepository.tssrc/utils/fetcher.tssrc/utils/getErrorMessage.tssrc/utils/getKeywordFromURL.tssrc/utils/pageParams.tssrc/utils/setURLParams.tsstyles/main.cssstyles/modal.cssstyles/thumbnail.csstemplates/modal.html
💤 Files with no reviewable changes (7)
- src/dom/render/renderInitialUI.ts
- templates/modal.html
- src/dom/render/renderMainUI.ts
- src/dom/eventHandler/handleMovieSearch.ts
- src/dom/eventHandler/handleSeeMore.ts
- src/dom/render/renderResultSectionContent.ts
- src/dom/render/renderSearchUI.ts
| <button class="close-modal" id="closeModal"> | ||
| <img src="./images/modal_button_close.png" /> | ||
| </button> |
There was a problem hiding this comment.
닫기 버튼에 접근 가능한 이름을 넣어 주세요.
아이콘만 들어 있는 버튼인데 텍스트나 aria-label이 없어 스크린리더에서는 그냥 “button”으로 읽힙니다. 모달을 닫는 핵심 컨트롤이라 이름이 꼭 필요합니다.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@index.html` around lines 221 - 223, The close button (<button
class="close-modal" id="closeModal">) lacks an accessible name; update the
element to provide one by adding an appropriate aria-label (e.g.,
aria-label="Close modal") or visible text inside the button, ensuring the label
describes its action (closing the modal) while keeping the existing <img
src="./images/modal_button_close.png"> for visuals.
| <div class="modal-image modal-skeleton-image" id="modal-image"> | ||
| <img src="" alt="" /> |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
# First, let's check the index.html file around lines 225-226
head -n 230 index.html | tail -n 20Repository: woowacourse/javascript-movie-review
Length of output: 821
🏁 Script executed:
# Check if there's a linting configuration in the repository
find . -maxdepth 2 -type f \( -name ".eslintrc*" -o -name "eslintignore" -o -name ".htmlhintrc" -o -name "w3crc" -o -name "prettier*" \)Repository: woowacourse/javascript-movie-review
Length of output: 109
🏁 Script executed:
# Search for JavaScript that sets the modal image src
rg "modal-image|modal.*src|querySelector.*modal" -A 3 -B 1Repository: woowacourse/javascript-movie-review
Length of output: 2356
🏁 Script executed:
# Check .htmlhintrc configuration
cat .htmlhintrcRepository: woowacourse/javascript-movie-review
Length of output: 232
🏁 Script executed:
# Check .eslintrc.json configuration
cat .eslintrc.jsonRepository: woowacourse/javascript-movie-review
Length of output: 268
🏁 Script executed:
# Check if there are any other relevant files related to linting
rg "src=\"\"" index.htmlRepository: woowacourse/javascript-movie-review
Length of output: 112
🌐 Web query:
empty src attribute browser side effects re-request document
💡 Result:
An empty src attribute on an <img> tag (src="") triggers a network request to the current document's URL in most browsers, potentially causing performance issues, traffic spikes, or state corruption via cookies/sessions. This is due to URI resolution treating "" as a relative path resolving to the base URL per RFC 3986. Key browser behaviors (historical and recent sources up to 2026): - Chrome/Safari/WebKit: Requests the page itself. - Firefox: Fixed in 3.5+ (no request). - IE: Requests the page directory. - Opera: Ignores. HTML spec requires non-empty valid URL for src; empty src shows broken image icon but still fetches in many engines. Recent WHATWG issues (2026) discuss extending no-fetch to whitespace-only src to match Chromium/WebKit interop. No full document reload occurs; it's an additional subresource request visible in Network tab (test: open DevTools Network, load <img src="">). Use data URI, about:blank, or omit src/JS-set later to avoid. Workarounds: - src="data:image/gif;base64,R0lGODlhAQABAIAAAAAAAP///yH5BAEAAAAALAAAAAABAAEAAAIBRAA7" - Omit src, set via JS: img.src = "image.jpg" - Transparent 1x1 placeholder. To verify: Chrome DevTools Network tab shows the request when loading <img src="">.
Citations:
- 1: https://humanwhocodes.com/blog/2009/11/30/empty-image-src-can-destroy-your-site/
- 2: https://humanwhocodes.com/blog/2009/11/30/empty-image-src-can-destroy-your-site
- 3: https://stackoverflow.com/questions/28891999/img-src-attribute-returns-the-url-of-the-page-when-blank
- 4: https://bugzilla.mozilla.org/show_bug.cgi?id=504309
- 5: https://bugzilla.mozilla.org/show_bug.cgi?id=469138
- 6: https://stackoverflow.com/questions/17221433/would-an-empty-src-attribute-cause-a-request-on-any-browser
- 7: <img> setting src = "" should maybe be sync whatwg/html#2429
- 8: Treatment of fallback image on <img src=''> whatwg/html#3280
- 9: Exclude whitespace-only src attributes from fetching resources whatwg/html#12160
src="" 속성을 제거하고 JavaScript에서 설정하도록 변경하세요.
.htmlhintrc의 "src-not-empty": true 규칙에 위배되며, 빈 src=""는 Chrome, Safari 등 주요 브라우저에서 현재 문서를 재요청하는 부수 효과를 유발합니다. ModalUI.ts의 동적 설정 로직은 올바르게 구현되어 있으므로, HTML 마크업에서 src="" 속성을 제거하고 이미지가 준비된 시점에만 JavaScript에서 src를 설정하면 됩니다.
🧰 Tools
🪛 HTMLHint (1.9.2)
[error] 226-226: The attribute [ src ] of the tag [ img ] must have a value.
(src-not-empty)
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@index.html` around lines 225 - 226, HTML에 빈 src="" 속성이 있어 브라우저의 불필요한 문서 재요청을
유발하므로 <img> 엘리먼트의 src 속성(아이디 "modal-image" 또는 클래스 "modal-image
modal-skeleton-image")을 마크업에서 제거하고, ModalUI.ts의 기존 동적 로직을 사용해 이미지가 준비된 시점에만
JavaScript에서 src를 설정하도록 변경하세요; 마크업에서는 alt는 유지하고 src는 삭제한 뒤 ModalUI.ts의 이미지 로딩/설정
코드에서 document.getElementById("modal-image") 또는 해당 셀렉터로 엘리먼트를 찾아 src를 할당하도록 수정하면
됩니다.
| <p | ||
| class="detail" | ||
| id="modal-detail" | ||
| class="modal-skeleton modal-skeleton-detail" | ||
| ></p> |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
find . -name "index.html" -type fRepository: woowacourse/javascript-movie-review
Length of output: 91
🏁 Script executed:
sed -n '280,290p' ./index.htmlRepository: woowacourse/javascript-movie-review
Length of output: 388
class 속성이 중복되어 첫 번째 클래스가 적용되지 않습니다.
동일 요소에 class 속성이 두 번 선언되면 마지막 선언이 이전 선언을 덮어씁니다. 현재 "detail" 클래스가 "modal-skeleton modal-skeleton-detail"로 인해 무시되므로, 줄거리 영역의 스켈레톤 로딩 상태가 의도대로 렌더링되지 않을 수 있습니다. 두 클래스를 하나의 class 속성으로 통합하세요.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@index.html` around lines 284 - 288, 요소 id="modal-detail"에 class 속성이 중복되어 첫번째
클래스 값("detail")가 덮어씌워지는 문제입니다; id="modal-detail"를 가진 <p> 엘리먼트를 찾아 class 속성들을 하나로
합쳐서 예: "detail modal-skeleton modal-skeleton-detail"처럼 단일 class 속성으로 통합하고 중복된
class 속성 선언을 제거하세요.
| - 상세정보 요청 API | ||
| - [x] 별점 매기기 | ||
| - 2점 단위 별 5개로 구성 | ||
| - API 연동 고려한 localsStorage 활용 |
There was a problem hiding this comment.
용어 오탈자 수정이 필요합니다
localsStorage → localStorage로 수정하는 편이 문서 신뢰성과 검색성을 높입니다.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@README.md` at line 145, Fix the typo in the README by changing the term
"localsStorage" to "localStorage" wherever it appears (notably the line that
currently reads "API 연동 고려한 localsStorage 활용") so the document consistently and
correctly uses the web-standard identifier localStorage for clarity and
searchability.
| belongs_to_collection: { | ||
| id: number; | ||
| name: string; | ||
| poster_path: string; | ||
| backdrop_path: string; | ||
| }; |
There was a problem hiding this comment.
belongs_to_collection의 null 케이스가 타입에서 빠져 있습니다.
같은 PR의 cypress/fixtures/movieDetail.json Line 14에서는 이 값이 null인데 여기서는 항상 객체로 선언돼 있습니다. 이 필드를 소비하는 쪽이 어떤 분기를 해야 안전한지 먼저 정리해 보시고, 그 분기가 타입에도 드러나게 맞춰 두는 편이 좋겠습니다.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/apis/movie/api.ts` around lines 24 - 29, The belongs_to_collection
property in the movie API type is declared as an object but can be null (per
cypress/fixtures/movieDetail.json), so update the type definition for
belongs_to_collection in src/apis/movie/api.ts to explicitly allow null (e.g.,
union with null) and then update any consumers of belongs_to_collection to
handle the null branch (check for null before accessing
id/name/poster_path/backdrop_path) so the runtime branching is reflected in the
type system.
| if (sentinel) { | ||
| let isLoading = false; | ||
|
|
||
| const observer = new IntersectionObserver( | ||
| async (entries, observer) => { | ||
| if (!entries[0].isIntersecting || isLoading) return; | ||
|
|
||
| isLoading = true; | ||
| try { | ||
| const keyword = getKeywordFromURL(); | ||
| const hasMore = keyword | ||
| ? await searchUI.seeMore() | ||
| : await mainUI.seeMore(); | ||
| if (!hasMore) observer.disconnect(); | ||
| } finally { | ||
| isLoading = false; | ||
| } | ||
| }, | ||
| { rootMargin: "200px" }, | ||
| ); | ||
|
|
||
| observer.observe(sentinel); |
There was a problem hiding this comment.
한 번 마지막 페이지에 도달하면 이후 무한 스크롤이 영구적으로 꺼집니다.
observer는 여기서 한 번만 생성/등록되는데, 마지막 페이지에서 disconnect()를 호출한 뒤 다시 observe()하는 경로가 없습니다. 그래서 메인 목록이나 특정 검색어에서 끝까지 내려간 다음 다른 검색을 하거나 홈 목록으로 돌아오면 #sentinel이 다시 보여도 seeMore()가 더 이상 자동 호출되지 않습니다.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/main.ts` around lines 79 - 100, The infinite-scroll IntersectionObserver
is permanently disabled when the last page triggers observer.disconnect(), so
further navigations or new searches won't re-enable loading; change the
disconnect behavior and re-enable observation on list resets: replace
observer.disconnect() with observer.unobserve(sentinel) (so the observer
instance stays usable) and ensure you call observer.observe(sentinel) when the
list/search state is reset (hook into the code paths that change results — e.g.,
when getKeywordFromURL changes, or in the places where mainUI or searchUI
reset/initialize their lists or after calling mainUI.seeMore/searchUI.seeMore).
Keep the same observer callback and isLoading guard but add re-observe calls in
the list-reset logic so new queries re-enable infinite scroll.
| getRate(movieId: number): number | null { | ||
| const value = localStorage.getItem(`movie-${movieId}`); | ||
| return value ? Number(value) : null; | ||
| } | ||
| setRate(movieId: number, rate: number) { | ||
| localStorage.setItem(`movie-${movieId}`, rate.toString()); |
There was a problem hiding this comment.
저장소가 비정상 값을 그대로 복원할 수 있습니다.
localStorage는 외부 입력이라 "abc", "0", "9" 같은 값이 남아 있을 수 있고, 지금은 Number(value) 결과가 그대로 흘러갑니다. 별점의 유효 범위를 먼저 정해 보시고, 읽기/쓰기 모두 그 범위를 벗어나면 null로 정규화하는 쪽이 안전하지 않을까요?
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/repository/RatingRepository.ts` around lines 2 - 7, 현재 getRate와 setRate가
localStorage의 임의 문자열을 그대로 숫자로 변환해 유효하지 않은 별점("abc","0","9" 등)을 복원할 수 있습니다;
getRate와 setRate에서 허용할 별점 범위(예: minRate, maxRate)를 정의하고 setRate는 입력 rate가 범위 내에
있을 때만 저장하도록, getRate는 localStorage에서 읽은 값을 Number로 변환한 뒤 범위를 벗어나면 null로 정규화하도록
수정하세요; 관련 식별자는 getRate, setRate, movieId, rate이며 저장키(`movie-${movieId}`)로 읽기/쓰기
모두 일관된 검증을 적용해야 합니다.
| const parseJSON = async (response: Response) => { | ||
| const text = await response.text(); | ||
| if (!text) return null; |
There was a problem hiding this comment.
빈 실패 응답이 throw null로 흘러갈 수 있습니다.
본문이 없는 non-2xx 응답이면 parseJSON()이 null을 반환하고, 아래에서는 그 값을 그대로 throw 합니다. 상위 레이어가 기대하는 에러 형태를 한 번 따라가 보시고, 이 지점에서 공통 Error로 정규화해야 하는지 점검해 보시면 좋겠습니다.
Also applies to: 24-30
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/utils/fetcher.ts` around lines 1 - 3, parseJSON currently returns null
for empty response bodies which can later be thrown as `null`; update parseJSON
(the async function taking Response) to detect empty/non-2xx responses and
normalize by throwing or returning a consistent Error instance (e.g., new Error
with HTTP status and message) instead of null so callers never receive `null` as
an error; ensure the normalization includes response.status and a short message
and adjust any callers that expect parsed JSON to handle that Error
consistently.
| const prevPage = Number(params.get("page") || 1); | ||
| return prevPage; |
There was a problem hiding this comment.
🧩 Analysis chain
🌐 Web query:
In JavaScript, what do Number("abc"), Number(""), and Number(null) return, and what is the recommended way to validate a numeric query parameter before using it?
💡 Result:
Number("abc") returns NaN, Number("") returns 0, and Number(null) returns 0. The recommended way to validate a numeric query parameter (which arrives as a string) before using it is to use Number.isFinite after conversion with Number, or Number.isSafeInteger for integers. This distinguishes valid finite numbers from NaN (invalid) and Infinity. Avoid relying solely on typeof or isNaN after Number conversion, as NaN and Infinity are both 'number' type but invalid for parameters. Example: const param = req.query.id; // string, e.g. "123" or "abc" const num = Number(param); if (Number.isFinite(num)) { // Safe to use num } else { // Invalid: return 400 Bad Request } For integers specifically: if (Number.isSafeInteger(num)) { ... }
Citations:
- 1: https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Global_Objects/Number/Number
- 2: https://developer.mozilla.org/pl/docs/Web/JavaScript/Reference/Global_Objects/Number
- 3: https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Global_Objects/Number
- 4: https://developer.mozilla.org/en-US/docs/Web/JavaScript/Guide/Numbers_and_strings
- 5: https://stackoverflow.com/questions/38447477/how-to-check-if-req-params-is-numbers
page 파라미터가 비정상 값일 때 NaN이 전파됩니다
page=abc 같은 케이스에서 현재 파싱 결과가 NaN이 되어 이후 증가 로직까지 깨질 수 있습니다.
JavaScript의 Number("abc")는 NaN을 반환하므로, 검증이 필수입니다. 다음 순서로 점검해 보세요:
- 숫자 변환 결과 유효성 검사 (
Number.isFinite()또는Number.isSafeInteger()활용) - 최소값 1 보정
- 실패 시 기본값 1로 복구
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/utils/pageParams.ts` around lines 4 - 5, The current parsing of the page
param (const prevPage = Number(params.get("page") || 1)) can yield NaN for
inputs like "page=abc"; update the logic that reads params.get("page") to parse
and validate the numeric result using Number (or parseInt), check
Number.isFinite() (or Number.isSafeInteger()), coerce any value < 1 to 1, and
fall back to the default 1 when validation fails so prevPage is always a valid
integer >= 1.
| @media (max-width: 1024px) { | ||
| .thumbnail-list { | ||
| grid-template-columns: repeat(3, 1fr); | ||
| gap: 24px; | ||
| } |
There was a problem hiding this comment.
첫 번째 브레이크포인트가 너무 늦습니다.
기본 5열 레이아웃 폭이 1280px인데 3열 전환이 1024px부터라서, 1025~1279px 구간에서는 여전히 가로 스크롤이 납니다. 이 구간을 확인해 보시고, 브레이크포인트를 올리거나 컬럼 정의를 더 유동적으로 잡는 편이 좋겠습니다.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@styles/thumbnail.css` around lines 10 - 14, The current media query (`@media`
(max-width: 1024px)) switches .thumbnail-list to 3 columns too late, causing
horizontal overflow between ~1025–1279px; update the responsive rule for
.thumbnail-list by either raising the breakpoint to 1280px (e.g. change the
media query threshold) or make the columns fluid (replace grid-template-columns:
repeat(3, 1fr) with a responsive pattern such as repeat(auto-fit, minmax(...,
1fr)) so thumbnails reflow smoothly across 1280px and below).
jw-r
left a comment
jw-r
left a comment
There was a problem hiding this comment.
라바 안녕하세요!
2단계 미션 진행하시느라 고생하셨습니다 👍
class 기반 상태 관리 리팩토링이 인상적입니다! 기존의 let 변수 + 유틸 함수 방식에서 Discriminated Union 패턴을 활용한 class 기반으로 전환한 것이 훌륭합니다. 셀프 리뷰에서 설명한 것처럼, 각 상태가 한 번에 하나만 존재할 수 있도록 타입으로 보장한 점이 핵심적으로 좋습니다.
Repository 패턴을 통한 별점 저장소 추상화, 모달의 로딩/데이터/에러 상태 관리, 무한스크롤 구현까지 전체적으로 완성도가 높습니다.
1단계에서도 충분히 좋았는데, 2단계에서 구조적으로 한 단계 더 성장한 코드입니다. 다만 아래 몇 가지 사항은 수정이 필요합니다. 확인 후 반영 부탁드립니다!
수정 요청 사항
1. seeMore 에러 핸들링
파일: MainUI.ts, SearchUI.ts
load()에는 try-catch가 있지만 seeMore()에는 없습니다. API 실패 시 사용자에게 아무런 피드백 없이 조용히 실패하게 됩니다. load()와 동일하게 에러 핸들링을 추가해주세요.
// 현재 코드
async seeMore() {
const popularMovies = await getPopularMovies({ page: getPageParam() + 1 });
// 에러 발생 시 피드백 없음
}2. SearchUI의 keyword 불일치 (버그)
파일: SearchUI.ts
keyword가 생성자에서 한 번만 초기화되는데, load()에서는 getKeywordFromURL()을 직접 호출하고 seeMore()에서는 this.keyword를 사용합니다. 검색어 변경 후 무한스크롤 시 이전 검색어로 요청이 갈 수 있는 잠재적 버그입니다. 수정해주세요.
// 현재 - 불일치 가능성
keyword = getKeywordFromURL(); // 생성 시점의 값
async load() {
const keyword = getKeywordFromURL(); // 최신 값 ✅
}
async seeMore() {
query: this.keyword || "", // 생성 시점의 값 ⚠️
}3. MainUI의 불필요한 프로퍼티
파일: MainUI.ts
thumbnailListElement가 search-thumbnail-list를 가리키고 있습니다. MainUI에서 사용하지 않는 프로퍼티이므로 제거해주세요. 불필요한 참조는 코드를 읽는 사람에게 혼란을 줍니다.
4. 접근 제어자 활용
파일: MainUI.ts, SearchUI.ts, ModalUI.ts
mainState, searchState 등 상태 프로퍼티와 DOM 프로퍼티들이 외부에서 직접 접근 가능합니다. #render()는 이미 private 메서드로 되어 있는데, 상태 프로퍼티는 public인 것이 일관성이 없습니다. setMainState를 통해서만 상태를 변경하도록 private으로 선언해주세요.
📝 Q&A
Q. Repository 패턴은 변경이 확실한 경우에만 쓰는게 좋은가?
좋은 질문입니다. 결론부터 말하면, "변경이 확실할 때만"이 아니라 "경계를 명확히 하고 싶을 때" 사용합니다.
실무에서의 Repository 패턴
실무에서 Repository 패턴은 꽤 자주 사용됩니다. 다만 목적이 조금 다릅니다:
| 목적 | 설명 |
|---|---|
| 교체 대비 | 지금처럼 localStorage → API 전환이 예정된 경우 |
| 테스트 용이성 | Mock Repository를 주입하여 UI 로직만 테스트 가능 |
| 경계 분리 | "데이터를 어디서 가져오는지"와 "데이터를 어떻게 쓰는지" 분리 |
언제 쓰고, 언제 안 쓰는가?
✅ 쓰면 좋은 경우:
- 외부 의존성(API, DB, localStorage)에 접근하는 경우
- 테스트에서 해당 의존성을 격리하고 싶은 경우
- 팀 내에서 "이 인터페이스만 맞추면 된다"는 계약이 필요한 경우
❌ 과할 수 있는 경우:
- 단순한 값 하나를 읽고 쓰는 경우 (config 값 등)
- 절대 변경될 일이 없는 내부 로직
- 프로젝트 규모가 작아 추상화 비용이 이점보다 큰 경우
라바 코드에 적용하면?
현재 RatingRepository는 적절한 사용입니다. 이유:
- 변경이 예정되어 있음 (localStorage → API)
- ModalUI가 저장소 구현을 몰라도 됨
- 테스트 시 Mock을 주입할 수 있음
한 가지 더 추천하자면, 명시적 interface를 분리하면 더 좋습니다:
// 인터페이스 정의
interface IRatingRepository {
getRate(movieId: number): number | null;
setRate(movieId: number, rate: number): void;
}
// localStorage 구현체
class LocalStorageRatingRepository implements IRatingRepository {
getRate(movieId: number): number | null { ... }
setRate(movieId: number, rate: number) { ... }
}
// 나중에 API 구현체
class ApiRatingRepository implements IRatingRepository {
getRate(movieId: number): number | null { ... }
setRate(movieId: number, rate: number) { ... }
}
// main.ts에서 교체 한 줄
const ratingRepository = new LocalStorageRatingRepository();
// → const ratingRepository = new ApiRatingRepository();이렇게 하면 "이 계약만 지키면 된다"는 점이 코드 레벨에서 명확해집니다.
"불필요한 wrapper 아닌가?"에 대해
말씀하신 우려가 맞는 경우도 있습니다. YAGNI(You Aren't Gonna Need It) 원칙에 따르면, 필요하지 않은 추상화는 하지 않는 것이 맞습니다.
하지만 Repository 패턴의 비용은 interface 하나 + class 하나로 매우 낮습니다. 그에 비해 얻는 이점(테스트 용이성, 경계 명확화)이 크기 때문에, 외부 의존성에 접근하는 레이어에서는 기본적으로 적용하는 것이 좋은 습관입니다.
🔍 추가로 고려해볼 것들
아래는 필수는 아니지만, 고려해보면 좋겠습니다.
- 무한스크롤 + 페이지 전환: observer disconnect 후 재연결 로직
- 모달 열릴 때 body 스크롤 잠금:
document.body.style.overflow = 'hidden' - MainUI/SearchUI 공통 요소 추출: skeletonList, errorContainer 등 공유 DOM 관리
전체적인 설계와 패턴 적용은 훌륭합니다. 위의 수정 요청 사항(특히 1, 2번)만 반영해주시면 바로 Approve 하겠습니다! 💪
자세한 내용은 코멘트를 확인해주세요!
| interceptMovieDetailWithDelay, | ||
| } from "./spec"; | ||
|
|
||
| describe("모달", () => { |
There was a problem hiding this comment.
스켈레톤 표시, 데이터 렌더링, 닫기(X/배경/ESC), 별점 유지까지. 모달 E2E 테스트가 매우 꼼꼼하네요👍
| mainUI.load(); | ||
| return; | ||
| } | ||
| setURLParams({ keyword: searchInput!.value, page: "1" }); |
There was a problem hiding this comment.
searchInput!로 non-null assertion을 사용하고 있네요
위의 optional chaining(에서 이미 searchInput이 null이면 value.trim()이 undefined가 되어 ""(empty string)과 비교하면 false가 되므로 여기까지 도달하긴 합니다.
다만 의도를 명확히 하려면 if (!searchInput) return; 을 먼저 처리하는 것도 방법입니다!
| const handleSearch = () => { | ||
| if (searchInput?.value.trim() === "") { | ||
| window.history.pushState({}, "", import.meta.env.BASE_URL); | ||
| searchUI.hide(); | ||
| mainUI.load(); | ||
| return; | ||
| } | ||
| setURLParams({ keyword: searchInput!.value, page: "1" }); | ||
| mainUI.hide(); | ||
| searchUI.load(); |
| | { type: "empty" }; | ||
|
|
||
| class MainUI { | ||
| mainState: MainState = { type: "loading" }; |
There was a problem hiding this comment.
mainState를 private으로 선언하여 setMainState 메서드를 통해서만 상태를 변경하도록 강제하면 더 안전할 것 같네요!
| } | ||
| } | ||
|
|
||
| async seeMore() { |
There was a problem hiding this comment.
seeMore에 try-catch가 없습니다
API 호출 실패 시 에러가 main.ts의 IntersectionObserver까지 전파되는데,
observer의 finally에서 isLoading만 false로 바꾸고 사용자에게 피드백이 없습니다
| modalBackground = document.getElementById("modalBackground"); | ||
| modalCloseButton = document.getElementById("closeModal"); | ||
| modalImageContainer = document.getElementById("modal-image"); | ||
| modalImage = document.querySelector<HTMLImageElement>("#modal-image img"); | ||
| modalTitle = document.getElementById("modal-title"); | ||
| modalCategory = document.getElementById("modal-category"); | ||
| modalRate = document.getElementById("modal-rate"); | ||
| modalDetail = document.getElementById("modal-detail"); | ||
| modalRateReview = document.getElementById("modal-rate-review"); | ||
| modalRatePoints = document.getElementById("modal-rate-points"); |
There was a problem hiding this comment.
DOM 프로퍼티들이 많네요
관련 요소들을 객체로 그룹핑하면 가독성이 좋아집니다!
예: this.els = { background, closeButton, image, ... }
| this.modalBackground?.classList.add("hidden"); | ||
| } | ||
|
|
||
| async load(movieId: number) { |
There was a problem hiding this comment.
모달 열기 시 로딩 → 데이터/에러 흐름이 깔끔하네요
모달이 열릴 때 body 스크롤을 막는 처리(overflow: hidden)도 고려해봐도 좋을 것 같습니다
| | { type: "error"; message: string } | ||
| | { type: "empty" }; | ||
|
|
||
| class SearchUI { |
There was a problem hiding this comment.
MainUI와 SearchUI가 skeletonList, errorContainer, emptyContainer 등
동일한 DOM 요소를 각자 참조하고 있습니다
두 UI가 동시에 존재하면 hide/show가 충돌할 수 있는데,
현재는 mainUI.hide() / searchUI.hide()로 관리되고 있어서 문제는 없지만,
공용 요소를 별도로 추출하면 더 명확해질 수 있습니다!
| "search-thumbnail-list", | ||
| ) as HTMLUListElement; | ||
|
|
||
| keyword = getKeywordFromURL(); |
There was a problem hiding this comment.
keyword가 생성자에서 한 번만 초기화되어 검색어 변경 시 업데이트되지 않습니다
load()에서는 getKeywordFromURL()를 직접 호출하지만, seeMore()에서는 this.keyword를 사용하고 있어 불일치가 될 수 있습니다.
| } | ||
| } | ||
|
|
||
| async seeMore() { |
2 participants
🎯 미션 소개
🕵️ 셀프 리뷰(Self-Review)
제출 전 체크 리스트
리뷰 요청 & 논의하고 싶은 내용
1) 이번 단계에서 가장 많이 고민했던 문제와 해결 과정에서 배운 점
renderResultSection이라는 유틸함수에서 모든 DOM 상태를 관리하고 let으로 선언한 내부 변수(isLoading, isError 등) 기반으로 조건부로 DOM에 hidden class를 추가하고 제거하면서 보여줄 항목을 관리했습니다. 이 방식으로 렌더링할 요소를 결정하다 보니 접근할 돔 요소가 너무 많아지고, 상태가 바뀌는 곳에서 매번renderResultSection을 호출해줘야했기 때문에 호출을 빠뜨리는 경우 UI가 동기화되지 않기도했습니다. 이런 문제를 해결하기 위해서 크게main,search,modal관련 UI를 class 기반으로 수정하고 해당 클래스 내부에서 보여줄 상태를 가지며 메서드를 통해 이 상태를 관리하는 방식으로 리팩토링하였습니다.this.로 접근 가능한 일반 상태로 관리하는 방식보다 UI 상태 일관성을 조금 더 보장할 수 있겠다고 생각했습니다.2) 이번 리뷰를 통해 논의하고 싶은 부분
상황 : 이 기능은 빠른 배포를 위해 임시로 local storage 로 구현하기로 하였다. 다음번 배포에 서버 API 로 교체할 예정이다. 프론트엔드 개발자는 local storage, 서버 API 를 쉽고, 안전하게 갈아끼울 수 있는 구조로 개발하여야 한다.라는 상황에서 요구사항을 만족시키기 위해서 인터페이스만 두고 실제 구현체는 다른 곳에서 만든 것을 호출하는 방식으로 추후 변경이 발생했을 때 호출부에서 한 곳만 변경할 수 있도록 Repository 패턴을 활용했습니다. 제가 이해한 바로 Repository 패턴은 "localStorage인지, API인지 어딘지 모르는 곳에서 가져오는 함수를 전달받아서 그거를 실행해서 나에게 반환해주는 함수"를 호출하는 방식으로 사용하고 그 내부를 localStorage로 할지, API로 할지는 호출하는 곳에서는 알 수 없도록 하는 방식으로 이해했습니다. 하지만 이 방식은 지금처럼 localStorage -> API로 변경될 것을 명확하게 알 수 있는 상황에서나 유용한게 아닌지 의문이 들었습니다.지금과 같이 변경이 확실할 땐 후에 수정이 발생했을 때 한 곳만 수정하도록 설계해두어 유용할 수 있지만,, 그렇지 않은 경우에는 추후 수정이 생길 것을 미리 예측하여 구현체를 Repository로 감싸고 그걸로 만든 인스턴스를 의존성 주입하여 사용하는 패턴을 사용하는 것이 불필요한 wrapper를 만들게 된다고 생각했습니다. 이러한 부분에서 실무에서도 Repository 패턴이 사용되는지, 사용된다면 지금과 같이 변경이 확실한 경우에만 사용되는지, 아니면 추후에 수정이 발생할 경우를 대비하여 한 곳만 수정할 수 있도록 미리 대응해두기 위한 용도로 사용하는지 궁금합니다
✅ 리뷰어 체크 포인트