[2단계 - 상세 정보 & UI/UX 개선하기] 먼지 미션 제출합니다.#316
[2단계 - 상세 정보 & UI/UX 개선하기] 먼지 미션 제출합니다.#316stardusty-lab wants to merge 104 commits intowoowacourse:stardusty-labfrom
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
Summary by CodeRabbit릴리스 노트
Walkthrough이 변경은 영화 앱에 다중 단계 기능을 추가합니다. 모달 기반 상세 정보 화면(포스터/제목 클릭 시 표시, ESC로 닫기), localStorage 기반 5점 별점 평가 시스템(새로고침 시 유지), 무한 스크롤(화면 하단 도달 시 20개 항목 추가 로드)이 포함됩니다. HTTP 요청 로직이 중앙화되고 리팩토링되며, 쿼리 캐싱 및 페이지네이션 상태 관리가 추가됩니다. CSS는 모달 스타일과 반응형 그리드 레이아웃(640px–1440px)으로 확장되고, 포괄적인 E2E 테스트와 DOM 템플릿 구조가 구현됩니다. Estimated code review effort🎯 4 (Complex) | ⏱️ ~65 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: 11
🧹 Nitpick comments (12)
src/states/MovieListState.ts (1)
1-17: 상태 관리 클래스 구조에 대해 생각해 볼 점현재 구현은 기본적인 상태 저장/조회 기능을 제공합니다. 몇 가지 고려해 볼 사항이 있습니다:
getTotalPages()에 반환 타입number | null을 명시하면 타입 안전성이 향상됩니다.검색어가 변경되거나 초기화가 필요할 때
total_pages를 리셋하는 메서드가 필요하지 않을까요? 현재setTotalPages만으로 충분한지 검토해 보세요.이 클래스가 앱 전역에서 하나의 인스턴스만 필요하다면, 싱글톤 패턴이나 모듈 레벨 상태로 관리하는 방식도 고려해 볼 수 있습니다.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/states/MovieListState.ts` around lines 1 - 17, The MovieListState class should explicitly type getTotalPages to return number | null and provide a method to reset total_pages; update the signature of getTotalPages() to return number | null, add a resetTotalPages() method that sets this.#total_pages = null, and consider exposing or converting MovieListState to a singleton if only one global instance is needed (e.g., by exporting a single instance at the module level or documenting that change) so callers can rely on type safety and a way to clear state when search terms change.public/styles/thumbnail.css (1)
3-80: 반응형 그리드 레이아웃 구현이 잘 되었습니다.
minmax(200px, 1fr)를 사용한 유연한 그리드와 미디어 쿼리를 통한 단계적 컬럼 조정이 적절합니다.한 가지 작은 개선 사항으로, 640px 이하에서
repeat(1, minmax(200px, 1fr))는 단순히minmax(200px, 1fr)또는1fr로 단순화할 수 있습니다.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@public/styles/thumbnail.css` around lines 3 - 80, Replace the redundant grid-template-columns value in the small-screen media query: in the .thumbnail-list rule inside the `@media` (max-width: 640px) block, change grid-template-columns: repeat(1, minmax(200px, 1fr)); to a simpler form such as grid-template-columns: minmax(200px, 1fr); or grid-template-columns: 1fr; to achieve the same single-column layout more clearly (refer to the .thumbnail-list selector and the `@media` (max-width: 640px) rule).cypress/e2e/movie-list-rendering.cy.ts (1)
35-38: 테스트 안정성을 위해 네트워크 요청 대기 추가 고려스크롤 후 바로 assertion을 수행하고 있는데, 스크롤 이벤트가 네트워크 요청을 트리거하고 응답이 처리될 때까지 시간이 걸릴 수 있습니다. 테스트가 간헐적으로 실패할 수 있습니다.
it("스크롤을 끝까지 내리면 영화 목록이 추가로 생성되어 렌더링 된다.", () => { cy.scrollTo('bottom', { duration: 500 }); + cy.wait("@getPopularPage2"); cy.get("#movie-list li").should("have.length.greaterThan", entries); });🤖 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 35 - 38, The test "스크롤을 끝까지 내리면 영화 목록이 추가로 생성되어 렌더링 된다." currently asserts immediately after cy.scrollTo which can race with the network request; fix by intercepting the movie-list network request, aliasing it, triggering the scroll (cy.scrollTo('bottom', ...)), then cy.wait for that alias before asserting on "#movie-list li" and entries; reference the test name, the selector "#movie-list li", and the variable entries, and use cy.intercept('/api/movies*') (or the actual API path) with an alias like "@loadMoreMovies" followed by cy.wait('@loadMoreMovies') then the existing should("have.length.greaterThan", entries).index.html (1)
86-89: 이미지 src 속성 빈 값 처리
src=""속성이 비어있어 HTMLHint에서 경고가 발생합니다. 템플릿이므로 JavaScript에서 동적으로 설정되지만, placeholder 이미지 경로나data-src속성 사용을 고려해볼 수 있습니다. 다만 현재 동작에는 문제가 없으므로 선택적으로 개선하면 됩니다.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@index.html` around lines 86 - 89, The empty src attribute on the image element (id "detail-modal-img" inside the "modal-image" / "modal-container") triggers HTMLHint; to fix, replace the empty src with either a small placeholder image path (e.g. a transparent pixel or "placeholder.png") or remove src and use a data-src attribute that your JavaScript sets at runtime (keep the id "detail-modal-img" so existing JS can query it), ensuring the template no longer contains src="" to silence the linter.src/main.ts (1)
196-218: 스크롤 이벤트 핸들러 최적화 고려
getIsBottom함수가 매 스크롤 이벤트마다 새로 생성됩니다. 스크롤은 매우 빈번하게 발생하므로, debounce/throttle 적용이나 함수를 외부로 추출하는 것을 고려해볼 수 있습니다.또한 Line 205의
pageYOffset은 deprecated되어window.scrollY를 권장합니다 (이미 Line 202에서 사용 중).현재
queryMoviePopular의isFetching가드가 중복 요청을 막아주므로 당장 문제는 없지만, 검색 모드에서는 해당 가드가 적용되지 않습니다.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/main.ts` around lines 196 - 218, Extract the getIsBottom logic out of the scroll event listener into a standalone function (e.g., function getIsBottom()) and replace pageYOffset with window.scrollY; then wrap the scroll handler with a throttle or debounce (e.g., throttleScroll) and call handleMoreMovie() only when getIsBottom() returns true. Also add a fetch-guard for search mode similar to queryMoviePopular.isFetching (or reuse an isFetching flag) so repeated scroll-triggered requests during search are blocked. Ensure you update references in the listener to use the extracted getIsBottom and the debounce/throttle wrapper.src/renders/detailModal.ts (3)
58-63: 유효하지 않은 release_date 처리
movieInfo.release_date가 빈 문자열이거나 유효하지 않은 날짜 형식인 경우new Date()가 Invalid Date를 반환하고,getFullYear()가NaN을 반환합니다. API 응답에서 release_date가 항상 유효한지 확인하거나, 방어적인 처리를 추가하는 것이 좋겠습니다.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/renders/detailModal.ts` around lines 58 - 63, The code that builds the category string using new Date(movieInfo.release_date).getFullYear() can produce NaN for invalid/empty release_date; update the block around detailModalCategory and movieInfo.release_date to defensively parse the date (e.g. create const d = new Date(movieInfo.release_date) and check !isNaN(d.getTime()) or use Date.parse) and derive year only when valid, otherwise use a sensible fallback like "Unknown" or omit the year; then build category (const category = validYear ? `${year} · ${genres}` : genres) and assign it to detailModalCategory.textContent so Invalid Date/NaN never appears.
6-9: getMovieRate 반환 타입 불일치
RateRepository인터페이스에서getMovieRate가number를 반환하도록 정의되어 있지만, 실제 구현에서는 평점이 없을 때undefined를 반환합니다.renderRateStart에서 기본값rate: number = 0으로 처리하고 있어 런타임에는 문제없지만, 타입 정의를number | undefined로 수정하면 타입 안전성이 향상됩니다.인터페이스 정의와 구현 사이의 타입 일관성에 대해 고민해보면 좋겠습니다.
Also applies to: 16-20
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/renders/detailModal.ts` around lines 6 - 9, The RateRepository interface's getMovieRate signature is incorrect: change its return type from number to number | undefined to match implementations that may return undefined (update interface definition for RateRepository and any related typing), then review usages such as renderRateStart (which currently defaults rate: number = 0) to accept number | undefined or coerce with a fallback (e.g., const rate = repo.getMovieRate(id) ?? 0) so callers remain type-safe; ensure implementations of getMovieRate and setMovieRate still conform to the updated RateRepository type.
12-15: JSON.parse 에러 처리 고려
localStorage에 잘못된 JSON이 저장된 경우JSON.parse가 예외를 던질 수 있습니다. 현재 코드에서는 이를 처리하지 않아 모달 렌더링이 실패할 수 있습니다.try-catch를 추가하거나, 어떤 상황에서 잘못된 JSON이 저장될 수 있는지 고민해보세요. (예: 다른 스크립트가 같은 키를 사용하거나, 사용자가 개발자 도구에서 수동 편집하는 경우)
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/renders/detailModal.ts` around lines 12 - 15, The `#getRates` method can throw if localStorage contains invalid JSON; wrap the JSON.parse call in a try-catch and return a safe empty object on error (optionally clear or reset the invalid 'rates' key). Specifically, in the `#getRates` function, read the string from localStorage.getItem('rates'), attempt JSON.parse inside a try block, catch any SyntaxError (or generic error) and return {} (and optionally call localStorage.removeItem('rates') or set it to "{}" to avoid repeated failures). Ensure the method still returns an object in all cases so callers that use `#getRates`() won't break.src/services/query.ts (1)
6-17: 요청 중복 방지 시 반환값의 의미 고려현재
isFetching이true일 때undefined를 반환하는데, 호출자 입장에서는 요청이 스킵된 것인지 에러가 발생한 것인지 구분하기 어렵습니다. 이 동작이 의도된 것인지, 아니면 호출자에게 "이미 요청 중"임을 알려줄 필요가 있는지 생각해보면 좋겠습니다.현재
main.ts에서if (movies)체크로 처리하고 있어 동작에는 문제가 없지만, 디버깅 시 요청이 스킵된 이유를 파악하기 어려울 수 있습니다.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/services/query.ts` around lines 6 - 17, The refetch function currently returns undefined when isFetching is true which makes it ambiguous to callers; change refetch to return a clear sentinel (e.g., null or a small result object like { skipped: true }) or reject with a specific Error so callers (and main.ts's if (movies) check) can distinguish "request skipped" from other failures; update the early-return branch in refetch to return that sentinel and ensure any code expecting a movie result (e.g., callers using getMoviePopular results) handles the sentinel appropriately.src/services/api.ts (1)
48-53: 함수명 개선 고려
getMovieMovieId라는 이름이 "Movie"가 반복되어 다소 어색합니다.getMovieById또는getMovieDetail같은 이름이 더 직관적일 수 있습니다. 현재 동작에는 문제없으니 선택적으로 검토해보세요.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/services/api.ts` around lines 48 - 53, The function name getMovieMovieId is awkward due to the repeated "Movie"; rename it to a clearer name such as getMovieById (or getMovieDetail) by updating the exported function identifier from getMovieMovieId to the chosen name, preserving the signature and implementation (the parameter { id }: { id: number } and the requestAjax call). Also update all internal references and imports/exports that call getMovieMovieId to use the new name so TypeScript/ES module resolution remains correct.public/styles/modal.css (1)
140-143: 빈 CSS 블록 제거
.star-message규칙이 비어 있습니다. 스타일이 필요 없다면 제거하고, 향후 스타일 추가 예정이라면 주석으로 의도를 명시하는 것이 좋겠습니다. Stylelint에서도block-no-empty오류로 감지됩니다.🔧 수정 제안
-.modal-description .star-message-box .star-message { - -} +/* .star-message 스타일은 star-message-box에서 상속받아 별도 정의 불필요 */🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@public/styles/modal.css` around lines 140 - 143, Remove the empty CSS rule for the selector .modal-description .star-message-box .star-message (or, if you intend to add styles later, replace the empty block with a short comment indicating intent), so the empty block is not present and the stylelint block-no-empty error is resolved; locate the .modal-description .star-message-box .star-message rule in modal.css and either delete it or add a comment like /* placeholder for future styles */ inside the block.cypress/e2e/movie-detail-modal.cy.ts (1)
28-114: 반복되는 모달 오픈 시퀀스는 작은 헬퍼로 분리해도 좋아 보입니다.
첫 번째 영화 클릭 →@getMovieId대기가 여러 테스트에 반복돼서, 나중에 준비 단계가 바뀌면 시나리오 본문까지 같이 수정해야 합니다. 이 흐름만 공통화해 두면 각 테스트가 “무엇을 검증하는지”에 더 잘 집중할 수 있겠습니다.As per coding guidelines, "Promote modular thinking—breaking problems into reusable components."
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@cypress/e2e/movie-detail-modal.cy.ts` around lines 28 - 114, Extract the repeated "open first movie and wait" sequence into a small helper (e.g., openFirstMovieAndWait or openDetailModal) that performs cy.get("#movie-list li").first().find(".thumbnail").click() and cy.wait("@getMovieId"), declare it at the top of the spec so all tests can call it, and replace each repeated block in the tests with a single call to that helper; keep the same selectors ("#movie-list li", ".thumbnail") and network alias ("@getMovieId") when implementing and using the helper so behavior is unchanged.
🤖 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-list-rendering.cy.ts`:
- Line 40: Update the test description string inside the it(...) block (the test
with title "마지막 페이지까지 렌더링 됬을때 스크롤을 끝까지 내려도 영화 리스트를 더 출력하지 않는다.") to correct the
Korean typo: change "렌더링 됬을때" to "렌더링 됐을 때" so the title reads "마지막 페이지까지 렌더링 됐을
때 스크롤을 끝까지 내려도 영화 리스트를 더 출력하지 않는다.".
In `@public/styles/modal.css`:
- Around line 103-105: The CSS rule for the selector .modal-description hr is
missing a trailing semicolon after the border-color declaration; update the
.modal-description hr rule to terminate the border-color property with a
semicolon so the declaration reads correctly and follows best practices.
In `@README.md`:
- Around line 30-53: Update the README checklist to mark implemented features as
completed by changing their boxes from [ ] to [x]: under "## 상세 팝업" mark the
popup and ESC behavior done, under "## 별점" mark star-rating, persistent storage
behavior (localStorage->API-ready), rating display in the popup, and persistence
on refresh as done, under "## 무한 스크롤" mark both movie list and search list
infinite-scroll behaviors done, and under "## 반응형" mark the three responsive
items (영화 목록, 영화 검색 목록, 영화 상세 팝업) as done so the README accurately reflects the
implemented code.
In `@src/main.ts`:
- Around line 142-147: The handleDetail function currently invokes
getMovieMovieId without error handling so network/404 failures silently prevent
renderDetailModal; wrap the async call in a try-catch (or use the existing
errorTryCatch helper) inside handleDetail, catch errors from getMovieMovieId,
log or surface the error and show user feedback (e.g., toast or alert) instead
of attempting to render; ensure you still call renderDetailModal only when
movieInfo is valid.
- Around line 107-110: The call to
movieListState.setTotalPages(movies.total_pages) can throw when movies is
undefined (runSearch may return undefined via errorTryCatch); guard the access
by checking movies before calling setTotalPages (similar to loadInit) — i.e.,
only call movieListState.setTotalPages(...) when movies is truthy, and handle
the fallback (e.g., set to 0 or skip) when movies is falsy; reference the
runSearch return value "movies", the errorTryCatch path, and the
"movieListState.setTotalPages" invocation to locate and fix the code.
In `@src/renders/detailModal.ts`:
- Line 11: Class name has a typo: rename RateLocalStroageRepository to
RateLocalStorageRepository across the file and any places that import or
reference it; update the class declaration (class RateLocalStorageRepository
implements RateRepository) and adjust all usages, exports, and imports to the
corrected identifier to keep types and references consistent.
In `@src/renders/movieList.ts`:
- Around line 32-34: The title (the strong element) is missing the click handler
so only the poster opens the detail; add a click listener to the title element
that calls handleDetail(movie.id) just like the thumbnail does (or refactor to a
shared function to avoid duplication). Locate the code that creates the
title/strong element (the variable representing the movie title) and bind
titleElement.addEventListener('click', () => handleDetail(movie.id)); ensure the
same movie.id value and event behavior as the thumbnail handler.
In `@src/services/dto.ts`:
- Around line 73-76: The ProductionCountry interface has a typo: the property
iso_3165_1 should be iso_3166_1 to match the ISO 3166-1 standard and TMDB API;
update the interface property name in ProductionCountry and then update all
references/usages that access ProductionCountry.iso_3165_1 to use
ProductionCountry.iso_3166_1 (or adjust deserialization/mapping where external
JSON keys differ) so types align with runtime data.
- Around line 78-82: The SpokenLanguage interface has a typo: rename the field
iso_638_1 to iso_639_1 to match the ISO 639-1 standard and TMDB API; update the
interface definition (SpokenLanguage) and then find and update all usages,
deserializers/parsers, type assertions, and tests that reference iso_638_1 so
they now use iso_639_1 or map the incoming API field correctly if you're
transforming response objects.
In `@src/services/http.ts`:
- Around line 38-69: Wrap the fetch call in a try/catch so network errors are
caught and rethrown as a RequestFetchError with a normalized response shape
(e.g., status 0, data: error.message, headers: {}, config). After a successful
fetch, inspect res.headers.get('content-type') (or clone the Response via
res.clone() if you may try multiple parsers) to decide whether to call
res.json() or res.text(); avoid calling both on the same Response body. Build
the response object with data, status: res.status, headers:
Object.fromEntries(res.headers), config and return it on res.ok, otherwise throw
new RequestFetchError(response) so callers always receive the same
{status,data,headers,config} shape; reference the existing fetch call/finalUrl,
res.json()/res.text(), and the RequestFetchError constructor when implementing.
- Around line 32-48: The request always sets 'Content-Type': 'application/json'
in customHeaders which breaks multipart FormData; modify the header construction
around customHeaders (used when calling fetch(finalUrl, ...)) to detect if data
is an instance of FormData and if so omit or delete the Content-Type entry so
the browser can set the correct multipart boundary; e.g., only include
'Content-Type' when data is not FormData (or remove it from customHeaders before
spreading into the fetch headers) while preserving Authorization and any other
headers.
---
Nitpick comments:
In `@cypress/e2e/movie-detail-modal.cy.ts`:
- Around line 28-114: Extract the repeated "open first movie and wait" sequence
into a small helper (e.g., openFirstMovieAndWait or openDetailModal) that
performs cy.get("#movie-list li").first().find(".thumbnail").click() and
cy.wait("@getMovieId"), declare it at the top of the spec so all tests can call
it, and replace each repeated block in the tests with a single call to that
helper; keep the same selectors ("#movie-list li", ".thumbnail") and network
alias ("@getMovieId") when implementing and using the helper so behavior is
unchanged.
In `@cypress/e2e/movie-list-rendering.cy.ts`:
- Around line 35-38: The test "스크롤을 끝까지 내리면 영화 목록이 추가로 생성되어 렌더링 된다." currently
asserts immediately after cy.scrollTo which can race with the network request;
fix by intercepting the movie-list network request, aliasing it, triggering the
scroll (cy.scrollTo('bottom', ...)), then cy.wait for that alias before
asserting on "#movie-list li" and entries; reference the test name, the selector
"#movie-list li", and the variable entries, and use cy.intercept('/api/movies*')
(or the actual API path) with an alias like "@loadMoreMovies" followed by
cy.wait('@loadMoreMovies') then the existing should("have.length.greaterThan",
entries).
In `@index.html`:
- Around line 86-89: The empty src attribute on the image element (id
"detail-modal-img" inside the "modal-image" / "modal-container") triggers
HTMLHint; to fix, replace the empty src with either a small placeholder image
path (e.g. a transparent pixel or "placeholder.png") or remove src and use a
data-src attribute that your JavaScript sets at runtime (keep the id
"detail-modal-img" so existing JS can query it), ensuring the template no longer
contains src="" to silence the linter.
In `@public/styles/modal.css`:
- Around line 140-143: Remove the empty CSS rule for the selector
.modal-description .star-message-box .star-message (or, if you intend to add
styles later, replace the empty block with a short comment indicating intent),
so the empty block is not present and the stylelint block-no-empty error is
resolved; locate the .modal-description .star-message-box .star-message rule in
modal.css and either delete it or add a comment like /* placeholder for future
styles */ inside the block.
In `@public/styles/thumbnail.css`:
- Around line 3-80: Replace the redundant grid-template-columns value in the
small-screen media query: in the .thumbnail-list rule inside the `@media`
(max-width: 640px) block, change grid-template-columns: repeat(1, minmax(200px,
1fr)); to a simpler form such as grid-template-columns: minmax(200px, 1fr); or
grid-template-columns: 1fr; to achieve the same single-column layout more
clearly (refer to the .thumbnail-list selector and the `@media` (max-width: 640px)
rule).
In `@src/main.ts`:
- Around line 196-218: Extract the getIsBottom logic out of the scroll event
listener into a standalone function (e.g., function getIsBottom()) and replace
pageYOffset with window.scrollY; then wrap the scroll handler with a throttle or
debounce (e.g., throttleScroll) and call handleMoreMovie() only when
getIsBottom() returns true. Also add a fetch-guard for search mode similar to
queryMoviePopular.isFetching (or reuse an isFetching flag) so repeated
scroll-triggered requests during search are blocked. Ensure you update
references in the listener to use the extracted getIsBottom and the
debounce/throttle wrapper.
In `@src/renders/detailModal.ts`:
- Around line 58-63: The code that builds the category string using new
Date(movieInfo.release_date).getFullYear() can produce NaN for invalid/empty
release_date; update the block around detailModalCategory and
movieInfo.release_date to defensively parse the date (e.g. create const d = new
Date(movieInfo.release_date) and check !isNaN(d.getTime()) or use Date.parse)
and derive year only when valid, otherwise use a sensible fallback like
"Unknown" or omit the year; then build category (const category = validYear ?
`${year} · ${genres}` : genres) and assign it to detailModalCategory.textContent
so Invalid Date/NaN never appears.
- Around line 6-9: The RateRepository interface's getMovieRate signature is
incorrect: change its return type from number to number | undefined to match
implementations that may return undefined (update interface definition for
RateRepository and any related typing), then review usages such as
renderRateStart (which currently defaults rate: number = 0) to accept number |
undefined or coerce with a fallback (e.g., const rate = repo.getMovieRate(id) ??
0) so callers remain type-safe; ensure implementations of getMovieRate and
setMovieRate still conform to the updated RateRepository type.
- Around line 12-15: The `#getRates` method can throw if localStorage contains
invalid JSON; wrap the JSON.parse call in a try-catch and return a safe empty
object on error (optionally clear or reset the invalid 'rates' key).
Specifically, in the `#getRates` function, read the string from
localStorage.getItem('rates'), attempt JSON.parse inside a try block, catch any
SyntaxError (or generic error) and return {} (and optionally call
localStorage.removeItem('rates') or set it to "{}" to avoid repeated failures).
Ensure the method still returns an object in all cases so callers that use
`#getRates`() won't break.
In `@src/services/api.ts`:
- Around line 48-53: The function name getMovieMovieId is awkward due to the
repeated "Movie"; rename it to a clearer name such as getMovieById (or
getMovieDetail) by updating the exported function identifier from
getMovieMovieId to the chosen name, preserving the signature and implementation
(the parameter { id }: { id: number } and the requestAjax call). Also update all
internal references and imports/exports that call getMovieMovieId to use the new
name so TypeScript/ES module resolution remains correct.
In `@src/services/query.ts`:
- Around line 6-17: The refetch function currently returns undefined when
isFetching is true which makes it ambiguous to callers; change refetch to return
a clear sentinel (e.g., null or a small result object like { skipped: true }) or
reject with a specific Error so callers (and main.ts's if (movies) check) can
distinguish "request skipped" from other failures; update the early-return
branch in refetch to return that sentinel and ensure any code expecting a movie
result (e.g., callers using getMoviePopular results) handles the sentinel
appropriately.
In `@src/states/MovieListState.ts`:
- Around line 1-17: The MovieListState class should explicitly type
getTotalPages to return number | null and provide a method to reset total_pages;
update the signature of getTotalPages() to return number | null, add a
resetTotalPages() method that sets this.#total_pages = null, and consider
exposing or converting MovieListState to a singleton if only one global instance
is needed (e.g., by exporting a single instance at the module level or
documenting that change) so callers can rely on type safety and a way to clear
state when search terms change.
🪄 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: 14e07d01-661a-4ec8-98d0-063322cab0f2
📒 Files selected for processing (18)
README.mdcypress/e2e/movie-detail-modal.cy.tscypress/e2e/movie-list-rendering.cy.tscypress/e2e/movie-search.cy.tsindex.htmlpublic/styles/main.csspublic/styles/modal.csspublic/styles/thumbnail.csssrc/main.tssrc/renders/detailModal.tssrc/renders/movieList.tssrc/services/api.tssrc/services/dto.tssrc/services/http.tssrc/services/query.tssrc/states/MovieListState.tstemplates/modal.htmltest/fixtures.ts
|
|
||
| it("마지막 페이지까지 렌더링 됬을때 더보기 버튼을 출력하지 않는다.", () => { | ||
| cy.get("#more-button").click(); | ||
| it("마지막 페이지까지 렌더링 됬을때 스크롤을 끝까지 내려도 영화 리스트를 더 출력하지 않는다.", () => { |
There was a problem hiding this comment.
오타 수정
"렌더링 됬을때" → "렌더링 됐을 때"로 수정하면 좋겠습니다.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@cypress/e2e/movie-list-rendering.cy.ts` at line 40, Update the test
description string inside the it(...) block (the test with title "마지막 페이지까지 렌더링
됬을때 스크롤을 끝까지 내려도 영화 리스트를 더 출력하지 않는다.") to correct the Korean typo: change "렌더링
됬을때" to "렌더링 됐을 때" so the title reads "마지막 페이지까지 렌더링 됐을 때 스크롤을 끝까지 내려도 영화 리스트를 더
출력하지 않는다.".
| .modal-description hr { | ||
| border-color: #67788E | ||
| } |
There was a problem hiding this comment.
세미콜론 누락
border-color 속성 끝에 세미콜론이 누락되어 있습니다. CSS에서 마지막 속성이라도 세미콜론을 붙이는 것이 좋습니다.
🔧 수정 제안
.modal-description hr {
- border-color: `#67788E`
+ border-color: `#67788E`;
}🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@public/styles/modal.css` around lines 103 - 105, The CSS rule for the
selector .modal-description hr is missing a trailing semicolon after the
border-color declaration; update the .modal-description hr rule to terminate the
border-color property with a semicolon so the declaration reads correctly and
follows best practices.
| ## 상세 팝업 | ||
| - [ ] 포스터나 제목을 클릭시 자세한 예고편이나 줄거리 등이 정보를 보여주는 팝업이 뜬다 | ||
| - [ ] ESC 를 누르면 팝업이 닫힌다 | ||
|
|
||
| ## 별점 | ||
| - [ ] 상세 팝업에서 내 별점 ui에서 사용자가 2점 단위로 10까지 평점을 별을 클릭해서 매길수 있다 | ||
| - 2점: 최악이예요 | ||
| - 4점: 별로예요 | ||
| - 6점: 보통이에요 | ||
| - 8점: 재미있어요 | ||
| - 10점: 명작이에요 | ||
| - [ ] 평점을 저장할 곳은 1차: localStorage -> 2 차 db(api 호출)로 갈아끼울수 있는 것을 고려해서 구현한다 | ||
| - [ ] 상세 팝업에서 자신이 매긴 평점을 확인할 수 있다 | ||
| - [ ] 새로고침시 평점이 유지가 되어야 한다 | ||
|
|
||
|
|
||
| ## 무한 스크롤 | ||
| - [ ] 영화 목록에서 더보기 버튼 클릭 대신 사용자가 브라우저 화면의 끝에 도달하면 20개의 목록을 서버에 요청하여 추가로 불러올 수 있다 | ||
| - [ ] 영화 검색 목록에서 더보기 버튼 클릭 대신 사용자가 브라우저 화면의 끝에 도달하면 20개의 목록을 서버에 요청하여 추가로 불러올 수 있다 | ||
|
|
||
| ## 반응형 | ||
| - [ ] 영화 목록 | ||
| - [ ] 영화 검색 목록 | ||
| - [ ] 영화 상세 팝업 |
There was a problem hiding this comment.
체크리스트 항목 업데이트 필요
PR 설명과 구현된 코드를 보면 상세 팝업, 별점, 무한 스크롤, 반응형 기능이 구현된 것으로 보입니다. 완료된 항목의 체크박스를 [x]로 업데이트하는 것이 좋겠습니다.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@README.md` around lines 30 - 53, Update the README checklist to mark
implemented features as completed by changing their boxes from [ ] to [x]: under
"## 상세 팝업" mark the popup and ESC behavior done, under "## 별점" mark star-rating,
persistent storage behavior (localStorage->API-ready), rating display in the
popup, and persistence on refresh as done, under "## 무한 스크롤" mark both movie
list and search list infinite-scroll behaviors done, and under "## 반응형" mark the
three responsive items (영화 목록, 영화 검색 목록, 영화 상세 팝업) as done so the README
accurately reflects the implemented code.
| ); | ||
|
|
||
| movieListState.setTotalPages(movies.total_pages); | ||
|
|
There was a problem hiding this comment.
movies가 undefined일 때 setTotalPages 호출
runSearch에서 에러 발생 시 errorTryCatch가 undefined를 반환할 수 있는데, Line 109에서 movies.total_pages에 바로 접근합니다. 이 경우 런타임 에러가 발생합니다.
loadInit (Line 78-82)처럼 if (movies) 체크를 추가하는 것이 좋겠습니다.
🔧 수정 제안
}
);
- movieListState.setTotalPages(movies.total_pages);
+ if (!movies) return;
+
+ movieListState.setTotalPages(movies.total_pages);
removeTopRatedMovie();🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/main.ts` around lines 107 - 110, The call to
movieListState.setTotalPages(movies.total_pages) can throw when movies is
undefined (runSearch may return undefined via errorTryCatch); guard the access
by checking movies before calling setTotalPages (similar to loadInit) — i.e.,
only call movieListState.setTotalPages(...) when movies is truthy, and handle
the fallback (e.g., set to 0 or skip) when movies is falsy; reference the
runSearch return value "movies", the errorTryCatch path, and the
"movieListState.setTotalPages" invocation to locate and fix the code.
| export const handleDetail = (id: number) => { | ||
| (async () => { | ||
| const movieInfo = await getMovieMovieId({id}); | ||
| renderDetailModal(movieInfo); | ||
| })(); | ||
| } |
There was a problem hiding this comment.
handleDetail 에러 처리 누락
getMovieMovieId 호출 시 네트워크 오류나 404 응답에 대한 에러 처리가 없습니다. API 호출 실패 시 사용자에게 피드백을 주지 못하고 모달이 열리지 않게 됩니다. 다른 API 호출처럼 errorTryCatch를 사용하거나, try-catch로 에러를 처리하는 방안을 고려해보세요.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/main.ts` around lines 142 - 147, The handleDetail function currently
invokes getMovieMovieId without error handling so network/404 failures silently
prevent renderDetailModal; wrap the async call in a try-catch (or use the
existing errorTryCatch helper) inside handleDetail, catch errors from
getMovieMovieId, log or surface the error and show user feedback (e.g., toast or
alert) instead of attempting to render; ensure you still call renderDetailModal
only when movieInfo is valid.
| thumbnail.addEventListener('click', () => { | ||
| handleDetail(movie.id); | ||
| }); |
There was a problem hiding this comment.
제목 클릭 이벤트 누락 확인
README에 따르면 "포스터나 제목을 클릭시" 상세 팝업이 열려야 합니다. 현재 썸네일(포스터)에만 클릭 이벤트가 바인딩되어 있고, 제목(strong 요소)에는 클릭 이벤트가 없습니다.
요구사항과 일치하도록 제목에도 클릭 핸들러를 추가해야 할지 검토해 보세요.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/renders/movieList.ts` around lines 32 - 34, The title (the strong
element) is missing the click handler so only the poster opens the detail; add a
click listener to the title element that calls handleDetail(movie.id) just like
the thumbnail does (or refactor to a shared function to avoid duplication).
Locate the code that creates the title/strong element (the variable representing
the movie title) and bind titleElement.addEventListener('click', () =>
handleDetail(movie.id)); ensure the same movie.id value and event behavior as
the thumbnail handler.
| export interface ProductionCountry { | ||
| iso_3165_1: string | ||
| name: string | ||
| } |
There was a problem hiding this comment.
ISO 코드 필드명 오타
iso_3165_1은 iso_3166_1의 오타로 보입니다 (ISO 3166-1 국가 코드 표준). TMDB API 응답의 실제 필드명과 일치하는지 확인하고 수정하는 것이 좋겠습니다.
🔧 수정 제안
export interface ProductionCountry {
- iso_3165_1: string
+ iso_3166_1: string
name: string
}📝 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.
| export interface ProductionCountry { | |
| iso_3165_1: string | |
| name: string | |
| } | |
| export interface ProductionCountry { | |
| iso_3166_1: string | |
| name: string | |
| } |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/services/dto.ts` around lines 73 - 76, The ProductionCountry interface
has a typo: the property iso_3165_1 should be iso_3166_1 to match the ISO 3166-1
standard and TMDB API; update the interface property name in ProductionCountry
and then update all references/usages that access ProductionCountry.iso_3165_1
to use ProductionCountry.iso_3166_1 (or adjust deserialization/mapping where
external JSON keys differ) so types align with runtime data.
| export interface SpokenLanguage { | ||
| english_name: string | ||
| iso_638_1: string | ||
| name: string | ||
| } |
There was a problem hiding this comment.
ISO 코드 필드명 오타
iso_638_1은 iso_639_1의 오타로 보입니다 (ISO 639-1 언어 코드 표준). TMDB API 응답의 실제 필드명과 일치하는지 확인하고 수정하는 것이 좋겠습니다.
🔧 수정 제안
export interface SpokenLanguage {
english_name: string
- iso_638_1: string
+ iso_639_1: string
name: string
}📝 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.
| export interface SpokenLanguage { | |
| english_name: string | |
| iso_638_1: string | |
| name: string | |
| } | |
| export interface SpokenLanguage { | |
| english_name: string | |
| iso_639_1: string | |
| name: string | |
| } |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/services/dto.ts` around lines 78 - 82, The SpokenLanguage interface has a
typo: rename the field iso_638_1 to iso_639_1 to match the ISO 639-1 standard
and TMDB API; update the interface definition (SpokenLanguage) and then find and
update all usages, deserializers/parsers, type assertions, and tests that
reference iso_638_1 so they now use iso_639_1 or map the incoming API field
correctly if you're transforming response objects.
| const customHeaders = { | ||
| 'Content-Type': 'application/json', | ||
| Authorization: `Bearer ${apiKey}`, | ||
| ...headers, | ||
| }; | ||
|
|
||
| const res = await fetch(finalUrl, { | ||
| method, | ||
| ...(!!Object.values(customHeaders).filter(Boolean).length && { | ||
| headers: { | ||
| ...customHeaders as Record<string, string>, | ||
| } | ||
| }), | ||
| // credentials: 'include', | ||
| ...(data && { | ||
| body: data instanceof FormData ? data : JSON.stringify(data), | ||
| }), |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
cat -n src/services/http.ts | head -70Repository: woowacourse/javascript-movie-review
Length of output: 2277
🌐 Web query:
FormData Content-Type header Fetch API multipart boundary
💡 Result:
When using FormData with the Fetch API for multipart/form-data requests, do not manually set the Content-Type header. The browser automatically sets it to "multipart/form-data; boundary=----WebKitFormBoundary..." (or similar unique boundary) when FormData is passed as the request body. Manually setting Content-Type: "multipart/form-data" without the boundary parameter overrides the browser's automatic header, causing the request to lack the required boundary, which breaks multipart parsing on the server. Correct usage: const formData = new FormData; formData.append('file', myFile); fetch('/upload', { method: 'POST', body: formData // No headers: { 'Content-Type': ... } }); This behavior is documented in MDN Web Docs and consistent across browsers. The Fetch spec handles FormData serialization by generating the boundary automatically.
Citations:
- 1: https://developer.mozilla.org/en-US/docs/Web/API/FormData/Using_FormData_Objects
- 2: https://stackoverflow.com/questions/39280438/fetch-missing-boundary-in-multipart-form-data-post
- 3: https://stackoverflow.com/questions/35192841/how-do-i-post-with-multipart-form-data-using-fetch
- 4: https://developer.mozilla.org/en-US/docs/Web/API/XMLHttpRequest_API/Using_FormData_Objects
- 5: https://developer.mozilla.org/en-US/docs/Web/API/Fetch_API/Using_Fetch
- 6: http://developer.mozilla.org/en-US/docs/Learn_web_development/Extensions/Forms/Sending_forms_through_JavaScript
FormData 요청이 깨질 수 있습니다.
현재 코드에서 data instanceof FormData일 때는 본문을 올바르게 전달하지만, customHeaders에서 항상 'Content-Type': 'application/json'을 설정합니다. 그런데 FormData를 사용할 때 브라우저는 자동으로 'multipart/form-data; boundary=...' 헤더를 설정하는데, 수동으로 Content-Type을 지정하면 이 자동 설정을 덮어씀으로써 boundary 정보가 손실됩니다. 결과적으로 서버에서 multipart를 제대로 파싱하지 못합니다.
본문의 타입을 확인하여 FormData일 때는 Content-Type 헤더를 어떻게 처리해야 할지 다시 검토해 주세요.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/services/http.ts` around lines 32 - 48, The request always sets
'Content-Type': 'application/json' in customHeaders which breaks multipart
FormData; modify the header construction around customHeaders (used when calling
fetch(finalUrl, ...)) to detect if data is an instance of FormData and if so
omit or delete the Content-Type entry so the browser can set the correct
multipart boundary; e.g., only include 'Content-Type' when data is not FormData
(or remove it from customHeaders before spreading into the fetch headers) while
preserving Authorization and any other headers.
| const res = await fetch(finalUrl, { | ||
| method, | ||
| ...(!!Object.values(customHeaders).filter(Boolean).length && { | ||
| headers: { | ||
| ...customHeaders as Record<string, string>, | ||
| } | ||
| }), | ||
| // credentials: 'include', | ||
| ...(data && { | ||
| body: data instanceof FormData ? data : JSON.stringify(data), | ||
| }), | ||
| }); | ||
|
|
||
| let responseData; | ||
| try { | ||
| responseData = await res.json(); | ||
| } catch (e) { | ||
| console.error(e); | ||
| responseData = await res.text(); | ||
| } | ||
|
|
||
| const response = { | ||
| data: responseData, | ||
| status: res.status, | ||
| headers: customHeaders, | ||
| config, | ||
| }; | ||
|
|
||
| if (res.ok) { | ||
| return response; | ||
| } else { | ||
| throw new RequestFetchError(response); |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
find . -name "http.ts" -path "*/services/*" | head -5Repository: woowacourse/javascript-movie-review
Length of output: 101
🏁 Script executed:
cat -n src/services/http.tsRepository: woowacourse/javascript-movie-review
Length of output: 2969
🏁 Script executed:
rg "class RequestFetchError|export.*RequestFetchError" -A 5Repository: woowacourse/javascript-movie-review
Length of output: 421
🏁 Script executed:
rg "fetch\(" -B 5 -A 20 src/services/http.tsRepository: woowacourse/javascript-movie-review
Length of output: 658
Fetch 에러 처리에서 body 소비와 네트워크 에러 처리 문제를 해결하세요.
응답 body는 한 번만 소비할 수 있으므로, res.json() 실패 후 res.text() 호출은 항상 실패합니다. 또한 네트워크 오류로 fetch() 자체가 reject되면 RequestFetchError로 감싸지지 않아, 호출부가 일관된 {status, data, headers, config} 구조를 기대할 수 없습니다.
응답 타입을 먼저 판단하고 (Content-Type 헤더 확인 등), fetch 실패와 파싱 실패를 모두 같은 에러 shape로 정규화할 방법을 생각해 보세요.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/services/http.ts` around lines 38 - 69, Wrap the fetch call in a
try/catch so network errors are caught and rethrown as a RequestFetchError with
a normalized response shape (e.g., status 0, data: error.message, headers: {},
config). After a successful fetch, inspect res.headers.get('content-type') (or
clone the Response via res.clone() if you may try multiple parsers) to decide
whether to call res.json() or res.text(); avoid calling both on the same
Response body. Build the response object with data, status: res.status, headers:
Object.fromEntries(res.headers), config and return it on res.ok, otherwise throw
new RequestFetchError(response) so callers always receive the same
{status,data,headers,config} shape; reference the existing fetch call/finalUrl,
res.json()/res.text(), and the RequestFetchError constructor when implementing.
ukkodeveloper
left a comment
ukkodeveloper
left a comment
There was a problem hiding this comment.
먼지 안녕하세요 :)
상세 모달, 별점, 무한 스크롤까지 작업량이 많으셨을텐데 수고 많으셨습니다.
상세 팝업 오픈을 위해서 이벤트 바인딩을 renderMovieList에서 하고 있습니다. render에서는 순수하게 render만 처리하고 싶었는데 어떻게 분리할 수 있을지 잘 모르겠습니다.
지금처럼 DOM이 만들어진 뒤에 이벤트를 연결해야 하는 상황 자체는 자연스러워 보여요. 다만 renderMovieList가 "노드 생성"과 "상세 모달을 여는 행위 연결"을 같이 맡기 시작하면, 렌더 함수라기보다 화면 조합과 같은 느낌이... 되곤하죠.. ㅠ
지금 단계에서는 createMovieNode는 노드만 만들고, 바깥에서 이벤트를 위임하는 쪽으로 한 번 더 나눠보는 것도 좋은 방법 같아요. 반대로 "이 컴포넌트가 클릭 가능한 카드 자체를 책임진다"는 기준을 세우셨다면 현재 구조도 가능은 한데, 모두 같은 인터랙션을 갖도록 책임 범위를 더 분명히 맞춰주는 쪽이 좋아 보였습니다.
pageState, MovieListState 이렇게 분산해서 저장하는게 맞는건지 의문이 들었습니다.
사용자 입력 상태와 서버 응답 상태를 구분하는 건 좋은 시도였다고 생각해요!
다만 지금은 둘이 분리돼 있기보다, 스크롤 시점마다 서로 강하게 의존하고 있어서 상태가 어긋날 때 복구가 많이 어려워 보여요.... 특히 page를 먼저 올리고 요청이 실패하거나 skip되면 "현재 몇 페이지를 실제로 가지고 있는지"가 애매합니다. 그래서 상태를 합칠지 나눌지보다, "요청 전 상태"와 "요청 성공 후 상태"를 어디서 확정할지를 먼저 정리해보시면 구조 판단이 더 쉬워질 것 같아요!!
rate를 위한 저장소(rateRepository)를 만들어서 처리하고 있는데 State vs Repository, MovieList[] vs Rate {} 형태가 좋을지 고민했습니다.
제 생각에는 이 값은 화면 상태라기보다 "저장소에서 읽고 쓰는 데이터"에 더 가까워 보이는데요.... Repository로 두는 방향이 더 자연스럽지 않나 생각이 듭니다. 다만 지금은 인터페이스를 만들었어도 구현체를 렌더링 파일 안에서 바로 생성하고 있어서, 실제로는 저장소 교체가 쉬운 구조까지는 아직 아닌 것 같아요. detailModal은 "현재 평점을 보여주고 클릭 결과를 반영하는 역할"만 맡고, 어떤 저장소를 쓸지는 바깥에서 주입받게 되면 고민하신 방향이 훨씬 명확해지지 않을까 생각해요!
고생 많으셨습니다 먼지!! 혹시 궁금한 점 있으면 커멘트 추가로 남겨주세요 ㅎㅎ 몇 가지만 확인해주시면 될 것 같아요 ㅎㅎ
| @@ -0,0 +1,17 @@ | |||
| class MovieListState { | |||
There was a problem hiding this comment.
서버 응답 상태를 별도 객체로 뺀 선택은 좋았는데요!
지금은 total_pages 하나만 들고 있어서 상태 객체의 책임이 아직 조금 애매하게 느껴...져요!
만약 MovieListState`가 앞으로도 계속 이 이름 그대로 유지한다면, 어떤 값들까지 책임지는 게 좋을지 먼지가 생각해보시ㅕㄴ 좋을 것 같아요!!
| ...headers, | ||
| }; | ||
|
|
||
| const res = await fetch(finalUrl, { |
There was a problem hiding this comment.
fetch 호출을 이 파일로 모아 둔 덕분에 예외 처리나 헤더 정책을 한곳에서 모아두니 좋네요 ㅎㅎ
추가로 생각해보면 좋을 것!
네트워크 실패도 공통으로 처리하면 좋을 것 같다는 생각인데, 먼지 생각은 어떠세요??
| let responseData; | ||
| try { | ||
| responseData = await res.json(); | ||
| } catch (e) { | ||
| console.error(e); | ||
| responseData = await res.text(); | ||
| } |
There was a problem hiding this comment.
네트워크 오류인 경우에 여기 코드까지 진행될 수 있는지 검토 한번만 부탁드릴게요!
| let isFetching = false; | ||
|
|
||
| const refetch = async ({ page }: { page: number }) => { | ||
| if( isFetching ) return; |
There was a problem hiding this comment.
무한스크롤에서 중복 요청을 막기 위해 isFetching을 둔 판단 좋아요!!
++ 호출부에서 isFetching 상태를 알수 있도록 하면 더 좋을 것 같아요! 별도의 ui처리가 필요할 수도 있으니까요 ㅎㅎ
| setMovieRate(id: number, rate: number): void | ||
| } | ||
|
|
||
| class RateLocalStroageRepository implements RateRepository { |
There was a problem hiding this comment.
RateRepository 인터페이스를 따로 둔 시도 좋았습니다!!
만약에 localStorage 대신에 다른 저장소를 사용하게 된다면 어느 부분들이 수정될까요?
그럴 가능성이 있을지 없을지도 고민해보면 좋을 것 같아요!
| const rates = localStorage.getItem('rates') || "{}"; | ||
| return JSON.parse(rates) || {}; | ||
| } |
There was a problem hiding this comment.
JSON.parse() 에서 localStorage 값이 깨져 있으면 모달 전체를 못 여는 것처럼... 보이는데요?
확인 한 번 해주세요! 그리고 어떻게 수정할 수 있을지도 고민해주세요 ㅎㅎ
| export const getMovieMovieId = async ({ id, }: { id: number }): Promise<MovieInfo> => { | ||
| const url = `/movie/${id}`; | ||
| const { data } = await requestAjax(url); | ||
|
|
||
| return data; | ||
| }; |
There was a problem hiding this comment.
getMovieMovieId라는 이름은 동작을 이해하는 데에 혼란을 주기 쉬운 것 같아요. 다음 둘 중에 하나로 변경하는 거 어떠세요?
getMovieByIdgetMovieDetail
3 participants
🎯 미션 소개
🕵️ 셀프 리뷰(Self-Review)
제출 전 체크 리스트
리뷰 요청 & 논의하고 싶은 내용
1) 이번 단계에서 가장 많이 고민했던 문제와 해결 과정에서 배운 점
평점 저장과정에서 localStorage 를 사용하다가 api로 변경 고려하며 요구사항을 해결하는 것을
앞으로 변경되는 서비스를 구현하면서 어떻게 코드를 작성해야 변경을 최소화 할 수 있을지
고민을 하며 작성해야한다는 것을 몸으로 느겼습니다.
이 평점을 저장할 곳을 변경해도 유지가 되도록 하는 부분을 고려하는 코드를 작성하면셔
상속을 사용해보고 solid의 d인 의존성 주입이 이런식으로 사용될수 있구나 하는것을 느꼈습니다.
2) 이번 리뷰를 통해 논의하고 싶은 부분
상세 팝업 오픈을 위해서 이벤트 바인딩을 renderMovieList에서 하고 있습니다
render에서는 순수하게 render만 처리하고 싶었는데
renderMovieList로 인해 dom이 생성되고 나서야 이벤트를 바인딩을 해야해서
render함수에 안에서 이벤트바인딩을 하고 있는 것이 걸립니다
어떻게 분리를 할수 있을지 잘 모르겠습니다. 우코는 어떻게 하시는지 여쭈어봐도 될까요?
무한 스크롤 구현시 마지막 데이터까지 볼러온 경우 더 api 호출을 하지 않기 위해서
total_page가 필요해 MovieListState라는 class를 만들어 여기에 넣어 관리하였습니다
이런식으로 state를 관리하는 케이스로 pageState가 있습니다
pageState, MovieListState 이렇게 분산해서 저장하는게 맞는건지 의문이 들었습니다
pageState는 사용자가 선택한 값이고 MovieListState 는 서버의 값인데 같은 states 폴더에 두는 것이 옳지 않다는 생각도 듭니다
별점의 경우 total_page를 저장하던 MovieListState에서 저장을 할까 하다
별도의 저장소를 만들었습니다
MovieListState를 없애고 rate 저장소와 합쳐야할까 하다가 별도로 만들었습니다
rate를 위한 저장소(rateRepository)를 만들어서 처리하고 있는데
MovieListRespository 라는걸 만들어서
이곳에서 movieList를만들고 배열로 저장하고 이 배열의 item안에 별점을 관리하는게 맞나
아니면 별점 만 관리하는 걸 만드는게 나을까 고민했습니다
todo 나 checkbox가 있는 table 같은 걸 만들때도 완료 여부를 확인하는 값을 todo 배열에 같이 포함시켜야하는지 스스로 고민했던 것 같습니다
특히 내 별점의 경우 완전히 서버 데이터인 movieList에 포함되지 않고 별도의 서버데이터인것 같기도해서 별도로 처리하였습니다
우코의 생각에는 별점 상태를 어디에 저장하고(State vs Respository) 어떤 형태(MovieList[] vs Rate {})로 저장하는게 좋나요?
리뷰어에게 PR을 보낼때 아래 처럼 고민을 적으려고해
무한 스크롤을 구현하면서 3번의 버그를 만났습니다
해당 현상은 초기 데이터 로딩(initLoad)으로 인한 rendering 완료시점과
scroll event handler 가 서로 독립적로로 으로실행되면서
타이밍이 어긋나느데서 발생하는 문제로 파악 했습니다
a. 새로고침
b. 브라우저가 이전 scroll 위치 복원
c. 아직 page1 렌더링 안됨 (DOM height 작음)
d. scroll 조건 (isBottom) 이미 true
이를 해결하기 위해서 isFetching 변수를 만들어서 api 호출이 완료되면 값을 변경하는 식으로 하여
중복 호출을 막으려 하얐습니다
하지만 이 방법으로는 버그를 잡을수 없었고 간혈적으로 page2 요청까지 일어나는 현상이 계속 됩니다
저는 이 문제를 초기 로딩 완료 이전에 scroll 조건이 먼저 만족되는 문제로 이해하고 있으며
이를 기준으로 해결 방향을 고민하고 있습니다
다만
초기 로딩 완료 이전까지 scroll 이벤트를 무시하는 방식은
전역 상태 플래그에 의존하는 문제처럼 느껴져 적절한 해결책인지 고민이 있습니다
또한 스크롤 계산 방식 자체의 문제라기보다는 이벤트 발생 시점과 렌더링 시점의 타이밍 문제로 보고 있어
IntersectionObserver 로 변경하더라도 간헐적으로 동일한 문제가 발생하는 것은 같았습니다
이러한 상황에서 제가 문제를 잘못 이해하고 있는 부분이 있는지
더 근본적인 해결방향이 있는지 피드백 부탁드립니다
step2 요구사항을 구현하면서 기존에 간단하게 작성한 fetch 함수 대신에 이전부터 고민해오던 공통 fetch 함수를 적용하는 방향으로 구현했습니다.
공통 fetch 함수를 여러 방식으로 고민해왔던 부분이라 구조나 설계 방향이 적절한지 피드백받고 싶습니다.
step1 리뷰로 작성했던 공통함수(requestAjax)를 영화 미션에 맞게 수정헤
src/services/http.ts에 두었습니다.관련해서 구조나 책임 분리 측면에서 더 나은 방향이 있다면
편하게 피드백 주시면 감사하겠습니다!
✅ 리뷰어 체크 포인트