[2단계 - 상세 정보 & UI/UX 개선하기] 도넛 미션 제출합니다#309
Open
geongyu09 wants to merge 33 commits intowoowacourse:geongyu09from
Open
[2단계 - 상세 정보 & UI/UX 개선하기] 도넛 미션 제출합니다#309geongyu09 wants to merge 33 commits intowoowacourse:geongyu09from
geongyu09 wants to merge 33 commits intowoowacourse:geongyu09from
Conversation
DTO 계층을 추가하여 API 응답 데이터를 애플리케이션에서 사용할 수 있는 형태로 변환하는 역할을 담당하도록 했습니다. 이를 통해서 api 응답 타입이 변경되거나 다른 서비스를 사용하게 되는 경우 내부 로직이 영향을 받지 않습니다.
- 콜백 방식을 택했으므로, 에러 처리를 외부로 넘기는 것이 아닌 콜백으로 처리하도록 수정
Walkthrough이 변경 사항은 영화 목록 애플리케이션의 주요 아키텍처 리팩토링과 기능 확장을 포함합니다. 무한 스크롤 기능으로 페이지네이션을 재구현하고, 영화 상세 조회 모달과 별점 평가 기능을 추가했습니다. API 응답을 DTO 패턴으로 변환하고, 상태 관리를 새로운 store 기반 시스템으로 전환하며, UI 렌더링 로직을 계층화된 컴포넌트 구조로 재조직했습니다. 템플릿 기반 HTML 파일을 제거하고 src 폴더의 index.html로 통합하였으며, 반응형 스타일을 추가했습니다. 테스트 코드를 무한 스크롤, 모달, 별점 기능에 맞게 업데이트했습니다. Estimated code review effort🎯 5 (Critical) | ⏱️ ~100 minutes 🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 7
🧹 Nitpick comments (19)
index.html (1)
55-55: 주석 처리된 모달 마크업은 정리하는 편이 안전합니다.Line [55] 같은 dead markup은 실제 렌더링 소스의 책임(HTML vs JS)을 헷갈리게 만들 수 있습니다. 사용하지 않는다면 제거를 권장합니다.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@index.html` at line 55, Remove the dead commented modal markup so unused HTML doesn't confuse ownership of rendering; specifically delete the commented line containing the div with id="modalBackground" and class="modal-background active" from index.html (remove the <!-- <div class="modal-background active" id="modalBackground"></div> --> comment) to keep the markup clean.src/ui/utils/eventListener.ts (1)
15-20:replace유틸의 element 타입 범위를add와 맞춰보는 걸 추천드립니다.Line [2]는
HTMLElement | Element | null인데 Line [16]은HTMLElement | null로 좁아져 있습니다.
두 함수의 지원 대상을 동일하게 가져갈지 먼저 기준을 정하면 호출부 타입 일관성이 좋아집니다.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/ui/utils/eventListener.ts` around lines 15 - 20, The ReplaceEventListenerToElementArg interface narrows element to HTMLElement | null while the add util accepts HTMLElement | Element | null; update ReplaceEventListenerToElementArg.element to Element | null to match the add function's supported targets (or vice versa if you decide the common contract should be HTMLElement), then adjust any call sites of replace or add to conform to the unified type; locate the interface declaration (ReplaceEventListenerToElementArg) and the replace/add functions to make the change consistently across the module.src/styles/thumbnail.css (1)
11-12: 반응형 구간에서 썸네일 비율 왜곡 가능성을 한 번 점검해 주세요.Line [11]은 가변 폭인데 Line [12]는 고정 높이라, 카드 폭이 커질 때 이미지가 찌그러질 수 있습니다.
aspect-ratio/object-fit기준을 정해두면 안정적입니다.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/styles/thumbnail.css` around lines 11 - 12, The thumbnail CSS currently sets width: 100% and a fixed height: 300px which can distort images at different card widths; update the thumbnail rule that contains the width/height declarations to use a responsive sizing strategy—remove the fixed height or replace it with an aspect-ratio (e.g., aspect-ratio: 16/9) and ensure the image inside uses object-fit: cover/contain (or set the thumbnail element to height: auto and let the img control aspect via object-fit) so the image scales without distortion; adjust the selector in thumbnail.css that holds the width/height properties and add the aspect-ratio/object-fit properties to the related img or container rule.src/storage/domain/myRating.ts (2)
7-7: 스토리지 키 생성 로직 중복
RATING_STORAGE_KEY생성 로직이 두 함수에서 중복됩니다. 키 패턴이 변경될 경우 두 곳을 모두 수정해야 합니다.💡 키 생성 함수 분리 예시
+const getRatingStorageKey = (movieId: string) => `rating_${movieId}`; + export const getMyRating = (movieId: string): number => { - const RATING_STORAGE_KEY = `rating_${movieId}`; - const savedRate = getItemFromLocalStorage(RATING_STORAGE_KEY); + const savedRate = getItemFromLocalStorage(getRatingStorageKey(movieId)); // ... }; export const setMyRating = (movieId: string, rating: number) => { - const RATING_STORAGE_KEY = `rating_${movieId}`; - setItemInLocalStorage(RATING_STORAGE_KEY, `${rating}`); + setItemInLocalStorage(getRatingStorageKey(movieId), `${rating}`); };Also applies to: 17-17
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/storage/domain/myRating.ts` at line 7, Extract the duplicated storage key creation into a single helper function (e.g., buildRatingStorageKey or getRatingKey) and replace both occurrences of the inline constant RATING_STORAGE_KEY = `rating_${movieId}` with calls to that function; update functions that reference RATING_STORAGE_KEY to call buildRatingStorageKey(movieId) so the key format is centralized and can be changed in one place (ensure the helper accepts movieId and returns the same `rating_${movieId}` string).
12-12:parseInt에 radix 파라미터 명시 권장
parseInt(savedRate)에 radix(기수) 파라미터를 명시하면 예상치 못한 파싱 결과를 방지할 수 있습니다.- const rate = parseInt(savedRate); + const rate = parseInt(savedRate, 10);🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/storage/domain/myRating.ts` at line 12, The parsing of savedRate uses parseInt without a radix which can yield unexpected results; update the usage of parseInt(savedRate) in myRating.ts to specify the radix (e.g., parseInt(savedRate, 10)) or replace it with Number(savedRate) depending on desired behavior, ensuring the variable rate is parsed reliably.src/lib/render.ts (2)
1-7: 콜백 타입이 비동기 함수와 맞지 않을 수 있습니다.
callback타입이() => void로 정의되어 있지만,src/page.ts의loadInitialMovie는async function입니다. 비동기 함수가 전달될 경우 반환된 Promise가 무시되어 에러 핸들링이 누락될 수 있습니다.이 부분이 의도된 것인지, 아니면 비동기 콜백을 지원해야 하는지 고민해보시면 좋겠습니다.
💡 비동기 콜백 지원 예시
-const render = (callback: () => void) => { +const render = (callback: () => void | Promise<void>) => { addEventListener("load", () => { const app = document.querySelector("#app"); if (!app) return; - callback(); + Promise.resolve(callback()).catch(console.error); }); };🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/lib/render.ts` around lines 1 - 7, The render helper declares callback as () => void but is invoked with an async function (e.g., loadInitialMovie), so change the callback signature to return a Promise (e.g., callback: () => Promise<void>), and update the load event handler to await the callback and wrap the await in try/catch to surface/rethrow/log any errors; reference the render function and its addEventListener load handler and the async consumer loadInitialMovie to locate and update the implementation.
2-2:window.addEventListener명시적 사용 권장
addEventListener가 전역 스코프에서 동작하지만,window.addEventListener로 명시하면 코드의 의도가 더 명확해집니다.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/lib/render.ts` at line 2, Change the global call to addEventListener to explicitly use window.addEventListener so the intent is clear; locate the top-level usage of addEventListener in src/lib/render.ts and replace it with window.addEventListener(...) (preserving the same event name, callback, and options) to make the target object explicit.src/ui/utils/intersectionObserver.ts (2)
1-4:Partial<IntersectionObserverEntry>타입 사용에 대한 질문콜백 파라미터로
Partial<IntersectionObserverEntry>를 사용하고 있는데, 실제로 전달되는entry는 완전한IntersectionObserverEntry객체입니다.
Partial을 사용한 특별한 이유가 있나요? 호출자 측에서 모든 속성이 optional로 취급되어 불필요한 null 체크가 필요할 수 있습니다.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/ui/utils/intersectionObserver.ts` around lines 1 - 4, The callback in reserveIntersectionHandler currently types its parameter as Partial<IntersectionObserverEntry>, but the observer always supplies a full IntersectionObserverEntry; change the callback signature to use IntersectionObserverEntry (not Partial) so callers don't need unnecessary optional checks, update any related type annotations/handlers that reference reserveIntersectionHandler and its callback, and run/typecheck to fix any call sites that assumed optional fields.
11-11:threshold값 하드코딩 고려사항현재
threshold: 0이 하드코딩되어 있습니다. 현재 사용 케이스에서는 문제없지만, 향후 다른 threshold가 필요한 경우 확장성을 고려해볼 수 있습니다.필요시 옵션 파라미터로 분리하는 방안을 검토해보세요.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/ui/utils/intersectionObserver.ts` at line 11, The hardcoded { threshold: 0 } in src/ui/utils/intersectionObserver.ts should be made configurable: modify the function that constructs the IntersectionObserver (the function in this file that calls new IntersectionObserver(..., { threshold: 0 })) to accept an options parameter (either a numeric threshold or an IntersectionObserverInit object) with a default of { threshold: 0 }, use that options value when creating the observer, and update all callers to pass a custom threshold when needed; ensure TypeScript types are updated so callers receive the new parameter and existing behavior remains unchanged when the parameter is omitted.src/state/business/movieState.ts (1)
4-11: 구조 분해 할당으로 가독성 개선 가능현재 인덱스 접근(
[0],[1])이 동작하지만, 구조 분해 할당을 사용하면 의도가 더 명확해질 수 있습니다.💡 구조 분해 할당 예시
-const getNextPageNum = nextPageNum[0]; -const setNextPageNum = nextPageNum[1]; +const [getNextPageNum, setNextPageNum] = nextPageNum; -const getNextSearchPageNum = nextSearchPageNum[0]; -const setNextSearchPageNum = nextSearchPageNum[1]; +const [getNextSearchPageNum, setNextSearchPageNum] = nextSearchPageNum; -const getRequestMovieCount = requestMovieCount[0]; -const setRequestMovieCount = requestMovieCount[1]; +const [getRequestMovieCount, setRequestMovieCount] = requestMovieCount;🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/state/business/movieState.ts` around lines 4 - 11, Replace the array-index assignments that pull getters/setters from nextPageNum, nextSearchPageNum, and requestMovieCount with array destructuring to improve clarity; e.g. destructure nextPageNum into [getNextPageNum, setNextPageNum], nextSearchPageNum into [getNextSearchPageNum, setNextSearchPageNum], and requestMovieCount into [getRequestMovieCount, setRequestMovieCount] so the intent of the variables (getters and setters) is explicit in movieState.ts.src/main.ts (1)
1-1: import 경로에서.ts확장자 제거 고려
"./lib/render.ts"에서.ts확장자를 명시하고 있습니다. 일반적인 TypeScript 프로젝트에서는 확장자를 생략하는 것이 관례입니다 (Line 2의"./page"처럼).빌드 설정에 따라 동작할 수 있지만, 일관성을 위해 확장자 생략을 고려해보세요.
-import render from "./lib/render.ts"; +import render from "./lib/render";🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/main.ts` at line 1, Remove the explicit ".ts" extension from the import in src/main.ts by changing import render from "./lib/render.ts" to import render from "./lib/render" so it matches the project's import style (like "./page") and maintains consistency across modules; update any similar imports in the same file if present.src/storage/utils/localstorage.ts (1)
6-8:value: any타입 개선 고려
any타입 대신 제네릭이나 더 구체적인 타입을 사용하면 타입 안전성이 향상됩니다.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/storage/utils/localstorage.ts` around lines 6 - 8, Replace the loose any with a generic type parameter to improve type safety: change setItemInLocalStorage signature to something like function setItemInLocalStorage<T>(key: string, value: T): void and use JSON.stringify(value) as before; update any call sites if needed to pass properly typed values so TypeScript infers T for the function.src/ui/business/scrollObserver.ts (1)
10-15: 불필요한?? undefined제거 고려
observeLoadMoreScroll이 이미 요소를 찾지 못하면undefined를 반환하므로,?? undefined는 불필요합니다.♻️ 간소화 제안
export const replaceLoadMoreScrollObserver = ( callback: () => Promise<void>, ) => { currentScrollDisconnect?.(); - currentScrollDisconnect = observeLoadMoreScroll(callback) ?? undefined; + currentScrollDisconnect = observeLoadMoreScroll(callback); };🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/ui/business/scrollObserver.ts` around lines 10 - 15, The assignment in replaceLoadMoreScrollObserver uses an unnecessary nullish coalescing to undefined; remove the "?? undefined" and assign the result of observeLoadMoreScroll(callback) directly to currentScrollDisconnect after calling currentScrollDisconnect?.(); update the replaceLoadMoreScrollObserver function so it simply calls currentScrollDisconnect?.(); then sets currentScrollDisconnect = observeLoadMoreScroll(callback); keeping the same behavior but without the redundant coalescing.src/ui/domain/observer.ts (2)
8-17:observeHeaderScroll의 disconnect 반환 누락
reserveIntersectionHandler가 disconnect 함수를 반환하지만,observeHeaderScroll에서는 이를 반환하지 않습니다. 헤더 옵저버 정리가 필요 없는 설계라면 문제없지만, 일관성을 위해 반환값을 제공하는 것도 고려해볼 수 있습니다.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/ui/domain/observer.ts` around lines 8 - 17, observeHeaderScroll currently calls reserveIntersectionHandler but does not return its disconnect function; update observeHeaderScroll to return the disconnect function (or a no-op) so callers can clean up the observer. Specifically, when topRatedMovie and backgroundContainer exist, capture the return value of reserveIntersectionHandler and return its disconnect (or the returned object) from observeHeaderScroll; if the elements are missing, return a no-op disconnect function to keep the API consistent. Ensure you reference observeHeaderScroll, reserveIntersectionHandler, topRatedMovie, and backgroundContainer when making the change.
19-40: 무한 스크롤 로직 잘 구현되어 있습니다.
initialized플래그로 초기 트리거를 건너뛰고,isLoading플래그로 중복 호출을 방지하는 패턴이 적절합니다.몇 가지 개선점을 고려해보세요:
Line 33의 중복 검사:
loadMoreInView는 이미 line 21에서 확인되었고 변하지 않으므로 제거할 수 있습니다.에러 처리:
callback()이 reject되면isLoading이false로 리셋되지 않을 수 있습니다.♻️ 개선 제안
return reserveIntersectionHandler( loadMoreInView, async ({ isIntersecting }) => { if (!initialized) { initialized = true; return; } - if (loadMoreInView && isIntersecting && !isLoading) { + if (isIntersecting && !isLoading) { isLoading = true; - await callback(); - isLoading = false; + try { + await callback(); + } finally { + isLoading = false; + } } }, );🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/ui/domain/observer.ts` around lines 19 - 40, The observeLoadMoreScroll function has a redundant loadMoreInView check inside the intersection handler and no guaranteed reset of isLoading if callback() rejects; remove the inner loadMoreInView guard (it’s already null-checked via getLoadMoreInViewElement) and wrap the await callback() in a try/finally so isLoading is always set back to false, optionally logging or rethrowing errors from the callback; references: observeLoadMoreScroll, getLoadMoreInViewElement, reserveIntersectionHandler, isLoading, initialized, and callback.src/ui/utils/inner.ts (1)
17-27:filterHTML가 기존 이벤트 리스너를 제거할 수 있습니다.
outerHTML을 통해 innerHTML을 재구성하면 남아있는 자식 요소들의 이벤트 리스너가 모두 제거됩니다. 현재clearSkeleton에서 스켈레톤 요소 제거용으로만 사용되고 있어 문제가 없을 수 있지만, 이 함수의 부작용을 인지하고 계신지 확인이 필요합니다.대안으로
removeChild또는remove()를 사용해 특정 요소만 제거하는 방식을 고려해보세요.♻️ 이벤트 리스너를 보존하는 대안
export const filterHTML = (parent: Element, className: string) => { - parent.innerHTML = [...parent.children] - .filter((child) => { - if (child instanceof HTMLElement) { - return !child.classList.contains(className); - } - return true; - }) - .map((child) => child.outerHTML) - .join(""); + [...parent.children].forEach((child) => { + if (child instanceof HTMLElement && child.classList.contains(className)) { + child.remove(); + } + }); };🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/ui/utils/inner.ts` around lines 17 - 27, filterHTML rebuilds parent.innerHTML using outerHTML which strips all event listeners; instead, update filterHTML to remove only matching child elements (preserving listeners) by iterating over parent.children (e.g., Array.from(parent.children)) and calling Element.remove() or parent.removeChild(child) for children whose classList contains the supplied className; keep the function name filterHTML and its signature so clearSkeleton continues to work but change its implementation to perform targeted removals rather than reconstructing innerHTML.src/apis/utils.ts (1)
8-19: 반환 타입 명시를 고려해보세요.
fetcher함수의 반환 타입이 암묵적으로Promise<T | undefined>입니다.catch블록에서undefined가 반환되는데, 이를 명시적으로 선언하면 호출부에서의 혼란을 줄일 수 있습니다.이 패턴이 의도적인지 확인해보세요: 에러 발생 시
onError콜백에만 의존하고 반환값을 사용하지 않는 설계인지, 아니면 호출부에서 반환값을 활용해야 하는지 고민해보시면 좋겠습니다.또한,
onLoading && onLoading()패턴 대신 optional chaining(onLoading?.())을 사용하면 더 간결해집니다.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/apis/utils.ts` around lines 8 - 19, The fetcher function currently returns an implicit Promise<T | undefined> (because the catch path returns nothing) and uses verbose callbacks; update the fetcher signature to explicitly state the intended return type and make the callbacks use optional chaining: either (A) if you want callers to receive no value on error, change the signature to export async function fetcher<T>(arg: FetcherArgs<T>): Promise<T | undefined> and replace onLoading && onLoading() / onSuccess && onSuccess(response) / onError && onError(...) with onLoading?.() / onSuccess?.(response) / onError?.(error instanceof Error ? error : new Error(String(error))); or (B) if callers should always get a value or an error, change the signature to Promise<T> and re-throw the caught error in the catch block after invoking onError?.(...), so the function either returns T or rejects—apply these changes in the fetcher function to make the behavior explicit.src/ui/component.ts (1)
181-186: 외부 데이터 XSS 주의
overview가 API 응답에서 직접 삽입됩니다. TMDB API는 일반적으로 안전한 데이터를 제공하지만, 방어적 코딩 관점에서 텍스트 콘텐츠에 대한 이스케이프 처리를 고려해볼 수 있습니다.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/ui/component.ts` around lines 181 - 186, The template inserts the external variable overview directly into the HTML, risking XSS; update the rendering in src/ui/component.ts to escape or sanitize overview before interpolation (e.g., create or use an escapeHtml/sanitize function and apply it where overview is used in the template or move insertion to textContent assignment), referencing the overview variable and the template/section that renders it so all API-derived text is safely encoded or sanitized before being embedded.src/ui/business/interactor.ts (1)
71-71: 빈 주석 제거Line 71의 빈 주석
//을 제거해주세요.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/ui/business/interactor.ts` at line 71, Remove the stray empty comment token "//" found in the source (the lone comment shown in the diff) — delete that line so there’s no bare comment or trailing whitespace left; ensure the surrounding code compiles and run the linter/formatter to confirm no style violations remain.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/state/utils/state.ts`:
- Around line 6-12: The current branch uses typeof initialState === "object"
which treats null and arrays as plain objects and causes type/structure loss
when spreading; update the initialization in the state module so that
initialState is shallow-copied only when it's a plain object (non-null, not an
Array, and has Object.prototype as its prototype—use Object.getPrototypeOf or a
small isPlainObject helper) and otherwise assign initialState directly (preserve
arrays and nulls); also change the getter behavior (the getter function that
currently returns state by direct reference) to avoid leaking internal mutable
state by returning a shallow clone for arrays/objects and the value directly for
primitives, referencing the variables/functions initialState, state, and getter
to locate the changes.
In `@src/storage/domain/myRating.ts`:
- Around line 6-14: getMyRating calls getItemFromLocalStorage which can throw
(JSON.parse failure); wrap the call in a try-catch inside getMyRating (or add a
defensive try-catch in getItemFromLocalStorage) so parse errors don't bubble
up—on exception return 0 (and optionally log the error). Ensure you still handle
the savedRate -> parseInt flow (i.e., if savedRate falsy return 0; if parseInt
yields NaN return 0) but guard the initial getItemFromLocalStorage call with
error handling to prevent runtime crashes.
In `@src/storage/utils/localstorage.ts`:
- Around line 1-4: getItemFromLocalStorage should safely handle invalid JSON and
return the correct parsed type: wrap the JSON.parse call in a try-catch inside
getItemFromLocalStorage to catch SyntaxError and return null (or fallback)
instead of throwing; remove the incorrect cast "as string" and make the function
generic or return unknown (e.g., change signature to
getItemFromLocalStorage<T>(key: string): T | null) so the parsed value preserves
its original type, and optionally log or silently ignore parse errors per
project conventions.
In `@src/ui/business/interactor.ts`:
- Around line 59-60: The check after document.querySelectorAll is wrong because
querySelectorAll never returns null; update the logic around const myRatings =
document.querySelectorAll(".my-rating__content img") in interactor.ts: remove
the redundant if (!myRatings) return; and, if you need to short-circuit when no
elements exist, replace it with a length check (myRatings.length === 0) before
proceeding to iterate or manipulate the NodeList.
In `@src/ui/component.ts`:
- Around line 94-95: The close button and star images in movieModalSkeleton,
movieModalError, and movieModal are using literal src paths (e.g.,
"src/images/modal_button_close.svg") rather than the module-imported image
variables used at the top of the file; replace the literal string usages with
the appropriate imported identifiers (the same names used in the file header
imports for modal_button_close and star images) inside the JSX for the <img>
elements so all image assets are referenced consistently via imports.
- Around line 158-159: The filled-star <img> uses a literal
"src/images/star_filled.png" instead of the imported image variable; update the
component to use the imported symbol (e.g., starFilledImg) just like
starEmptyImg is used on line 20, and if starFilledImg is not yet imported at the
top of src/ui/component.ts add the import for the filled star image so all image
assets are referenced consistently via imports.
In `@step2.md`:
- Line 1: The document title in step2.md is incorrect: locate the Markdown
heading string "# javascript-movie-review step1" and change it to "#
javascript-movie-review step2" so the file name and top-level header match;
update only the heading text in step2.md to "step2".
---
Nitpick comments:
In `@index.html`:
- Line 55: Remove the dead commented modal markup so unused HTML doesn't confuse
ownership of rendering; specifically delete the commented line containing the
div with id="modalBackground" and class="modal-background active" from
index.html (remove the <!-- <div class="modal-background active"
id="modalBackground"></div> --> comment) to keep the markup clean.
In `@src/apis/utils.ts`:
- Around line 8-19: The fetcher function currently returns an implicit Promise<T
| undefined> (because the catch path returns nothing) and uses verbose
callbacks; update the fetcher signature to explicitly state the intended return
type and make the callbacks use optional chaining: either (A) if you want
callers to receive no value on error, change the signature to export async
function fetcher<T>(arg: FetcherArgs<T>): Promise<T | undefined> and replace
onLoading && onLoading() / onSuccess && onSuccess(response) / onError &&
onError(...) with onLoading?.() / onSuccess?.(response) / onError?.(error
instanceof Error ? error : new Error(String(error))); or (B) if callers should
always get a value or an error, change the signature to Promise<T> and re-throw
the caught error in the catch block after invoking onError?.(...), so the
function either returns T or rejects—apply these changes in the fetcher function
to make the behavior explicit.
In `@src/lib/render.ts`:
- Around line 1-7: The render helper declares callback as () => void but is
invoked with an async function (e.g., loadInitialMovie), so change the callback
signature to return a Promise (e.g., callback: () => Promise<void>), and update
the load event handler to await the callback and wrap the await in try/catch to
surface/rethrow/log any errors; reference the render function and its
addEventListener load handler and the async consumer loadInitialMovie to locate
and update the implementation.
- Line 2: Change the global call to addEventListener to explicitly use
window.addEventListener so the intent is clear; locate the top-level usage of
addEventListener in src/lib/render.ts and replace it with
window.addEventListener(...) (preserving the same event name, callback, and
options) to make the target object explicit.
In `@src/main.ts`:
- Line 1: Remove the explicit ".ts" extension from the import in src/main.ts by
changing import render from "./lib/render.ts" to import render from
"./lib/render" so it matches the project's import style (like "./page") and
maintains consistency across modules; update any similar imports in the same
file if present.
In `@src/state/business/movieState.ts`:
- Around line 4-11: Replace the array-index assignments that pull
getters/setters from nextPageNum, nextSearchPageNum, and requestMovieCount with
array destructuring to improve clarity; e.g. destructure nextPageNum into
[getNextPageNum, setNextPageNum], nextSearchPageNum into [getNextSearchPageNum,
setNextSearchPageNum], and requestMovieCount into [getRequestMovieCount,
setRequestMovieCount] so the intent of the variables (getters and setters) is
explicit in movieState.ts.
In `@src/storage/domain/myRating.ts`:
- Line 7: Extract the duplicated storage key creation into a single helper
function (e.g., buildRatingStorageKey or getRatingKey) and replace both
occurrences of the inline constant RATING_STORAGE_KEY = `rating_${movieId}` with
calls to that function; update functions that reference RATING_STORAGE_KEY to
call buildRatingStorageKey(movieId) so the key format is centralized and can be
changed in one place (ensure the helper accepts movieId and returns the same
`rating_${movieId}` string).
- Line 12: The parsing of savedRate uses parseInt without a radix which can
yield unexpected results; update the usage of parseInt(savedRate) in myRating.ts
to specify the radix (e.g., parseInt(savedRate, 10)) or replace it with
Number(savedRate) depending on desired behavior, ensuring the variable rate is
parsed reliably.
In `@src/storage/utils/localstorage.ts`:
- Around line 6-8: Replace the loose any with a generic type parameter to
improve type safety: change setItemInLocalStorage signature to something like
function setItemInLocalStorage<T>(key: string, value: T): void and use
JSON.stringify(value) as before; update any call sites if needed to pass
properly typed values so TypeScript infers T for the function.
In `@src/styles/thumbnail.css`:
- Around line 11-12: The thumbnail CSS currently sets width: 100% and a fixed
height: 300px which can distort images at different card widths; update the
thumbnail rule that contains the width/height declarations to use a responsive
sizing strategy—remove the fixed height or replace it with an aspect-ratio
(e.g., aspect-ratio: 16/9) and ensure the image inside uses object-fit:
cover/contain (or set the thumbnail element to height: auto and let the img
control aspect via object-fit) so the image scales without distortion; adjust
the selector in thumbnail.css that holds the width/height properties and add the
aspect-ratio/object-fit properties to the related img or container rule.
In `@src/ui/business/interactor.ts`:
- Line 71: Remove the stray empty comment token "//" found in the source (the
lone comment shown in the diff) — delete that line so there’s no bare comment or
trailing whitespace left; ensure the surrounding code compiles and run the
linter/formatter to confirm no style violations remain.
In `@src/ui/business/scrollObserver.ts`:
- Around line 10-15: The assignment in replaceLoadMoreScrollObserver uses an
unnecessary nullish coalescing to undefined; remove the "?? undefined" and
assign the result of observeLoadMoreScroll(callback) directly to
currentScrollDisconnect after calling currentScrollDisconnect?.(); update the
replaceLoadMoreScrollObserver function so it simply calls
currentScrollDisconnect?.(); then sets currentScrollDisconnect =
observeLoadMoreScroll(callback); keeping the same behavior but without the
redundant coalescing.
In `@src/ui/component.ts`:
- Around line 181-186: The template inserts the external variable overview
directly into the HTML, risking XSS; update the rendering in src/ui/component.ts
to escape or sanitize overview before interpolation (e.g., create or use an
escapeHtml/sanitize function and apply it where overview is used in the template
or move insertion to textContent assignment), referencing the overview variable
and the template/section that renders it so all API-derived text is safely
encoded or sanitized before being embedded.
In `@src/ui/domain/observer.ts`:
- Around line 8-17: observeHeaderScroll currently calls
reserveIntersectionHandler but does not return its disconnect function; update
observeHeaderScroll to return the disconnect function (or a no-op) so callers
can clean up the observer. Specifically, when topRatedMovie and
backgroundContainer exist, capture the return value of
reserveIntersectionHandler and return its disconnect (or the returned object)
from observeHeaderScroll; if the elements are missing, return a no-op disconnect
function to keep the API consistent. Ensure you reference observeHeaderScroll,
reserveIntersectionHandler, topRatedMovie, and backgroundContainer when making
the change.
- Around line 19-40: The observeLoadMoreScroll function has a redundant
loadMoreInView check inside the intersection handler and no guaranteed reset of
isLoading if callback() rejects; remove the inner loadMoreInView guard (it’s
already null-checked via getLoadMoreInViewElement) and wrap the await callback()
in a try/finally so isLoading is always set back to false, optionally logging or
rethrowing errors from the callback; references: observeLoadMoreScroll,
getLoadMoreInViewElement, reserveIntersectionHandler, isLoading, initialized,
and callback.
In `@src/ui/utils/eventListener.ts`:
- Around line 15-20: The ReplaceEventListenerToElementArg interface narrows
element to HTMLElement | null while the add util accepts HTMLElement | Element |
null; update ReplaceEventListenerToElementArg.element to Element | null to match
the add function's supported targets (or vice versa if you decide the common
contract should be HTMLElement), then adjust any call sites of replace or add to
conform to the unified type; locate the interface declaration
(ReplaceEventListenerToElementArg) and the replace/add functions to make the
change consistently across the module.
In `@src/ui/utils/inner.ts`:
- Around line 17-27: filterHTML rebuilds parent.innerHTML using outerHTML which
strips all event listeners; instead, update filterHTML to remove only matching
child elements (preserving listeners) by iterating over parent.children (e.g.,
Array.from(parent.children)) and calling Element.remove() or
parent.removeChild(child) for children whose classList contains the supplied
className; keep the function name filterHTML and its signature so clearSkeleton
continues to work but change its implementation to perform targeted removals
rather than reconstructing innerHTML.
In `@src/ui/utils/intersectionObserver.ts`:
- Around line 1-4: The callback in reserveIntersectionHandler currently types
its parameter as Partial<IntersectionObserverEntry>, but the observer always
supplies a full IntersectionObserverEntry; change the callback signature to use
IntersectionObserverEntry (not Partial) so callers don't need unnecessary
optional checks, update any related type annotations/handlers that reference
reserveIntersectionHandler and its callback, and run/typecheck to fix any call
sites that assumed optional fields.
- Line 11: The hardcoded { threshold: 0 } in
src/ui/utils/intersectionObserver.ts should be made configurable: modify the
function that constructs the IntersectionObserver (the function in this file
that calls new IntersectionObserver(..., { threshold: 0 })) to accept an options
parameter (either a numeric threshold or an IntersectionObserverInit object)
with a default of { threshold: 0 }, use that options value when creating the
observer, and update all callers to pass a custom threshold when needed; ensure
TypeScript types are updated so callers receive the new parameter and existing
behavior remains unchanged when the parameter is omitted.
🪄 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: 008969c0-0c2e-4efc-9358-fb0b0c387e04
⛔ Files ignored due to path filters (6)
src/images/modal_button_close.svgis excluded by!**/*.svgtemplates/images/logo.pngis excluded by!**/*.pngtemplates/images/modal_button_close.pngis excluded by!**/*.pngtemplates/images/star_empty.pngis excluded by!**/*.pngtemplates/images/star_filled.pngis excluded by!**/*.pngtemplates/images/woowacourse_logo.pngis excluded by!**/*.png
📒 Files selected for processing (45)
README.mdcypress/e2e/spec.cy.tsindex.htmlsrc/apis/api.tssrc/apis/dtos.tssrc/apis/utils.tssrc/component.tssrc/lib/render.tssrc/main.tssrc/observer.tssrc/page.tssrc/render.tssrc/state.tssrc/state/business/movieState.tssrc/state/domain/moviePage.tssrc/state/domain/requestMovieCount.tssrc/state/utils/state.tssrc/storage/domain/myRating.tssrc/storage/utils/localstorage.tssrc/styles/colors.csssrc/styles/main.csssrc/styles/modal.csssrc/styles/thumbnail.csssrc/ui/business/interactor.tssrc/ui/business/painter.tssrc/ui/business/scrollObserver.tssrc/ui/component.tssrc/ui/domain/movieElement.tssrc/ui/domain/observer.tssrc/ui/domain/render.tssrc/ui/utils/elementGetter.tssrc/ui/utils/eventListener.tssrc/ui/utils/inner.tssrc/ui/utils/intersectionObserver.tssrc/utils.tsstep1.mdstep2.mdtemplates/index.htmltemplates/modal.htmltemplates/styles/colors.csstemplates/styles/index.csstemplates/styles/main.csstemplates/styles/reset.csstemplates/styles/tab.csstemplates/styles/thumbnail.css
💤 Files with no reviewable changes (13)
- templates/styles/thumbnail.css
- templates/styles/index.css
- templates/styles/reset.css
- src/observer.ts
- src/utils.ts
- templates/index.html
- templates/styles/colors.css
- templates/styles/tab.css
- src/component.ts
- src/render.ts
- templates/styles/main.css
- src/state.ts
- templates/modal.html
👮 Files not reviewed due to content moderation or server errors (8)
- cypress/e2e/spec.cy.ts
- src/page.ts
- src/apis/dtos.ts
- src/apis/api.ts
- src/ui/business/painter.ts
- src/ui/domain/render.ts
- src/styles/modal.css
- src/ui/domain/movieElement.ts
Comment on lines
+6
to
+12
| if (typeof initialState === "object") { | ||
| state = { ...initialState }; | ||
| } else { | ||
| state = initialState; | ||
| } | ||
|
|
||
| const getter = () => state; |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
# First, let's look at the file structure and find the exact file
find . -type f -name "state.ts" | head -20Repository: woowacourse/javascript-movie-review
Length of output: 105
🏁 Script executed:
# Let's examine the file content around lines 6-12
cat -n src/state/utils/state.ts | head -30Repository: woowacourse/javascript-movie-review
Length of output: 913
🏁 Script executed:
# Get the full file to understand the function context
wc -l src/state/utils/state.tsRepository: woowacourse/javascript-movie-review
Length of output: 106
🏁 Script executed:
# Search for test files related to state.ts
find . -type f -name "*.test.ts" -o -name "*.spec.ts" | grep -i stateRepository: woowacourse/javascript-movie-review
Length of output: 61
🏁 Script executed:
# Search for how createStore is used in the codebase
rg "createStore" -A 3 -B 1 --type tsRepository: woowacourse/javascript-movie-review
Length of output: 998
🏁 Script executed:
# Check if there are any type constraints or interfaces for State
rg "StateType" -B 2 -A 2 --type tsRepository: woowacourse/javascript-movie-review
Length of output: 979
🏁 Script executed:
# Search for any documentation or comments about expected types
rg "object|array|state" src/state -B 2 -A 2 --type ts | head -50Repository: woowacourse/javascript-movie-review
Length of output: 1379
🏁 Script executed:
# Check if there are any usage patterns with objects or arrays
rg "createStore\(" -A 5 --type ts | head -100Repository: woowacourse/javascript-movie-review
Length of output: 346
🏁 Script executed:
# Let's verify JavaScript's typeof behavior with null and arrays
node <<'EOF'
console.log("typeof null:", typeof null);
console.log("typeof []:", typeof []);
console.log("typeof {}:", typeof {});
// Spread behavior verification
console.log("\nSpread with null:");
console.log({...null});
console.log("\nSpread with array:");
console.log({...[1, 2, 3]});
console.log("\nSpread with undefined:");
console.log({...undefined});
EOFRepository: woowacourse/javascript-movie-review
Length of output: 230
객체 초기화 분기에서 상태 타입이 변질될 수 있습니다.
6번 줄의 typeof initialState === "object" 조건은 null과 배열도 포함합니다. 현재 7번 줄 스프레드 연산자를 사용하면:
null→{}(null이 빈 객체로 변환)- 배열
[1, 2, 3]→{'0': 1, '1': 2, '2': 3}(배열 구조 손실)
이렇게 되면 초기 상태의 타입 정보가 손실됩니다.
이 함수의 설계 의도를 먼저 명확히 해보세요:
- plain object만 복사할 건지, 아니면 모든 객체 타입을 그대로 보존할 건지?
- 상태가 항상 primitive 값(숫자, 문자열 등)만 저장되는 건지, 객체/배열도 저장될 수 있는 건지?
이 기준을 정한 후 6~10번 줄의 분기를 다시 정리하고, 12번 줄의 직접 참조 반환(getter)도 함께 검토해 보세요.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/state/utils/state.ts` around lines 6 - 12, The current branch uses typeof
initialState === "object" which treats null and arrays as plain objects and
causes type/structure loss when spreading; update the initialization in the
state module so that initialState is shallow-copied only when it's a plain
object (non-null, not an Array, and has Object.prototype as its prototype—use
Object.getPrototypeOf or a small isPlainObject helper) and otherwise assign
initialState directly (preserve arrays and nulls); also change the getter
behavior (the getter function that currently returns state by direct reference)
to avoid leaking internal mutable state by returning a shallow clone for
arrays/objects and the value directly for primitives, referencing the
variables/functions initialState, state, and getter to locate the changes.
Comment on lines
+6
to
+14
| export const getMyRating = (movieId: string): number => { | ||
| const RATING_STORAGE_KEY = `rating_${movieId}`; | ||
| const savedRate = getItemFromLocalStorage(RATING_STORAGE_KEY); | ||
|
|
||
| if (!savedRate) return 0; | ||
|
|
||
| const rate = parseInt(savedRate); | ||
| return Number.isNaN(rate) ? 0 : rate; | ||
| }; |
There was a problem hiding this comment.
상위 함수의 예외 전파 가능성
getItemFromLocalStorage가 JSON.parse 실패 시 예외를 throw할 수 있습니다 (src/storage/utils/localstorage.ts 참조). 현재 코드에서는 이 예외가 처리되지 않아 런타임 에러가 발생할 수 있습니다.
localstorage.ts에서 try-catch를 추가하거나, 이 함수에서 방어적으로 처리하는 것을 고려해보세요.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/storage/domain/myRating.ts` around lines 6 - 14, getMyRating calls
getItemFromLocalStorage which can throw (JSON.parse failure); wrap the call in a
try-catch inside getMyRating (or add a defensive try-catch in
getItemFromLocalStorage) so parse errors don't bubble up—on exception return 0
(and optionally log the error). Ensure you still handle the savedRate ->
parseInt flow (i.e., if savedRate falsy return 0; if parseInt yields NaN return
0) but guard the initial getItemFromLocalStorage call with error handling to
prevent runtime crashes.
Comment on lines
+1
to
+4
| export const getItemFromLocalStorage = (key: string) => { | ||
| const item = localStorage.getItem(key); | ||
| return item ? (JSON.parse(item) as string) : null; | ||
| }; |
There was a problem hiding this comment.
JSON.parse 예외 처리 누락
JSON.parse는 저장된 값이 유효하지 않은 JSON일 경우 SyntaxError를 발생시킵니다. localStorage가 외부에서 손상되거나 다른 코드에 의해 변경될 수 있으므로, try-catch로 안전하게 처리하는 것이 좋습니다.
또한 반환 타입 as string은 정확하지 않습니다 - JSON.parse는 파싱된 값의 원래 타입을 반환합니다.
🛡️ 안전한 파싱 예시
-export const getItemFromLocalStorage = (key: string) => {
+export const getItemFromLocalStorage = <T>(key: string): T | null => {
const item = localStorage.getItem(key);
- return item ? (JSON.parse(item) as string) : null;
+ if (!item) return null;
+ try {
+ return JSON.parse(item) as T;
+ } catch {
+ return null;
+ }
};🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/storage/utils/localstorage.ts` around lines 1 - 4,
getItemFromLocalStorage should safely handle invalid JSON and return the correct
parsed type: wrap the JSON.parse call in a try-catch inside
getItemFromLocalStorage to catch SyntaxError and return null (or fallback)
instead of throwing; remove the incorrect cast "as string" and make the function
generic or return unknown (e.g., change signature to
getItemFromLocalStorage<T>(key: string): T | null) so the parsed value preserves
its original type, and optionally log or silently ignore parse errors per
project conventions.
src/ui/business/interactor.ts
Outdated
Comment on lines
+59
to
+60
| const myRatings = document.querySelectorAll(".my-rating__content img"); //TODO: 계층화 하기 | ||
| if (!myRatings) return; |
There was a problem hiding this comment.
querySelectorAll 반환값 검사 불필요
document.querySelectorAll은 null을 반환하지 않고 빈 NodeList를 반환합니다. if (!myRatings) return은 항상 false이므로 의미가 없습니다. 빈 목록 처리가 필요하다면 myRatings.length === 0을 확인해야 합니다.
🐛 수정 제안
export const setupMyRatingInteraction = (
onRatingSelect: (rating: number) => void,
) => {
const myRatings = document.querySelectorAll(".my-rating__content img"); //TODO: 계층화 하기
- if (!myRatings) return;
+ if (myRatings.length === 0) return;📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| const myRatings = document.querySelectorAll(".my-rating__content img"); //TODO: 계층화 하기 | |
| if (!myRatings) return; | |
| const myRatings = document.querySelectorAll(".my-rating__content img"); //TODO: 계층화 하기 | |
| if (myRatings.length === 0) return; |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/ui/business/interactor.ts` around lines 59 - 60, The check after
document.querySelectorAll is wrong because querySelectorAll never returns null;
update the logic around const myRatings =
document.querySelectorAll(".my-rating__content img") in interactor.ts: remove
the redundant if (!myRatings) return; and, if you need to short-circuit when no
elements exist, replace it with a length check (myRatings.length === 0) before
proceeding to iterate or manipulate the NodeList.
Comment on lines
+94
to
+95
| <img src="src/images/modal_button_close.svg" /> | ||
| </button> |
There was a problem hiding this comment.
모달 이미지 경로도 동일한 불일치
movieModalSkeleton, movieModalError, movieModal의 close 버튼 이미지(src/images/modal_button_close.svg)와 별점 이미지들이 리터럴 경로를 사용하고 있습니다. 파일 상단에서 import된 이미지들과 동일한 방식으로 처리하는 것을 권장합니다.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/ui/component.ts` around lines 94 - 95, The close button and star images
in movieModalSkeleton, movieModalError, and movieModal are using literal src
paths (e.g., "src/images/modal_button_close.svg") rather than the
module-imported image variables used at the top of the file; replace the literal
string usages with the appropriate imported identifiers (the same names used in
the file header imports for modal_button_close and star images) inside the JSX
for the <img> elements so all image assets are referenced consistently via
imports.
Comment on lines
+158
to
+159
| <img src="src/images/star_filled.png" class="star" /> | ||
| <span>${voteAverage.toFixed(1)}</span> |
There was a problem hiding this comment.
이미지 경로 불일치
Line 20에서는 import된 starEmptyImg를 사용하지만, line 158에서는 "src/images/star_filled.png" 리터럴 경로를 사용하고 있습니다. 빌드 환경에 따라 문제가 될 수 있습니다.
일관성을 위해 모든 이미지를 import하거나, 경로 처리 방식을 통일하는 것이 좋습니다.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/ui/component.ts` around lines 158 - 159, The filled-star <img> uses a
literal "src/images/star_filled.png" instead of the imported image variable;
update the component to use the imported symbol (e.g., starFilledImg) just like
starEmptyImg is used on line 20, and if starFilledImg is not yet imported at the
top of src/ui/component.ts add the import for the filled star image so all image
assets are referenced consistently via imports.
| @@ -0,0 +1,73 @@ | |||
| # javascript-movie-review step1 | |||
There was a problem hiding this comment.
문서 제목과 파일명 불일치
파일명은 step2.md인데 제목이 # javascript-movie-review step1로 되어 있습니다. step2로 수정이 필요해 보입니다.
📝 수정 제안
-# javascript-movie-review step1
+# javascript-movie-review step2📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| # javascript-movie-review step1 | |
| # javascript-movie-review step2 |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@step2.md` at line 1, The document title in step2.md is incorrect: locate the
Markdown heading string "# javascript-movie-review step1" and change it to "#
javascript-movie-review step2" so the file name and top-level header match;
update only the heading text in step2.md to "step2".
geongyu09
changed the title
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
🎯 미션 소개
🕵️ 셀프 리뷰(Self-Review)
제출 전 체크 리스트
리뷰 요청 & 논의하고 싶은 내용
1) 이번 단계에서 가장 많이 고민했던 문제와 해결 과정에서 배운 점
이번 미션에서 가장 고민을 했던 것은 기존 코드에 새로운 기능을 붙이거나 변경하는 것을 쉽게 하기 위해서는 어떻게 해야할까였습니다.
이번 미션에서 더보기 버튼에서 무한스크롤로 변경하기 요구사항이 있었는데, 해당 부분을 수정하는 것은 크게 어렵지 않았습니다.
그 이유를 생각해보았을 때, 더 보기 버튼을 눌렀을 때 할 일을 따로 상위에서 받아서 처리하도록 개발했기 때문이라고 생각하였습니다.
즉 ui를 그리는 로직과, 버튼이 눌렸을 때 동작하는 트리거 로직이 분리가 되어있었고, 이 트리거 부분만 수정해주면 되었기 때문이라고 생각이 들었습니다.
이를 통해서 확실히 ui 로직들은 분리가 되는 것이 좋겠다는 것을 깨닫게 되었습니다.
이와 더불어서 요구사항에는 없지만 영화 정보를 가져오는 외부 api를 다른 서비스로 변경해야한다는 것을 가정해보았습니다.
기존의 코드는 외부 api의 데이터 타입을 그대로 여러 곳에서 사용중이라, 변경에 따른 영향이 클 것이라는 생각이 들었습니다.
이를 해결하기 위해서 많은 크루들과 이야기를 한 끝에, DTO단을 두어서 외부의 데이터를 내부에서 사용하는 데이터의 형식으로 변환하는 과정을 거치도록 해서 그 영향력을 줄이도록 했습니다.
마지막으로, 현재로서 앞으로 어떤 변경사항이 추가될지 모르므로, 도메인과 관련된 코드를 분리하는 것이 정말 중요할 것이라는 생각이 들었습니다.
이에 따라서 코드의 계층(알아야 하는것, 몰라도 되는 것에 대한 책임)을 나누어서 개발해 보는 것이 올바를 것이라는 생각이 들었고, 이에 대한 고민을 많이 하게 되었습니다.
최종적으로 이번 미션에서는 유틸 / 도메인을 아는 단계 / 실제 비즈니스 문제를 해결하는 단계로 나누어서 개발해보았습니다.
그리고 각 계층은 바로 하위 계층에만 의존하도록 개발하려고 노력해보았습니다.
설계하면서 정말 가볍게 메모한 내용
2) 이번 리뷰를 통해 논의하고 싶은 부분
[코드의 계층화가 오히려 복잡도를 높이는 것은 아닐까?]
이번에는 변경 사항에 유연하게 대응할 수 있는 코드를 작성하기 위해, 나름대로 도메인을 분리하려는 시도를 해보았습니다.
구조는 크게 유틸 / 도메인을 아는 단계 / 실제 비즈니스 문제를 해결하는 단계로 나누어 개발을 진행했는데, 이 과정에서 생각보다 많은 고민을 하게 되었습니다.
특히 각 계층이 바로 하위 계층에만 의존하도록 설계하려고 했지만, 구현을 진행하다 보니 이를 지키지 못하는 순간들이 종종 발생했습니다. 예를 들어
ui/domain/observer.ts에서 동일 레벨의./movieElement를 참조하거나,ui/business/interactor.ts에서 바로 하위 계층이 아닌ui/utils/eventListener.ts를 사용하는 경우가 있었습니다.코드를 작성하다보니 모든 계층 규칙을 완벽히 지키는 것이 생각보다 어렵다는 것을 체감하게 되었습니다. 더 나아가, 계층을 억지로 지키려 할수록 오히려 코드가 더 복잡해지는 느낌도 받았습니다. 그래서 “이 방향이 정말 맞는 걸까?”라는 고민이 계속해서 들었습니다.
조이의 생각이 궁금합니다!
이와 같은 시도가 변경에 유연한 코드를 만들기 위한 올바른 접근인지, 그리고 실제로 도메인을 분리한다면 어떤 기준과 방식으로 나누는 것이 좋은지 궁금합니다!!
[비동기를 처리하는 로직은 언제 어떤 방식을 사용하는 것이 좋을까?]
프론트엔드 수업을 들으며 비동기 처리의 세 가지 방식(콜백, Promise, async/await)과 그 차이에 대해 공부하게 되었습니다.
수업을 듣기 전에는 콜백 방식을 활용해 fetcher를 구현하고 있었고, 당시에는 이 방식만으로도 fetch 과정에서의 비동기 상태를 처리하는 데에 충분히 괜찮다고 느꼈습니다.
하지만 수업 이후, async/await 방식이 더 선언적이고 가독성이 뛰어나다는 점을 알게 되었습니다. (물론 이번에는 시간적인 여유가 없어 async/await으로 리팩토링하지는 못했습니다.)
그러다 보니 자연스럽게 async/await이 다른 방식들에 비해 (어느 하나 부족함이 없는)상위호환처럼 느껴지기도 했습니다.
그럼에도 불구하고 콜백이나 Promise 방식이 여전히 사용되고 있는 것을 보면, 각 방식마다 고유한 특징과 장점이 있고, 특정 상황에서는 그 장점이 더 잘 드러나기 때문이 아닐까 하는 생각이 들었습니다.
실제 현업에서는 이 세 가지 방식이 각각 어떤 상황에서, 어떤 기준으로 선택되어 사용되는지 궁금합니다.
✅ 리뷰어 체크 포인트