Conversation
템플릿에 맞게 body에 접근하여 생성
dataset 사용
커스텀 이벤트를 사용하여 무한스크롤 구현
- 스토를링과 isLoading을 추가하여 fetch의 중복 호출을 막음 - totalpage를 멤버 변수로 추가하여 마지막 요소때 fetch막음
Element를 맵으로 저장하여 한번 생성한 페이지는 다시 생성하지 않도록함
- 윈도우 이벤트 리스너가 다른탭에서도 살아있는 문제 해결(className) - 입력값 까지 같이 캐싱되는 문제 해결(input.value="")
|
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영화 상세 정보 모달 기능을 추가하는 중규모 기능 개선사항입니다. 무한 스크롤 기반의 페이지네이션으로 "더 보기" 버튼을 대체하고, 페이지 캐싱 메커니즘을 도입했습니다. 새로운 Modal 및 SubmitRate 컴포넌트를 추가하여 사용자가 영화 세부정보를 보고 평점을 남길 수 있도록 지원합니다. LocalStorage를 통한 사용자 평점 저장, API 확장(영화 상세 조회 엔드포인트), 이벤트 기반 아키텍처 개선, 반응형 CSS 업데이트가 포함됩니다. Estimated code review effort🎯 4 (Complex) | ⏱️ ~50 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
🧹 Nitpick comments (14)
src/components/header/SearchHeader.ts (1)
16-17:$justLayout요소의 목적을 명확히 해주세요.빈
div요소가 클래스나 ID 없이 추가되어 있습니다. 이 요소의 역할이 무엇인지 (예: flexbox 레이아웃을 위한 spacer) 코드만으로는 파악하기 어렵습니다.의도를 명확히 하기 위해 다음 중 하나를 고려해 보세요:
- 목적을 나타내는 클래스명 추가 (예:
layout-spacer)- 간단한 주석으로 역할 설명
이렇게 하면 향후 유지보수 시 혼란을 줄일 수 있습니다.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/components/header/SearchHeader.ts` around lines 16 - 17, $justLayout is an empty div whose purpose is unclear; update its declaration in SearchHeader (the $justLayout const) to either add a descriptive class name (e.g., "layout-spacer" or "search-header-spacer") or add an inline comment above its creation explaining its role (e.g., spacer for flex alignment), and ensure the same descriptive class/comment is used where $justLayout is appended alongside Logo() and SearchForm(onSubmit) so its intent is obvious to future readers.src/storage/LocalStorage.ts (2)
17-18: 전역localStorage와 이름 충돌을 피하세요.내보내는 변수명
localStorage가 브라우저의 전역window.localStorage를 섀도잉합니다. 이로 인해 다른 모듈에서 혼란이 발생하거나, 의도치 않게 전역 객체 대신 이 래퍼를 참조할 수 있습니다.
rateStorage,movieRatingStorage등 목적을 명확히 드러내는 이름으로 변경하는 것을 고려해 보세요.💡 제안된 수정
-const localStorage = new LocalStorage(); -export default localStorage; +const rateStorage = new LocalStorage(); +export default rateStorage;🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/storage/LocalStorage.ts` around lines 17 - 18, The exported variable localStorage shadows the browser global window.localStorage; rename the instance to a descriptive identifier (e.g., rateStorage or movieRatingStorage) where it’s created (new LocalStorage()) and update the default export to use that new name, and then update any import sites to the new name; ensure the class LocalStorage remains unchanged and only the instance identifier and export are renamed to avoid the global name collision.
1-6: TypeScript 타입 명시를 권장합니다.
#myStorage필드에 명시적 타입 어노테이션이 없습니다. TypeScript의 장점을 활용하여 타입을 명확히 지정하면 코드 가독성과 유지보수성이 향상됩니다.💡 제안된 수정
class LocalStorage { - `#myStorage`; + `#myStorage`: Storage; constructor() { this.#myStorage = window.localStorage; }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/storage/LocalStorage.ts` around lines 1 - 6, The private field `#myStorage` in class LocalStorage lacks a TypeScript type annotation; update its declaration to include the appropriate type (e.g., Storage) and keep the constructor assignment as is. Locate the class LocalStorage and change the field declaration for `#myStorage` to explicitly type it (for example, "#myStorage: Storage;") so the compiler and readers know it holds window.localStorage. Ensure any usages of `#myStorage` remain compatible with the chosen Storage type.src/components/main/MoreButton.ts (1)
1-8: 주석 처리된 코드 삭제를 권장합니다.더 이상 사용하지 않는 코드는 주석으로 남기기보다 삭제하는 것이 좋습니다. Git과 같은 버전 관리 시스템이 이력을 보존하므로, 필요시 언제든 복원할 수 있습니다. 주석 처리된 코드는 코드베이스의 가독성을 저해하고 유지보수 혼란을 야기할 수 있습니다.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/components/main/MoreButton.ts` around lines 1 - 8, Remove the dead commented-out component block for MoreButton: delete the entire commented export/function block (the "// export const MoreButton ..." through its closing "};") so the codebase no longer contains unused commented code; if the component is intended to be kept, restore it as an actual export function named MoreButton with its event listener instead of leaving it commented.src/components/header/SearchForm.ts (1)
23-26: 빈 검색어 처리 검토 필요현재 구현에서는 빈 문자열도
onSubmit으로 전달됩니다. 빈 검색어에 대한 처리가 호출하는 쪽(onSubmit콜백)에서 적절히 이루어지고 있는지 확인해 보세요.빈 검색어를 허용하지 않으려면
onSubmit호출 전에 검증 로직을 추가하는 것이 좋습니다.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/components/header/SearchForm.ts` around lines 23 - 26, The current handler reads const query = $input.value and always calls onSubmit(query), which passes empty or whitespace-only queries through; update the submit logic in SearchForm to validate the input before calling onSubmit by trimming $input.value and returning early (or not calling onSubmit) when the trimmed string is empty, and only clear $input.value after a successful submit; refer to the $input element (query from $input.value), the $form submission flow and the onSubmit callback when making the change.src/api/fetchApi.ts (1)
48-53: fetchMovieDetails 반환 타입 추론 확인
fetchAPI호출 시 제네릭 타입 인자를 명시하지 않으면 TypeScript가ResponseMovie | MovieDetail로 추론할 수 있습니다. 타입 안전성을 높이기 위해 명시적으로 타입 인자를 전달하는 것을 고려해 보세요.return fetchAPI<MovieDetail>({ path: PATH.MOVIE_DETAIL(movie_id), params: {}, });🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/api/fetchApi.ts` around lines 48 - 53, The fetchMovieDetails function relies on fetchAPI without an explicit generic, allowing TypeScript to infer a union type; update the fetchAPI invocation in fetchMovieDetails to pass the concrete generic type MovieDetail (i.e., call fetchAPI<MovieDetail>(...)) so the function returns Promise<MovieDetail> explicitly and improves type safety; locate the fetchMovieDetails function and its fetchAPI call to make this change.src/components/common/Rate.ts (1)
3-10: 타입 및 반올림 로직 검토두 가지 개선 포인트가 있습니다:
vote_average파라미터가string으로 정의되어 있지만,Number()로 변환 후 사용합니다. API 응답 타입(MovieData.vote_average)은number이므로 타입 일관성을 고려해 보세요.
Math.round(Number(vote_average) * 10) / 10후toFixed(1)를 호출하는데,toFixed(1)만으로도 소수점 첫째 자리까지 반올림됩니다.어떤 의도로 이렇게 구현하셨는지, 그리고 단순화할 수 있는지 검토해 보시면 좋겠습니다.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/components/common/Rate.ts` around lines 3 - 10, The Rate component should accept a numeric vote_average (use the API type MovieData.vote_average) and simplify rounding: change the Rate signature to take vote_average: number and replace the Math.round(Number(vote_average) * 10) / 10 logic with vote_average.toFixed(1) (or if you must accept string, use Number(vote_average).toFixed(1)); then assign that result to $span.textContent. This ensures type consistency with MovieData.vote_average and removes the redundant Math.round + toFixed combination in the Rate function.src/utils/event.ts (1)
19-26: throttle vs debounce 동작 검토현재 구현은 이벤트 발생 후
delay시간이 지난 뒤에 이벤트를 발생시키므로, 실제로는 debounce 패턴에 가깝습니다.throttle은 일정 간격으로 즉시 실행하고, debounce는 마지막 호출 후 일정 시간 대기 후 실행합니다. 무한 스크롤 용도라면 현재 동작(스크롤 멈춘 후 300ms 뒤 로드)이 의도한 것인지 확인해 보세요.
즉시 반응하는 throttle이 필요하다면 구현 방식을 재검토해 보시기 바랍니다.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/utils/event.ts` around lines 19 - 26, The current scrollEvent implementation behaves like debounce (waits delay then fires) rather than a throttle; decide intended behavior and update scrollEvent accordingly: if you want debounce, rename scrollEvent or the throttle object to debounce and keep the current setTimeout semantics; if you want a true throttle, change scrollEvent so it dispatches CustomEvent(CUSTOM_EVENT.SCROOL_END) immediately when throttle.timer is null and then sets throttle.timer = setTimeout(() => throttle.timer = null, throttle.delay) to block further dispatches until delay elapses; reference the scrollEvent function and the throttle object and ensure CUSTOM_EVENT.SCROOL_END is still the event name you want.src/pages/SearchPage.ts (1)
67-71: 스켈레톤 렌더링 순서를 검토해 보세요.현재
renderSkeletons가#page >#totalPage`` 체크보다 먼저 호출됩니다. 마지막 페이지를 넘어선 경우에도 스켈레톤이 잠시 보였다가 사라지는 현상이 발생할 수 있습니다.페이지 범위 검증을 먼저 수행하면 어떨까요?
💡 순서 변경 제안
async `#appendMovies`(): Promise<ResponseMovie | void> { - this.#main.renderSkeletons(this.#page); - try { if (this.#page > this.#totalPage) return; + this.#main.renderSkeletons(this.#page); const response = await fetchSearchMovies(this.#getQuery(), this.#page);🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/pages/SearchPage.ts` around lines 67 - 71, Move the page-range check before rendering skeletons in `#appendMovies`: check if this.#page > this.#totalPage and return early, then call this.#main.renderSkeletons(this.#page) only when the page is valid; update the control flow in the async `#appendMovies` method so renderSkeletons is invoked after the guard that uses this.#page and this.#totalPage.src/pages/HomePage.ts (2)
69-73: 스켈레톤 렌더링 순서를 검토해 보세요.SearchPage와 동일하게,
renderSkeletons가 페이지 범위 검증(#page >#totalPage``)보다 먼저 호출됩니다. 마지막 페이지 이후에도 스켈레톤이 잠시 나타났다가 사라질 수 있습니다.💡 순서 변경 제안
async `#appendMovies`(): Promise<ResponseMovie | void> { - this.#main.renderSkeletons(this.#page); - try { if (this.#page > this.#totalPage) return; + this.#main.renderSkeletons(this.#page); const response = await fetchPopularMovies(this.#page);🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/pages/HomePage.ts` around lines 69 - 73, In `#appendMovies`(), renderSkeletons(this.#page) is called before checking the page bounds which causes skeletons to flash when this.#page > this.#totalPage; change the order so the page-range guard (if (this.#page > this.#totalPage) return;) runs first and only call this.#main.renderSkeletons(this.#page) after the guard passes (matching the SearchPage behavior).
11-44: HomePage와 SearchPage 간 코드 중복이 있습니다.두 페이지 클래스가 페이지네이션 상태 관리(
#page,#totalPage,#isLoading), 무한 스크롤 이벤트 처리,#onDetail핸들러, 에러 처리 등에서 거의 동일한 로직을 가지고 있습니다.현재 기능이 정상 동작한다면 당장 변경할 필요는 없지만, 추후 유지보수를 위해 공통 로직을 추상화하는 방안을 고려해 보시면 좋겠습니다. 예를 들어 베이스 클래스나 composition 패턴을 적용할 수 있습니다.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/pages/HomePage.ts` around lines 11 - 44, HomePage and SearchPage duplicate pagination and infinite-scroll logic (properties `#page`, `#totalPage`, `#isLoading`), the scroll event handler (window.addEventListener(CUSTOM_EVENT.SCROOL_END,...)), common handlers (`#onDetail`), and error/loading handling; extract these into a shared abstraction (e.g., a PaginatedPage base class or a composition helper) that declares and manages `#page`, `#totalPage`, `#isLoading`, methods like `#loadMore` and `#initialFetch`, and the scroll subscription, then have HomePage and SearchPage extend or compose that class and remove the duplicated implementations from their constructors and methods so they reuse the shared handlers and state.src/components/modal/SubmitRate.ts (1)
5-6: 변수명 오타가 있습니다.
#$submiteRateContainer와#$starContanier에 오타가 있습니다. 코드의 일관성과 가독성을 위해 수정을 고려해 주세요.
submiteRateContainer→submitRateContainerstarContanier→starContainer🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/components/modal/SubmitRate.ts` around lines 5 - 6, The private field names in SubmitRate.ts contain typos: rename the class private properties #$submiteRateContainer -> #$submitRateContainer and #$starContanier -> #$starContainer, and update every reference to those symbols (constructor assignments, methods, querySelector calls, event listeners, template/CSS references, and any type annotations) to use the corrected identifiers so the class compiles and uses consistent names.src/main.ts (1)
11-33: 페이지 캐시가 무한히 증가할 수 있습니다.
PAGE_CACHE가 전체 URL 경로(fullpath)를 키로 사용하므로, 서로 다른 검색어마다 새로운SearchPage인스턴스가 생성되어 캐시에 쌓입니다. 예:/search?query=아바타,/search?query=인터스텔라등.장시간 사용 시 메모리 문제가 발생할 수 있습니다. 캐시 크기 제한이나 LRU(Least Recently Used) 전략을 고려해 보시겠어요?
추가로, PR 설명에 언급된 "윈도우 이벤트 리스너 누적" 문제와도 연관됩니다. 각 페이지가 생성될 때
window.addEventListener로 스크롤 이벤트를 등록하는데, 캐시된 페이지가 많아지면 리스너도 계속 쌓이게 됩니다.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/main.ts` around lines 11 - 33, PAGE_CACHE can grow unbounded because router() uses fullpath (including query) as the cache key; limit growth by switching to a bounded cache with eviction (e.g., an LRU strategy) and by normalizing keys for pages that should be shared (e.g., use route.path for SearchPage instead of fullpath) — implement a small LRUMap wrapper used by PAGE_CACHE and use it inside router() when getting/setting. Also ensure pages provide a dispose() method (add to HomePage/SearchPage classes) that removes any window event listeners, and call page.dispose() when evicting entries from the cache (or before replacing a cached entry) so listeners don’t accumulate.src/components/main/Main.ts (1)
56-62: 스켈레톤 제거 후 Map에서도 삭제하면 메모리 관리에 도움이 됩니다.
removeSkeletons에서 DOM에서는 요소를 제거하지만#$skeletonsMap에서는 삭제하지 않습니다. 무한 스크롤로 많은 페이지를 로드하는 경우 Map에 빈 참조가 계속 쌓일 수 있습니다.💡 Map 정리 추가
removeSkeletons(page: number) { if (!this.#$skeletons.has(String(page))) { return; } const $skeletonList = this.#$skeletons.get(String(page)); $skeletonList?.forEach(($skeleton) => $skeleton.remove()); + this.#$skeletons.delete(String(page)); }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/components/main/Main.ts` around lines 56 - 62, The removeSkeletons method currently removes DOM nodes but leaves an empty entry in the #$skeletons Map; update removeSkeletons to also remove the Map entry after iterating ($skeletonList) by calling the Map's delete with the stringified page key (String(page)) so the Map no longer retains the empty reference; ensure you still guard with this.#$skeletons.has(String(page)) and perform delete after the forEach removal.
🤖 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/spec.cy.ts`:
- Around line 26-33: The test can flake on CI due to the 300ms throttle in
src/utils/event.ts and network variability; update the test that calls
cy.scrollTo('bottom') to wait longer for the API alias by changing
cy.wait('@getNextPopularMovies') to cy.wait('@getNextPopularMovies', { timeout:
10000 }) (or another increased timeout) so the
scrollEvent()/CUSTOM_EVENT.SCROOL_END debounce in src/main.ts has ample time to
trigger the second fetch after the initial cy.wait('@getPopularMovies'); keep
the same aliases and assertions ('.thumbnail-list > li' length check) but
increase the wait timeout to stabilize CI runs.
In `@src/api/fetchApi.ts`:
- Line 22: Remove the leftover debugging console.log(resultUrl) from
src/api/fetchApi.ts (or guard it behind a dev-only check); either delete the
console.log call or wrap it so it only runs in non-production (e.g., check
process.env.NODE_ENV or a DEBUG env var) to prevent debug output in production
builds while preserving dev visibility when needed.
In `@src/api/types.ts`:
- Around line 12-20: MovieDetail.vote_average is incorrectly typed as string
while TMDB returns a JSON number and MovieData.vote_average is already a number;
update the MovieDetail type to use number for vote_average to match the API and
MovieData, then search for usages of MovieDetail.vote_average (e.g., any code
that assumes a string) and update parsing/formatting (toFixed, toString, etc.)
as needed to handle numeric values consistently.
In `@src/components/main/Main.ts`:
- Around line 28-33: Click handler on $ul can call onDetail(NaN) when the click
target isn't an li or the li has no data-id; update the listener in Main.ts to
validate the clicked element and id before calling onDetail: inside the $ul
click callback (the $ul variable and existing $li extraction), check that $li is
non-null and that $li.dataset.id is defined (use parseInt or Number safely), and
return early if validation fails so onDetail is only invoked with a valid
numeric id; ensure you reference $li, $li.dataset.id and onDetail when making
the change.
In `@src/components/modal/Modal.ts`:
- Around line 50-55: In `#update`(movie: MovieDetail) guard against
movie.poster_path being null before calling getOriginalImageUrl: if poster_path
is falsy, set the img src to a placeholder/default image (or a
getPlaceholderImage() helper) instead of calling getOriginalImageUrl(null);
update the assignment that targets $<HTMLImageElement>(this.#$modal,
'.modal-image img').src and ensure any helper used (getOriginalImageUrl or new
getPlaceholderImage) is referenced so reviewers can locate the change.
- Around line 89-93: Remove the leftover debug console.log from the Modal.close
method: delete the statement console.log('object') inside the close() function
so only the DOM class removals (this.#$body.classList.remove('modal-open') and
this.#$modal.classList.remove('active')) remain; ensure no other debug logs are
left in the Modal class.
- Around line 71-74: The open(movie: MovieDetail) method on Modal overwrites the
body element's existing classes by setting this.#$body.className = 'modal-open';
change this to add the class instead (use classList.add('modal-open')) so
existing classes are preserved and behavior is consistent with close() which
uses classList.remove; update the open method where this.#$body is referenced
(and leave this.#$modal.classList.add('active') and this.#update(movie)
unchanged).
In `@src/pages/HomePage.ts`:
- Around line 107-115: In `#onDetail` remove the leftover debug console.log
statements (console.log(this.#$modal) and console.log(e)); keep the async
fetchMovieDetails(movie_id) call and this.#$modal.open(movie) intact and replace
the error console.log with proper error handling (e.g., use console.error(e) or
a project logger or rethrow) so no debug-only console.log remains.
In `@src/pages/SearchPage.ts`:
- Around line 34-37: 현재 코드 uses querySelector(`#${query}`) inside the
CUSTOM_EVENT.SCROOL_END listener which breaks when query contains spaces/special
chars; replace that selector usage with a safer check: either use
document.getElementById(query) if query is an element id, or call
CSS.escape(query) before building the selector, or (better) track and check the
actual DOM element reference instead of string names; update the listener in
window.addEventListener(CUSTOM_EVENT.SCROOL_END, ...) to use
document.getElementById(query) or an escaped selector and then call
this.#loadMore() only if the element exists.
- Around line 110-118: Remove the debug console.log calls in the `#onDetail`
method: delete console.log(this.#$modal) and replace console.log(e) in the catch
block with proper error handling — e.g., show a user-facing error via
this.#$modal (or another UI error handler) and/or log the error through your app
logger; ensure fetchMovieDetails(movie_id) failures surface a friendly message
instead of raw console output.
In `@src/utils/event.ts`:
- Line 4: There's a typo in the exported event constant name: rename the
constant SCROOL_END to SCROLL_END in the src/utils/event.ts definition and
update every usage/reference throughout the codebase (imports, switch/case,
event emitters/listeners) to use SCROLL_END; ensure any exports and re-exports
(e.g., in enums or objects) and tests are updated and run the build to catch
missed occurrences so no runtime import/usage breaks occur.
In `@styles/main.css`:
- Around line 312-318: The CSS rule for .top-rated-container and
.search-container uses justify-items which has no effect on a flex container;
remove the justify-items declaration and, if horizontal centering of child items
was intended, replace it with justify-content (e.g., center) while keeping
align-items for vertical alignment, or adjust whichever property
(justify-content / align-items / flex-direction) matches the intended layout;
update the rule targeting .top-rated-container and .search-container
accordingly.
In `@styles/modal.css`:
- Line 123: The CSS rule in styles/modal.css uses an invalid padding declaration
"padding: 40 24"; update the padding property in that rule (padding) to include
explicit units for both values (e.g., px, rem, etc.) so it becomes a valid CSS
shorthand like "padding: 40px 24px;" using the same selector where padding is
defined.
- Around line 138-150: In the .modal rule consolidate duplicate/conflicting
declarations: remove the earlier border-radius: 0 and any extra width lines so
only one border-radius (keep border-radius: 16px) and one width remain, and fix
the invalid width: 390 by adding a unit (e.g., width: 390px) or replace it with
the intended width (e.g., 800px) — ensure height/top/bottom/padding/gap remain
consistent with the chosen width and remove any redundant duplicate properties.
In `@styles/thumbnail.css`:
- Around line 5-10: Remove the unnecessary blank line before the first
declaration in the CSS rule that contains "justify-content" and
"grid-template-columns" (the rule block with those properties); delete the empty
line so declarations start immediately after the selector, satisfying
Stylelint's declaration-empty-line-before rule (or adjust the rule config if an
exception is required).
---
Nitpick comments:
In `@src/api/fetchApi.ts`:
- Around line 48-53: The fetchMovieDetails function relies on fetchAPI without
an explicit generic, allowing TypeScript to infer a union type; update the
fetchAPI invocation in fetchMovieDetails to pass the concrete generic type
MovieDetail (i.e., call fetchAPI<MovieDetail>(...)) so the function returns
Promise<MovieDetail> explicitly and improves type safety; locate the
fetchMovieDetails function and its fetchAPI call to make this change.
In `@src/components/common/Rate.ts`:
- Around line 3-10: The Rate component should accept a numeric vote_average (use
the API type MovieData.vote_average) and simplify rounding: change the Rate
signature to take vote_average: number and replace the
Math.round(Number(vote_average) * 10) / 10 logic with vote_average.toFixed(1)
(or if you must accept string, use Number(vote_average).toFixed(1)); then assign
that result to $span.textContent. This ensures type consistency with
MovieData.vote_average and removes the redundant Math.round + toFixed
combination in the Rate function.
In `@src/components/header/SearchForm.ts`:
- Around line 23-26: The current handler reads const query = $input.value and
always calls onSubmit(query), which passes empty or whitespace-only queries
through; update the submit logic in SearchForm to validate the input before
calling onSubmit by trimming $input.value and returning early (or not calling
onSubmit) when the trimmed string is empty, and only clear $input.value after a
successful submit; refer to the $input element (query from $input.value), the
$form submission flow and the onSubmit callback when making the change.
In `@src/components/header/SearchHeader.ts`:
- Around line 16-17: $justLayout is an empty div whose purpose is unclear;
update its declaration in SearchHeader (the $justLayout const) to either add a
descriptive class name (e.g., "layout-spacer" or "search-header-spacer") or add
an inline comment above its creation explaining its role (e.g., spacer for flex
alignment), and ensure the same descriptive class/comment is used where
$justLayout is appended alongside Logo() and SearchForm(onSubmit) so its intent
is obvious to future readers.
In `@src/components/main/Main.ts`:
- Around line 56-62: The removeSkeletons method currently removes DOM nodes but
leaves an empty entry in the #$skeletons Map; update removeSkeletons to also
remove the Map entry after iterating ($skeletonList) by calling the Map's delete
with the stringified page key (String(page)) so the Map no longer retains the
empty reference; ensure you still guard with this.#$skeletons.has(String(page))
and perform delete after the forEach removal.
In `@src/components/main/MoreButton.ts`:
- Around line 1-8: Remove the dead commented-out component block for MoreButton:
delete the entire commented export/function block (the "// export const
MoreButton ..." through its closing "};") so the codebase no longer contains
unused commented code; if the component is intended to be kept, restore it as an
actual export function named MoreButton with its event listener instead of
leaving it commented.
In `@src/components/modal/SubmitRate.ts`:
- Around line 5-6: The private field names in SubmitRate.ts contain typos:
rename the class private properties #$submiteRateContainer ->
#$submitRateContainer and #$starContanier -> #$starContainer, and update every
reference to those symbols (constructor assignments, methods, querySelector
calls, event listeners, template/CSS references, and any type annotations) to
use the corrected identifiers so the class compiles and uses consistent names.
In `@src/main.ts`:
- Around line 11-33: PAGE_CACHE can grow unbounded because router() uses
fullpath (including query) as the cache key; limit growth by switching to a
bounded cache with eviction (e.g., an LRU strategy) and by normalizing keys for
pages that should be shared (e.g., use route.path for SearchPage instead of
fullpath) — implement a small LRUMap wrapper used by PAGE_CACHE and use it
inside router() when getting/setting. Also ensure pages provide a dispose()
method (add to HomePage/SearchPage classes) that removes any window event
listeners, and call page.dispose() when evicting entries from the cache (or
before replacing a cached entry) so listeners don’t accumulate.
In `@src/pages/HomePage.ts`:
- Around line 69-73: In `#appendMovies`(), renderSkeletons(this.#page) is called
before checking the page bounds which causes skeletons to flash when this.#page
> this.#totalPage; change the order so the page-range guard (if (this.#page >
this.#totalPage) return;) runs first and only call
this.#main.renderSkeletons(this.#page) after the guard passes (matching the
SearchPage behavior).
- Around line 11-44: HomePage and SearchPage duplicate pagination and
infinite-scroll logic (properties `#page`, `#totalPage`, `#isLoading`), the scroll
event handler (window.addEventListener(CUSTOM_EVENT.SCROOL_END,...)), common
handlers (`#onDetail`), and error/loading handling; extract these into a shared
abstraction (e.g., a PaginatedPage base class or a composition helper) that
declares and manages `#page`, `#totalPage`, `#isLoading`, methods like `#loadMore` and
`#initialFetch`, and the scroll subscription, then have HomePage and SearchPage
extend or compose that class and remove the duplicated implementations from
their constructors and methods so they reuse the shared handlers and state.
In `@src/pages/SearchPage.ts`:
- Around line 67-71: Move the page-range check before rendering skeletons in
`#appendMovies`: check if this.#page > this.#totalPage and return early, then call
this.#main.renderSkeletons(this.#page) only when the page is valid; update the
control flow in the async `#appendMovies` method so renderSkeletons is invoked
after the guard that uses this.#page and this.#totalPage.
In `@src/storage/LocalStorage.ts`:
- Around line 17-18: The exported variable localStorage shadows the browser
global window.localStorage; rename the instance to a descriptive identifier
(e.g., rateStorage or movieRatingStorage) where it’s created (new
LocalStorage()) and update the default export to use that new name, and then
update any import sites to the new name; ensure the class LocalStorage remains
unchanged and only the instance identifier and export are renamed to avoid the
global name collision.
- Around line 1-6: The private field `#myStorage` in class LocalStorage lacks a
TypeScript type annotation; update its declaration to include the appropriate
type (e.g., Storage) and keep the constructor assignment as is. Locate the class
LocalStorage and change the field declaration for `#myStorage` to explicitly type
it (for example, "#myStorage: Storage;") so the compiler and readers know it
holds window.localStorage. Ensure any usages of `#myStorage` remain compatible
with the chosen Storage type.
In `@src/utils/event.ts`:
- Around line 19-26: The current scrollEvent implementation behaves like
debounce (waits delay then fires) rather than a throttle; decide intended
behavior and update scrollEvent accordingly: if you want debounce, rename
scrollEvent or the throttle object to debounce and keep the current setTimeout
semantics; if you want a true throttle, change scrollEvent so it dispatches
CustomEvent(CUSTOM_EVENT.SCROOL_END) immediately when throttle.timer is null and
then sets throttle.timer = setTimeout(() => throttle.timer = null,
throttle.delay) to block further dispatches until delay elapses; reference the
scrollEvent function and the throttle object and ensure CUSTOM_EVENT.SCROOL_END
is still the event name you want.
🪄 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: d99af467-6179-4db3-8d75-cc996ccae99c
📒 Files selected for processing (24)
__test__/api.test.tscypress/e2e/spec.cy.tsindex.htmlsrc/api/constant.tssrc/api/fetchApi.tssrc/api/types.tssrc/components/common/Rate.tssrc/components/common/Star.tssrc/components/header/SearchForm.tssrc/components/header/SearchHeader.tssrc/components/header/TopRate.tssrc/components/main/Main.tssrc/components/main/MoreButton.tssrc/components/main/MovieItem.tssrc/components/modal/Modal.tssrc/components/modal/SubmitRate.tssrc/main.tssrc/pages/HomePage.tssrc/pages/SearchPage.tssrc/storage/LocalStorage.tssrc/utils/event.tsstyles/main.cssstyles/modal.cssstyles/thumbnail.css
| it('스크롤을 내리면 다음 페이지의 영화 20개를 추가로 렌더링한다.', () => { | ||
| cy.visit('http://localhost:5175/'); | ||
| cy.wait('@getPopularMovies'); | ||
|
|
||
| cy.get('.more-button').click(); | ||
| cy.scrollTo('bottom'); | ||
| cy.wait('@getNextPopularMovies'); | ||
|
|
||
| cy.get('.thumbnail-list > li').should('have.length', 40); |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Description: Check if there are any scroll-related test utilities or configurations
# Search for any existing scroll test helpers or wait configurations
rg -n "scroll" --type=ts -C2 cypress/Repository: woowacourse/javascript-movie-review
Length of output: 315
🏁 Script executed:
# Search for scroll event handlers and custom event implementation
rg -n "SCROOL_END|scroll.*Event|scrollTo" --type=ts -C3 src/Repository: woowacourse/javascript-movie-review
Length of output: 2455
🏁 Script executed:
# Check the event utilities mentioned in the comment
cat -n src/utils/event.ts | head -50Repository: woowacourse/javascript-movie-review
Length of output: 873
🏁 Script executed:
# Look for API interceptor setup to understand `@getNextPopularMovies`
rg -n "getNextPopularMovies|@getPopularMovies|cy\.intercept" --type=ts -C2 cypress/Repository: woowacourse/javascript-movie-review
Length of output: 2431
스크롤 기반 테스트의 구조는 적절합니다.
cy.scrollTo('bottom')은 네이티브 스크롤 이벤트를 발생시키고, src/main.ts의 스크롤 리스너가 이를 감지하여 scrollEvent()를 호출합니다. src/utils/event.ts에서 300ms 쓰로틀을 거쳐 CUSTOM_EVENT.SCROOL_END를 디스패치하고, 페이지가 이 이벤트를 수신하여 API 요청을 트리거합니다. cy.wait('@getNextPopularMovies')는 기본값으로 5초 동안 대기하므로, 300ms 쓰로틀 딜레이는 충분히 처리됩니다.
현재 테스트 구조는 단일 스크롤 액션이므로 타이밍 문제 없이 작동해야 합니다. 다만, CI 환경에서 실제로 안정적으로 동작하는지 여러 번 실행해 확인하면 좋습니다.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@cypress/e2e/spec.cy.ts` around lines 26 - 33, The test can flake on CI due to
the 300ms throttle in src/utils/event.ts and network variability; update the
test that calls cy.scrollTo('bottom') to wait longer for the API alias by
changing cy.wait('@getNextPopularMovies') to cy.wait('@getNextPopularMovies', {
timeout: 10000 }) (or another increased timeout) so the
scrollEvent()/CUSTOM_EVENT.SCROOL_END debounce in src/main.ts has ample time to
trigger the second fetch after the initial cy.wait('@getPopularMovies'); keep
the same aliases and assertions ('.thumbnail-list > li' length check) but
increase the wait timeout to stabilize CI runs.
src/api/fetchApi.ts
Outdated
|
|
||
| if (page) params.set('page', String(page)); | ||
| const resultUrl = url + '?' + params.toString(); | ||
| console.log(resultUrl); |
There was a problem hiding this comment.
디버깅용 console.log 제거 필요
프로덕션 코드에 디버깅용 console.log가 남아있습니다. 배포 전에 제거하거나, 필요하다면 환경 변수를 통해 개발 모드에서만 출력되도록 조건부 로깅을 적용하는 것이 좋습니다.
🔧 제안하는 수정
- console.log(resultUrl);📝 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.
| console.log(resultUrl); |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/api/fetchApi.ts` at line 22, Remove the leftover debugging
console.log(resultUrl) from src/api/fetchApi.ts (or guard it behind a dev-only
check); either delete the console.log call or wrap it so it only runs in
non-production (e.g., check process.env.NODE_ENV or a DEBUG env var) to prevent
debug output in production builds while preserving dev visibility when needed.
| export type MovieDetail = { | ||
| id: number; | ||
| title: string; | ||
| overview: string; | ||
| poster_path: string; | ||
| genres: Genres[]; | ||
| vote_average: string; | ||
| release_date: string; | ||
| }; |
There was a problem hiding this comment.
🧩 Analysis chain
🌐 Web query:
TMDB API movie details vote_average type
💡 Result:
In TMDB’s Movie Details response (GET /3/movie/{movie_id}), vote_average is a JSON number (i.e., a float/double), representing the movie’s average user rating. The value may be returned with more than 1 decimal place (TMDB has discussed it showing up to ~3 decimals at times). [1][2]
Sources:
[1] (gist.github.com)
[2] (themoviedb.org)
Citations:
- 1: https://gist.github.com/semusings/c3afb9ce20fce29d37082d25c2945bff
- 2: https://www.themoviedb.org/talk/62b957133d43e0005129ea08?utm_source=openai
MovieDetail.vote_average 타입 수정 필요
TMDB API 검증 결과에 따르면 vote_average는 **숫자 타입(JSON number)**으로 반환됩니다.
현재 코드에서:
MovieData.vote_average는number로 올바르게 정의됨MovieDetail.vote_average는string으로 정의됨
두 타입이 불일치합니다.
다음 질문을 고려하며 해결 방법을 생각해 보세요:
- TMDB API가 실제로 반환하는 타입은 무엇인가요?
- 두 타입 정의가 같은 데이터 소스를 다루고 있다면, 왜 다르게 정의되어 있을까요?
- 타입이 일치하지 않으면 데이터 처리 시 어떤 문제가 발생할 수 있을까요?
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/api/types.ts` around lines 12 - 20, MovieDetail.vote_average is
incorrectly typed as string while TMDB returns a JSON number and
MovieData.vote_average is already a number; update the MovieDetail type to use
number for vote_average to match the API and MovieData, then search for usages
of MovieDetail.vote_average (e.g., any code that assumes a string) and update
parsing/formatting (toFixed, toString, etc.) as needed to handle numeric values
consistently.
| const $ul = $(this.#$element, 'ul'); | ||
| $ul.addEventListener('click', (e) => { | ||
| const $li = (e.target as Element).closest('li'); | ||
| const id = $li?.dataset.id; | ||
| onDetail(Number(id)); | ||
| }); |
There was a problem hiding this comment.
클릭 이벤트 위임 시 유효성 검사가 필요합니다.
현재 구현에서 ul 영역을 클릭했지만 li 요소가 아닌 경우(예: 목록 사이의 빈 공간), $li가 null이 되어 onDetail(NaN)이 호출될 수 있습니다.
이 경우를 어떻게 처리하면 좋을지 생각해 보시겠어요?
💡 힌트
$ul.addEventListener('click', (e) => {
const $li = (e.target as Element).closest('li');
+ if (!$li) return;
const id = $li?.dataset.id;
onDetail(Number(id));
});🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/components/main/Main.ts` around lines 28 - 33, Click handler on $ul can
call onDetail(NaN) when the click target isn't an li or the li has no data-id;
update the listener in Main.ts to validate the clicked element and id before
calling onDetail: inside the $ul click callback (the $ul variable and existing
$li extraction), check that $li is non-null and that $li.dataset.id is defined
(use parseInt or Number safely), and return early if validation fails so
onDetail is only invoked with a valid numeric id; ensure you reference $li,
$li.dataset.id and onDetail when making the change.
src/components/modal/Modal.ts
Outdated
| #update(movie: MovieDetail) { | ||
| const { title, release_date, overview, poster_path, genres, vote_average } = movie; | ||
|
|
||
| $<HTMLImageElement>(this.#$modal, '.modal-image img').src = getOriginalImageUrl(poster_path); | ||
| $(this.#$modal, 'h2').textContent = title; | ||
| $(this.#$modal, '.detail').textContent = overview; |
There was a problem hiding this comment.
poster_path가 null일 경우를 고려해 보세요.
TMDB API에서 일부 영화는 poster_path가 null일 수 있습니다. 이 경우 getOriginalImageUrl(null)이 호출되어 유효하지 않은 이미지 URL이 생성됩니다.
기본 이미지(placeholder)를 표시하는 방법을 고려해 보시겠어요?
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/components/modal/Modal.ts` around lines 50 - 55, In `#update`(movie:
MovieDetail) guard against movie.poster_path being null before calling
getOriginalImageUrl: if poster_path is falsy, set the img src to a
placeholder/default image (or a getPlaceholderImage() helper) instead of calling
getOriginalImageUrl(null); update the assignment that targets
$<HTMLImageElement>(this.#$modal, '.modal-image img').src and ensure any helper
used (getOriginalImageUrl or new getPlaceholderImage) is referenced so reviewers
can locate the change.
src/utils/event.ts
Outdated
| export const ROUTE_CHANGE_EVENT = 'ROUTE_CHANGE'; | ||
| export const CUSTOM_EVENT = { | ||
| ROUTE_CHANGE: 'ROUTE_CHANGE', | ||
| SCROOL_END: 'SCROOL_END', |
There was a problem hiding this comment.
오타 수정: SCROOL_END → SCROLL_END
이벤트 이름에 오타가 있습니다. SCROOL_END는 SCROLL_END로 수정해야 합니다. 이 상수는 여러 파일에서 사용되므로 일괄 수정이 필요합니다.
🔧 제안하는 수정
export const CUSTOM_EVENT = {
ROUTE_CHANGE: 'ROUTE_CHANGE',
- SCROOL_END: 'SCROOL_END',
+ SCROLL_END: 'SCROLL_END',
} as const;📝 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.
| SCROOL_END: 'SCROOL_END', | |
| export const CUSTOM_EVENT = { | |
| ROUTE_CHANGE: 'ROUTE_CHANGE', | |
| SCROLL_END: 'SCROLL_END', | |
| } as const; |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/utils/event.ts` at line 4, There's a typo in the exported event constant
name: rename the constant SCROOL_END to SCROLL_END in the src/utils/event.ts
definition and update every usage/reference throughout the codebase (imports,
switch/case, event emitters/listeners) to use SCROLL_END; ensure any exports and
re-exports (e.g., in enums or objects) and tests are updated and run the build
to catch missed occurrences so no runtime import/usage breaks occur.
| .top-rated-container, | ||
| .search-container { | ||
| display: flex; | ||
| flex-direction: column; | ||
| justify-items: center; | ||
| align-items: center; | ||
| } |
There was a problem hiding this comment.
flex 컨테이너에서 justify-items는 동작하지 않습니다.
display: flex인 컨테이너에서 justify-items는 Grid 전용 속성이라 효과가 없습니다. 의도하신 정렬이 이루어지고 있는지 확인해 보세요.
🔧 수정 제안
.top-rated-container,
.search-container {
display: flex;
flex-direction: column;
- justify-items: center;
+ justify-content: center;
align-items: center;
}📝 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.
| .top-rated-container, | |
| .search-container { | |
| display: flex; | |
| flex-direction: column; | |
| justify-items: center; | |
| align-items: center; | |
| } | |
| .top-rated-container, | |
| .search-container { | |
| display: flex; | |
| flex-direction: column; | |
| justify-content: center; | |
| align-items: center; | |
| } |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@styles/main.css` around lines 312 - 318, The CSS rule for
.top-rated-container and .search-container uses justify-items which has no
effect on a flex container; remove the justify-items declaration and, if
horizontal centering of child items was intended, replace it with
justify-content (e.g., center) while keeping align-items for vertical alignment,
or adjust whichever property (justify-content / align-items / flex-direction)
matches the intended layout; update the rule targeting .top-rated-container and
.search-container accordingly.
styles/modal.css
Outdated
| border-radius: 16px; | ||
| } | ||
| .modal-container { | ||
| padding: 40 24; |
There was a problem hiding this comment.
CSS 값에 단위 누락
padding: 40 24는 유효하지 않은 CSS입니다. 숫자 값에는 단위가 필요합니다.
🔧 제안하는 수정
- padding: 40 24;
+ padding: 40px 24px;📝 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.
| padding: 40 24; | |
| padding: 40px 24px; |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@styles/modal.css` at line 123, The CSS rule in styles/modal.css uses an
invalid padding declaration "padding: 40 24"; update the padding property in
that rule (padding) to include explicit units for both values (e.g., px, rem,
etc.) so it becomes a valid CSS shorthand like "padding: 40px 24px;" using the
same selector where padding is defined.
| .modal { | ||
| top: 20px; | ||
| border-radius: 0; | ||
| bottom: 0; | ||
| width: 800px; | ||
| height: 749px; | ||
| width: 390; | ||
| gap: 24px; | ||
|
|
||
| opacity: 1; | ||
| padding: 40px 24px; | ||
| border-radius: 16px; | ||
| } |
There was a problem hiding this comment.
중복 속성 및 단위 누락 수정 필요
이 블록에 여러 문제가 있습니다:
border-radius: 라인 140에서0, 라인 149에서16px로 중복 선언 (마지막 값만 적용)width: 라인 142에서800px, 라인 144에서390으로 중복 선언width: 390는 단위가 누락되어 유효하지 않은 CSS입니다
의도한 최종 스타일만 남기고 정리해 보세요.
🔧 제안하는 수정 예시
`@media` (max-width: 390px) {
.modal {
top: 20px;
- border-radius: 0;
bottom: 0;
- width: 800px;
height: 749px;
- width: 390;
+ width: 390px;
gap: 24px;
-
opacity: 1;
padding: 40px 24px;
border-radius: 16px;
}🧰 Tools
🪛 Stylelint (17.6.0)
[error] 142-142: Unexpected duplicate "width" (declaration-block-no-duplicate-properties)
(declaration-block-no-duplicate-properties)
[error] 140-140: Unexpected duplicate "border-radius" (declaration-block-no-duplicate-properties)
(declaration-block-no-duplicate-properties)
[error] 147-147: Unexpected empty line before declaration (declaration-empty-line-before)
(declaration-empty-line-before)
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@styles/modal.css` around lines 138 - 150, In the .modal rule consolidate
duplicate/conflicting declarations: remove the earlier border-radius: 0 and any
extra width lines so only one border-radius (keep border-radius: 16px) and one
width remain, and fix the invalid width: 390 by adding a unit (e.g., width:
390px) or replace it with the intended width (e.g., 800px) — ensure
height/top/bottom/padding/gap remain consistent with the chosen width and remove
any redundant duplicate properties.
| justify-content: center; | ||
|
|
||
| width: 100%; | ||
| margin: 0 auto 56px; | ||
| grid-template-columns: repeat(auto-fit, 200px); | ||
| gap: 40px; |
There was a problem hiding this comment.
Stylelint 오류: 불필요한 빈 줄
라인 6에 선언문 앞에 불필요한 빈 줄이 있습니다. Stylelint의 declaration-empty-line-before 규칙 위반입니다.
🔧 제안하는 수정
.thumbnail-list {
display: grid;
justify-content: center;
-
width: 100%;
margin: 0 auto 56px;
grid-template-columns: repeat(auto-fit, 200px);
gap: 40px;
}📝 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.
| justify-content: center; | |
| width: 100%; | |
| margin: 0 auto 56px; | |
| grid-template-columns: repeat(auto-fit, 200px); | |
| gap: 40px; | |
| justify-content: center; | |
| width: 100%; | |
| margin: 0 auto 56px; | |
| grid-template-columns: repeat(auto-fit, 200px); | |
| gap: 40px; |
🧰 Tools
🪛 Stylelint (17.6.0)
[error] 7-7: Unexpected empty line before declaration (declaration-empty-line-before)
(declaration-empty-line-before)
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@styles/thumbnail.css` around lines 5 - 10, Remove the unnecessary blank line
before the first declaration in the CSS rule that contains "justify-content" and
"grid-template-columns" (the rule block with those properties); delete the empty
line so declarations start immediately after the selector, satisfying
Stylelint's declaration-empty-line-before rule (or adjust the rule config if an
exception is required).
DB 변경사항에 따라 의존성 주입을 통해 레포지토리 추가
모달안에서 전역(window) 이벤트를 사용하여 추가
- 실 서버는 비동기기 때문에 전파를 막기위함
사용자에게 보여주는 에러 수정
스켈레톤이 남아있는 문제해결
Repository 레이어 도입 + Promise 인터페이스
어제 작성 중 시지프의 의견으로 API분리에 대해서 생각하게 됐습니다 
처음 생각한 방법으론 
하지만 레스의 의견을 듣고나니 로컬스토리지는 동기지만 비동기 DB로 전환하기 때문에, 동기적인 // localstorage.ts
export type MovieStore = {
get(key: string): Promise<string | null>;
save(key: string, value: string): Promise<void>;
};
export default class LocalStorage {
#myStorage;
constructor() {
this.#myStorage = window.localStorage;
}
async get(key: string): Promise<string | null> {
return this.#myStorage.getItem(key);
}
//... 그래서 로컬 스토리지를 // main.ts(entry point)
const db = new LocalStorage();
const movieRepo = new MovieRepository(db);
const modal = new Modal(movieRepo, $body);DI를 통해 DB 변경에 대응했습니다. 컴포넌트 재사용 중 생긴 고민
위에 표시한 주황색 박스들은 같은 곳에 사용되는 컴포넌트를 묶어봤습니다. <div class="rate">
<별 이미지 컴포넌트>
<span>별점 평균</span>
</div>이런 중복된 형식을 하고 있어 컴포넌트를 재사용하면 되겠다고 생각했습니다. 하나는 이 경우에서 컴포넌트를 재사용하려면 옵션으로 우선 재사용 로직을 빼는게 맞는지, 뺀다면 옵션으로 받아 변경할 수 있게 하는 방법이 맞는지 점검 받고 싶습니다! |
There was a problem hiding this comment.
안녕하세요 콘티!
2단계 미션도 구현하시느라 고생 많으셨습니다. 👍
앱 동작
앱 동작 확인했습니다!
- 영화 목록에서 추가로 불러오는 기능이 무한 스크롤 방식으로 잘 바뀌었네요!
- 반응형 레이아웃이 디바이스(모바일, 태블릿, 데스크탑)에 따라 잘 구현되어있네요~!
- 별점 매기기도 데이터가 잘 저장되고, 불러와지네요!
- ESC로 모달 닫기도 잘 되구요!
사용성 관련 개선포인트:
- 모달이 열릴 때 이미지 로드 전후로 모달 크기가 바뀌네요. 원인이 뭘지, 어떻게 해결할 수 있을지 생각해보면 좋겠습니다!
- 별점 클릭 시 별 5개가 전부 사라졌다가 다시 그려지면서 레이아웃이 흔들리네요..! 전부 다시 그리지 않고 해결할 수 있는 방법이 있을까요?
- 모달을 닫고 다른 영화를 클릭하면, 새 이미지가 로드되기 전까지 이전 영화 이미지가 보여요. 이건 위 두개보단 중요성이 좀 높아보이네요! (네트워크 상황에 따라 꽤 오랜시간 동안 이전 이미지가 보일 수 있어서, 사용자에게 혼란이나 잘못된 정보를 줄 수 있겠어요!)
세 가지 모두 아주 크리티컬한 버그는 아니지만, 해결 난이도가 높지 않으니 시간이 되면 개선해보시면 좋겠습니다!
논의
1. const 와 let
- 우선, 궁금한 점! 우테코에서 const의 사용을 강조하는 이유가 무엇일까요? 콘티는 const 사용이 왜 중요하다고 생각하시나요?
- let을 피하려고 값을 객체로 감싸는 건.. 강력히 말리고 싶습니다. 그건 const의 의도를 우회하는 거예요. let이 필요한 상황이라면 먼저 '재할당 없이 해결할 수 있는 구조가 있는지' 고민해보고, 정말 필요하다면 우회가 아니라 let을 쓰는게 맞을 것 같네요.
2. 이벤트 위임 중 발견한 타입 문제
as, instanceof 두 방식의 차이를 알고 계신 것 같아요! 어떤 기준으로 as를 선택하셨는지 궁금합니다! 그리고 각각 어떤 상황에서 쓰는 게 더 적절한지에 대해 콘티의 생각이 궁금합니다!
3. 이벤트 위임은 옳은가
음음.. 주로 쓰는건 보통 리스트 요소면 부모 요소로 위임하는거? 정도..가 생각나네요!
콘티가 말씀하신 별점 5개처럼 고정된 소수의 요소에는 이벤트 위임이 필수는 아닌 것 같아요. 지금 구조에서는 어느 쪽이든 괜찮다고 봅니다.
4. 스켈레톤 삭제 문제
요 스켈레톤 삭제 문제는 fetch 중복 호출 때문이고, fetch 중복 호출 문제를 해결한 걸로 보이는데요..! 그러면 스켈레톤이 여러 세트 쌓이는 문제 자체가 안 생기는 거 아닌가요? Map까지 필요했나? 하는 생각이 우선 듭니다.
그리고 "한 페이지 묶음의 class를 만들어야 하나"라는 고민은 위 스켈레톤 삭제 문제가 아니더라도 관심사 분리 관점에서 좋은 방향인 것 같습니다. Main이 지금 영화 목록 + 스켈레톤 + 에러 + 빈결과까지 다 담당하고 있는데, 그 부담을 줄여줄 수 있을 것 같아요. 다만 지금 규모에서 꼭 필요하냐? 하면 또 그건 아닐 것 같네요 ㅎㅎ
결론: 다 일리가 있다고 봅니다!
5. Repository + Promise 인터페이스
MovieStore 인터페이스를 만들고 LocalStorage를 async로 감싸서 나중에 서버 API로 교체할 수 있는 구조를 만든 건 잘 하신 것 같습니다! DI 체인도 잘 구성되어 있네요. 다만 MovieRepository 레이어에 대해서는 필요성에 대한 의문이 있습니다. (인라인 코멘트로 따로 남겼습니다!)
6. Rate 컴포넌트 재사용 고민
UI를 보면 히어로 배너와 모달에서 별 이미지 종류, 텍스트 포맷, 크기, 색상까지 꽤 다른 것 같아요. 공통점이 "별 이미지 + 숫자" 정도인데, 이걸 하나의 컴포넌트로 만들려면 옵션이 많아질 수밖에 없을 것 같습니다. 이 정도 차이라면 무리하게 합치기보다 별개의 컴포넌트로 두는 게 더 자연스러울 수 있을 것 같아요!
7. localStorage → 서버 API 교체 시 구조
미션 상황에서 "local storage, 서버 API를 쉽고 안전하게 갈아끼울 수 있는 구조"를 요구하고 있는데요. 콘티는 이 상황에서 무엇을 중점적으로 생각했고, 어떤 구조로 대응하려 했는지 궁금합니다! 그리고 실제로 서버 API로 교체할 때 어떤 파일을 어떻게 수정하면 되는지, 그 수정 범위가 최소한인지도 궁금합니다!
구조 관련 피드백
스크롤/라우트 이벤트 관심사
스크롤 감지는 main.ts에서 하고, 그 이벤트를 받는 곳은 Page 안인데, 발행자와 구독자가 떨어져 있어서 흐름 추적이 어려운 것 같아요. 이 흐름을 개선할 수 있는 방법이 있을지 생각해보면 좋겠습니다! (너무 깊게 고민은 X !)
페이지 캐싱과 파생 문제들
PR 본문에서 페이지 캐싱, Fragment → $div 전환, 리스너 누적 우회, input 값 잔존 처리 등 고민 과정을 상세히 설명해주셨는데요. 이 문제들을 보면 "DOM을 전부 날리고 새로 만드는 구조"에서 파생된 문제들을 하나씩 해결하고 있는 것 같아요. 고민 과정 자체는 좋은데, 이런 문제들이 계속 나온다면 근본 원인이 뭔지 한번 생각해보면 좋겠습니다!
ROUTE_CHANGE 커스텀 이벤트
ROUTE_CHANGE 커스텀 이벤트 없이 Page에서 직접 location.hash를 변경하면 hashchange가 발생해서 router()가 실행될 텐데, 커스텀 이벤트를 거쳐야 하는 이유가 있나요??
Modal을 main.ts에서 주입하는 구조
Modal을 main.ts에서 만들어서 Page에 주입하고 있는데, 다른 컴포넌트(Header, Main, Footer)는 Page 안에서 만들잖아요? Modal도 Page 안에서 만들면 안 되는 이유가 있을까요..?
src/main.ts
Outdated
| const router = (modal: Modal) => { | ||
| const $app = document.querySelector('#app'); | ||
| if (!$app) return; | ||
|
|
||
| $app.innerHTML = ''; | ||
|
|
||
| const fullHash = location.hash.replace('#', '') || '/'; | ||
| const [path, queryString] = fullHash.split('?'); | ||
| const [path] = fullHash.split('?'); | ||
| const match = routes.find((route) => route.path === path); | ||
|
|
||
| const View = match ? match.view : HomePage; | ||
| const page = new View(); | ||
| $app.replaceChildren(page.$element); | ||
| const fullpath = match ? fullHash : '/'; | ||
|
|
||
| const cachedPage = PAGE_CACHE.get(fullpath); | ||
| if (cachedPage !== undefined) { | ||
| $app.append(cachedPage.$element); | ||
| return; | ||
| } | ||
|
|
||
| const newPage = new View(modal); | ||
| $app.append(newPage.$element); | ||
| PAGE_CACHE.set(fullpath, newPage); | ||
| }; | ||
|
|
There was a problem hiding this comment.
이 router 함수가 라우팅 + 캐싱 + 페이지 생성을 다 하고 있는데, 역할을 나눌 수 있을지 생각해보면 좋겠습니다!
src/main.ts
Outdated
| window.addEventListener('scroll', () => { | ||
| const isScrollEnded = window.innerHeight + window.scrollY + 400 >= document.body.offsetHeight; | ||
| if (isScrollEnded) { | ||
| scrollEvent(); | ||
| } | ||
| }); | ||
|
|
||
| addEventListener('load', () => { | ||
| window.addEventListener('hashchange', router); | ||
| router(); | ||
| const $body = document.querySelector('body'); | ||
| if (!$body) return; | ||
|
|
||
| const db = new LocalStorage(); | ||
| const movieRepo = new MovieRepository(db); | ||
| const modal = new Modal(movieRepo, $body); | ||
| $body.append(modal.$element); | ||
|
|
||
| window.addEventListener('hashchange', () => router(modal)); | ||
|
|
||
| router(modal); | ||
| }); |
There was a problem hiding this comment.
55행에서 addEventListener를 window. 없이 호출하고 있는데, 위에서는 window.addEventListener를 쓰고 있는데요. 의도적인 차이일까요?
There was a problem hiding this comment.
아닙니다. 그냥 원본 코드에서 크게 변경하지 말아야겠다는 생각을 했었습니다.
하지만 다른 곳에서 전역이벤트에 window를 표기했다면 해당 이벤트에도 통일성있게 명시를 해줬어야 됐다고 생각합니다.
src/main.ts
Outdated
| const isScrollEnded = window.innerHeight + window.scrollY + 400 >= document.body.offsetHeight; | ||
| if (isScrollEnded) { |
There was a problem hiding this comment.
이 400이라는 숫자가 어떤 의미인지 코드만 봐서는 알기 어려워 보입니다! 상수로 빼는건 어떨까요?
src/main.ts
Outdated
| const cachedPage = PAGE_CACHE.get(fullpath); | ||
| if (cachedPage !== undefined) { |
There was a problem hiding this comment.
검색어마다 새 SearchPage가 캐시에 쌓이게 되는 것 같은데요...! 이 캐시가 무한히 커질 수 있는 거 아닌가요..? 요거 리스크 검토를 해보시면 좋겠습니다..!
src/storage/LocalStorage.ts
Outdated
| export default class LocalStorage { | ||
| #myStorage; |
There was a problem hiding this comment.
MovieStore 타입을 implements로 명시하도록 만들어두면 MovieStore 인터페이스와 어긋나는 순간 컴파일타임에 잡아줄 수 있을 것 같네요!
src/pages/HomePage.ts
Outdated
| #onDetail = async (movie_id: number) => { | ||
| try { | ||
| const movie = await fetchMovieDetails(movie_id); | ||
| this.#$modal.open(movie); | ||
| } catch (e) {} | ||
| }; |
There was a problem hiding this comment.
음 여기 에러 캐치는 좋은데요, 사용자에게 아무런 피드백을 주고있지 않네요! 지금 동작은 어떻게 될까요? 어디서 에러가 발생할 여지가 있을지, 그 경우에 UI/UX는 어떻게 되는지 궁금합니다
src/pages/HomePage.ts
Outdated
There was a problem hiding this comment.
HomePage와 SearchPage가 거의 동일한 코드(페이지네이션, 무한스크롤, onDetail, handleError 등)를 가지고 있는 것 같으네욥..! 이 중복을 해결할 방법이 있을까요?
저도 생각을 좀 해봤는데,
- 이 페이지 클래스들의 역할이 너무 큰게 아닐까 싶기도 하고요...?
- 두 페이지의 실질적인 차이가 뭔지 생각해보면, 어떤 API를 호출하느냐와 헤더 형태 정도인 것 같은데.. 그렇다면 페이지를 분리하지 않고 하나의 페이지에서 처리하는 구조도 가능하지 않았을까요?
중복 코드가 있다고 무조건 합쳐야 하는 건 아니에요! 두 페이지가 앞으로 다른 방향으로 발전할 가능성이 있다면 분리해두는 게 나을 수도 있어요. 지금 시점에서 어떻게 판단하는지, 콘티의 생각이 궁금합니다
There was a problem hiding this comment.
구현 중 홈페이지에서 작업했던걸 서치페이지에 그대로 복붙하는 과정을 겪을 때마다 한 페이지로 합쳐야 하나 고민이 됐었습니다.
하지만 합치지 않았던 이유는 각 페이지가 다른 API를 쓰기 때문에 다른 관심사이지 않을까? 생각했습니다.
완성된 모습을 보니 분리해두는게 굉장히 비효율적으로 보였습니다.
두 페이지가 앞으로 다른 방향으로 발전할 가능성이 있다면 분리해두는 게 나을 수도 있어요.
다른 방향으로 발전할 시, 그때 분리하면 되겠다고 생각했습니다. 현재 주어진 문제에서는 합치는 것이 효율적이라고 느끼기도 했구요. 하나의 페이지로 합치며 어떤 것이 잘못되었었는지 몸으로 체득해보겠습니다.
| it('사용자는 모달을 열고 평점을 6점준다. 별점은 로컬 스토리지에 저장된다', () => { | ||
| cy.visit('http://localhost:5173/'); |
There was a problem hiding this comment.
localStorage를 직접 확인하는 것보다, 새로고침 후 별점이 유지되는지를 사용자 관점에서 확인하는 시나리오가 더 E2E 테스트의 역할에 맞지 않을까요?
There was a problem hiding this comment.
e2e에 대한 이해도가 부족했었네요. 사용자 관점에서 처음부터 끝까지란걸 놓쳤습니다.
해당 부분 수정하겠습니다!
cypress/e2e/spec.cy.ts
Outdated
| }); | ||
| }); | ||
|
|
||
| context('검색 기능', () => { |
There was a problem hiding this comment.
이 context 이름이 아래 검색 기능과 중복되네요! 적절한 이름으로 바꾸면 좋을 것 같습니다!
| beforeEach(() => { | ||
| cy.intercept('GET', '**/movie/popular?*page=1*', { | ||
| statusCode: 200, | ||
| body: MOCK_PAGE_1, | ||
| }).as('getPopularMovies'); | ||
| }).as('getPopularMoviePage1'); | ||
|
|
||
| cy.intercept('GET', '**/movie/popular?*page=2*', { | ||
| statusCode: 200, | ||
| body: MOCK_PAGE_2, | ||
| }).as('getNextPopularMovies'); | ||
| }).as('getPopularMoviePage2'); | ||
| }); |
There was a problem hiding this comment.
fetchMovieDetails API는 mock하지 않고 있는데, 실제 API를 호출하는 건 의도일까요?
There was a problem hiding this comment.
아뇨 전혀 의도가 아니었습니다.
테스트 짜는 과정 중 누락되었습니다.. 해당 부분 수정하겠습니다
const 와 let
제가 생각하는
const 는 해당 로직에서 절대 변경되지 않아야 하는 값으로 이해했습니다. 그래야 코드를 읽는 사람이 const 값은 끝까지 유지되겠구나 생각할 수 있습니다. 아직 변경될 가능성이 보이는 부분을 const나 클래스의 멤버로 분리하지 못했다면, let 으로 표기하여 위험성을 드러내야 하지않나? 라고 생각했었습니다 (unknown처럼 나중에 변경하겠다를 드러내는 의미로 생각했습니다) 타입 단언
타입 단언의 단점이라 하면 런타임에서 잡아낼 수 없기 때문에 사용을 자제하란 말을 들었었는데요, 다른 곳이였으면 타입을 확실하게 좁히는게 맞다곤 생각이 들었지만, 스켈레톤 삭제 문제
맞습니다 해당 부분은 34f4bec 의 문제였습니다. Map으로 해도 근본적인 원인을 제거하지 않으면 다른 버그가 생기는 걸 경험했습니다. 서버 API로 교체할 때 구조
이제서야 제가 크게 오해한 부분이 보였습니다 과한 추상화// 처음 생각한 구조
class LocalStorage {
#myStorage;
constructor() {
this.#myStorage = window.localStorage;
}
}
const localStorage = new LocalStorage();
export default localStorage;
// main.ts
const movieRepo = new MovieRepository(LocalStorage);
//const movieRepo = new MovieRepository(OtherDB);우선 저는 클라이언트임을 망각하고 Repository 에서 DB를 받아 sql문으로 조작하도록 변경하게 될 것이라고 오해했습니다. 나중에 그 사실이 머릿속에 지워졌고 저런 결과가 나왔습니다. 너무 부끄럽네요
대응 방식저는 변경에 유연한 구조를 그래서 의존성주입과 비동기를 추가 하였습니다.
수정이 일어나는 곳지금 파일에서 이런 구조로 변경했을 시, 의존성 주입을 해주는 export type MovieStore = { // interface로 분리후
get(key: string): Promise<string | null>;
save(key: string, value: string): Promise<void>;
};
export default class ServerAPI {
async save(key: string, value: string): Promise<void> {
fetch()
}
//...
}MovieStore의 계약을 가진 파일이 추가되고 // main.ts
import ServerAPI from './storage/ServerAPI.ts'; // 변경
//...
const movieRepo = ServerAPI; // 변경
구조 관련 피드백
스크롤 이벤트는 하나만 존재해야한다고 생각하여 전역으로 추가해놨지만, 이렇게 되면 수야의 말씀대로 흐름 추적이 복잡해지더라구요 해당 부분은 스크롤 이벤트는 전역으로 동작하니,
이를 해결하려면 상태가 변경되는 컴포넌트에 대해서만 변경이 이루어져야 한다고 생각했습니다.
없습니다.. 라우터에 대한 이해가 부족했습니다. 말씀대로 hashchange가 발생하기 때문에, 불필요한 이벤트로 보입니다.
제가 main.ts에 만든 이유가 세 가지였습니다.
// styles 의 css
/* modal.css */
body.modal-open {
overflow: hidden;
}
돌아보니 마크업을 바꾸면 #app 안에 있어도 지금과 동일하게 보일 수 있으며, 페이지 내에서 모달을 만들도록 변경하겠습니다. |
- appendMovies가 생성의 책임만 갖도록 함 - 리스트의 스켈레톤을 맵으로 저장하는 구조 삭제
- 라우터 분리 및 돔 캐시 제거
- class toggle 방식으로 변경
- 타입 변환도 로컬 스토리지 구현체에서 진행
로컬 스토리지 수정
제가 잘못이해한 점은 그래서 사용하는 모달과 구현체 둘 다 모두 인터페이스에 의존하게 만들어 제어권을 가져오도록 만들었습니다. // main.ts
const movieDB = new LocalStorage();
window.addEventListener('hashchange', () => router(movieDB));
router(movieDB);// createPage.ts
export const createHomePage = (movieDB: MovieStore): MoviePage => {
return new MoviePage({
type: 'home',
fetchMovie: (page: number) => fetchPopularMovies(page),
fetchDetail: (movie_id: number) => fetchMovieDetails(movie_id),
onSubmit,
movieDB, // DB를 넘겨줌
});
};// MoviePage.ts
this.#$modal = new Modal(option.movieDB, this.#$div);의존성 주입을 메인부터 모달까지 의존성 주입하도록 변경했습니다. // LocalStorage.ts
async save(key: number, value: number): Promise<void> {
this.#myStorage.setItem(String(key), String(value));
}로컬 스토리지의 API클래스에서 타입변환을 하도록 변경했습니다. PR을 쓰며 어떤 부분이 변경 되는지 따라가보다가 마지막에 수정하였습니다. 04-14(화) 추가 내용: 의존성 주입의 위치export const createHomePage = (movieDB: MovieStore): MoviePage => {
return new MoviePage({
type: 'home',
fetchMovie: (page: number) => fetchPopularMovies(page),
fetchDetail: (movie_id: number) => fetchMovieDetails(movie_id),
onSubmit,
movieDB, // 여기로 변경예정 movieDB: new LocalStorage();
});
};다시 보니 엔트리 포인트부터 의존성을 가져갈 필요는 없고, 페이지를 만드는 곳부터 로컬 스토리지를 가져가는 구조가 맞다는 생각이 들었습니다. 페이지 ,헤더 통일import MoviePage from './MoviePage.ts';
import { fetchMovieDetails, fetchPopularMovies, fetchSearchMovies } from '../api/fetchApi.ts';
import { MovieStore } from '../storage/types.ts';
const onSubmit = (query: string): void => {
if (query.trim()) {
location.hash = `/search?query=${encodeURIComponent(query)}`;
}
};
export const createHomePage = (movieDB: MovieStore): MoviePage => {
return new MoviePage({
type: 'home',
fetchMovie: (page: number) => fetchPopularMovies(page),
fetchDetail: (movie_id: number) => fetchMovieDetails(movie_id),
onSubmit,
movieDB,
});
};
export const createSearchPage = (query: string, movieDB: MovieStore): MoviePage => {
return new MoviePage({
type: 'search',
fetchMovie: (page: number) => fetchSearchMovies(query, page),
fetchDetail: (movie_id: number) => fetchMovieDetails(movie_id),
onSubmit,
query,
movieDB,
});
};캐시 로직을 없애고 페이지와 헤더를 통일하였습니다 . 페이지의 생성은 내부에서 query의 유무로 분리하기 보다는 생성 때 부터 분리하여 변경에 대응하였습니다. 모달 생성 위치 변경//MoviePage.ts
this.#$modal = new Modal(option.movieDB, this.#$div);추가로 모달도 무비페이지 내에서 생성하도록 변경했습니다. IntersectionObserver 도입스크롤의 전역이벤트를 삭제하고 응집도를 늘리기 위해 실제로 이벤트가 일어나는 파일에 옵저버를 뒀습니다. // MovieItem.ts
renderMovies(movies: MovieData[]) {
this.removeSkeletons();
const $fragment = new DocumentFragment();
movies.forEach((movie) => $fragment.append(MovieItem(movie)));
this.#$list.append($fragment);
return this.#$list.lastElementChild;
}
// MoviePage.ts
const lastElement = this.#appendMovies(response);
if (lastElement) this.#observer.observe(lastElement);반환 값으로 마지막 요소를 반환하여 옵저버가 센티넬에 등록하기 편한 구조로 변경하였습니다. 근본적인 문제란?// Header.ts
showBanner(data: MovieData) {
this.#$background.style.backgroundImage = `url(${getOriginalImageUrl(data.backdrop_path)})`;
this.#$background.classList.add('top-header-container');
if (this.#$topRate) this.#$topRate.remove();
this.#$topRate = TopRate(data, this.#onDetail);
this.#$background.append(this.#$topRate);
this.#$overlay.classList.remove('hidden');
this.#$banner.classList.remove('hidden');
}그래도 저의 근본적인 문제, 바디에서 리스트 전체를 렌더링 하는 문제는 해결하지 못하였습니다. 하지만 리스트의 경우 전부 리렌더링 되는 것을 막을 수 없었기에 아직 근본적인 문제에 닿지 못했습니다. 모달 이미지 출력 문제 해결// MoviePage.ts
async #onDetail(movie_id: number) {
try {
this.#$modal.renderSkeleton(); // 스켈레톤 도입
const movie = await this.#option.fetchDetail(movie_id);
this.#$modal.open(movie);
} catch (error) {
// Modal.ts
renderSkeleton() {
this.#$modalImg.src = './images/empty.png';
}임시방편으로 모달에 임시 이미지를 넣어 잘못된 이미지 출력을 나타내도록 했습니다. 테스트 context('모달', () => {
cy.get('ul li:first-child').click();
cy.wait('@getMovieDetail');
cy.get('.modal').should('be.visible');
// ...
cy.get('.star-container .submit-star-button').each(($el, index) => {
const img = $el.find('img');
if (index <= 2) {
cy.wrap(img).should('have.attr', 'alt', 'star_filled');
} else {
cy.wrap(img).should('have.attr', 'alt', 'star_empty');
}
});여전히 테스트는 저에겐 큰 문제였습니다. 질문
이 근본적인 원인이 무엇인지만 고민했었는데 돔 요소가 리렌더링 되는 것도, 매번 전체 요소를 렌더링 하는 것도, 전부 바뀌기 전의 상태를 추적하지 않아서 생긴 문제라고 생각했습니다. 하지만 의문이 들었던 것은 리스트의 경우, 매번 이번 미션에서 리뷰어님께 많은 실례를 했습니다. 그럼에도 많은 리뷰를 달아주셔서 학습에 너무 큰 도움이 됐습니다. |
2 participants
🎯 미션 소개
🕵️ 셀프 리뷰(Self-Review)
제출 전 체크 리스트
리뷰 요청 & 논의하고 싶은 내용
안녕하세요 수야!
step2에서는 제출하고 피드백 받은 것을 빠르게 적용해보려고 완벽하지 않은 PR을 올리게 되었습니다!
키보드 입력으로 종료와테스트, 마크업이 작성되지 않았습니다,해당 부분에선 리뷰 이후 완성시키겠습니다!1) 이번 단계에서 가장 많이 고민했던 문제와 해결 과정에서 배운 점
이번 단계에서 고민했던 문제를 크게 나누면 아래의 세 가지가 있습니다.
여기서 가장 고민한 문제는
페이지 캐싱입니다.페이지 캐싱
V-DOM이 없어 매번 전부 렌더링되는 문제를 고민해봤습니다.처음엔 저장할 수 있는 기능을 가진 클래스를 만들어
Map<page, MovieData[]>의 구조로 패치받아온 데이터를 넣어 놓으려고 했습니다.하지만 이는 근본적인 문제(렌더링)가 해결되지 않을 것이라 생각하여 생성된 돔 요소 자체를 캐싱하는 전략으로 선택했습니다.
문제 1 . fragment라서 돔 요소가 붙기 전까지 가질 수 없는 문제
저는 홈페이지와 서치페이지에
DocumentFragment를 사용하여$app에 추가하는 로직을 구성해놨었습니다.하지만
Fragment는 돔 생성이 아니라 메모리 단계에서 조립해놓고 해당 부분에 붙이는 과정을 가지고 있어$app에 붙이고$app부터 하위요소를 모두 가지려고 했습니다. 하지만 리렌더링 과정에서$app.innerHTML=""을 하기 때문에 다른 곳에 복사해서 가지려고 해도 자식의 이벤트 리스너를 가지고 갈 수 없는 문제가 생겼습니다.그래서 페이지의 반환형을
$div로 바꿔해결했습니다문제 2. 윈도우 이벤트 리스너 누적 문제
이 시점은 마지막 단계였고 위와 같이 커스텀 이벤트로 무한 스크롤이 완성 되어있었던 상태였습니다.
하지만 검색으로 이동한 이후에도 홈페이지의
fetch를 요청하고 있었습니다. 원인은$app.innerHTML=""을 한다고, 윈도우 이벤트 리스너가 사라지지 않기 때문에 계속해서 누적되는 문제였습니다.처음엔 페이지마다 멤벼 변수로
현재 보고 있는 페이지인지 검사하는 로직을 달려했으나 그럼 최상위 모듈main.ts에서 상태를 가져야 하기 때문에 꺼려졌습니다.그래서 각 페이지마다 래핑하는 div 객체에 이름을 줘서 해당 페이지인지 검사할 수 있도록 조건문을 추가하여 해결했습니다.
문제 3. input입력 값이 남아 있는 문제
캐싱에 성공했지만, 돔 요소를 전부 저장하기 때문에 서치 페이지에서 검색이 끝난 이후
input에 입력 값이 남아있는 문제를 확인했습니다.멤버로 검색 값을 들고 있으면 될까?생각했지만 페이지를 캐싱했기 때문에 페이지가 화면에 불려오는 시점을 알려면 라우팅 시점에서 확인하는 방법밖에 떠오르지 않아 엔트리 포인트에서View를 조작하는게 걱정되어 작업을 중단했습니다.현재는 제출 후, 입력 값을 비워주는 방법으로 처리한 상태입니다.
2) 이번 리뷰를 통해 논의하고 싶은 부분
const 와 let
우테코에서
const의 사용을 강조하지만 내부 멤버가 변할 수 있는 배열이나 객체의 경우에도const를 사용하는 게 옳은 지 궁금합니다.제가 상수 사용의 중요성은 알지만 가끔 제 실력의 부족으로 인해
let을 사용해야하는 상황이 올 때가 있었습니다.그럴 때
const객체를 사용하여 해결할 수 있었지만 변할 수 있는 부분을const로 두는 게 의문이 들었습니다.변할 수 있는 곳을 발견했을 때에도
const객체의 멤버로 넘기는 것이 좋을까요?이벤트 위임 중 발견한 타입 문제
이번에 리펙토링을 진행하다 크루에게 이벤트 위임의 인사이트를 얻어 이벤트 위임을 사용하였습니다.
구현 중
as Element를 사용하게 되었고 이를 해결하려면type체크가 두 라인 들어가야 됐습니다.이런 경우에는
as를 사용하시나요? 아니면instance of로 타입을 좁히시나요 ?이벤트 위임은 옳은가
리스트에서 이벤트 위임을 사용해보니 로직을 구현하는 곳의
depth가 한 단계 줄었습니다.하지만 별점을 클릭했을 때에도 이벤트 위임을 걸어야 하는 지, 5개밖에 안되니 하나 씩 이벤트를 추가하는게 옳은 지 고민이 들었습니다.
보통 이벤트 위임을 사용하는 경우는 동적으로 생기는 다수의 데이터에 경우 사용하는 것으로 알았기 때문에 이벤트 위임을 걸진 않았지만 깊이를 줄이기 위해 사용하는게 옳지 않았을까? 생각도 들었습니다.
리뷰어님의 경우 이벤트 위임은 어떤 경우에서 주로 사용하시나요?!
스켈레톤 삭제 문제
무한 스크롤을 적용하던 과정 중
fetch가 여러번 실행되어 스켈레톤 요소가 사라지기 전에 추가되는 문제를 확인했습니다.그래서 멤버로 스켈레톤의 맵을 가지고, 지우는 로직을 추가하였는데, 생각해보니 기능이 커졌으니 한 페이지 묶음의 class 를 만들고 멤버로서 스켈레톤과 무비 리스트를 관리해야하나 생각이 들었습니다.
이에 관한 리뷰어님의 의견이 궁금합니다!
✅ 리뷰어 체크 포인트