[2단계 - 상세 정보 & UI/UX 개선하기] 아지 미션 제출합니다#299
[2단계 - 상세 정보 & UI/UX 개선하기] 아지 미션 제출합니다#299gustn99 wants to merge 62 commits intowoowacourse:gustn99from
Conversation
searchParams 기반으로 변경
|
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이 pull request는 무한 스크롤, 영화 상세 모달, 별점 기능을 중심으로 영화 애플리케이션을 전면 리팩토링합니다. 기존 페이지네이션 기반 "더 보기" 버튼 방식을 Intersection Observer 기반 무한 스크롤로 전환하고, 포스터/제목 클릭 시 상세 모달을 렌더링하며, localStorage 기반 별점 등록/관리 기능을 추가합니다. 컴포넌트 구조를 재구성하고 API 계층을 확장하며, 반응형 CSS를 개선하고 포괄적인 E2E 테스트를 추가합니다. Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes 🚥 Pre-merge checks | ✅ 3✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 17
🧹 Nitpick comments (9)
src/main.ts (1)
20-20: 헤더 이벤트 책임 분리 TODO는 별도 추적 이슈로 남겨두면 좋겠습니다.리팩터링 우선순위를 관리하기 위해 TODO를 이슈로 분리해두면 이후 단계에서 누락 없이 진행하기 좋습니다. 원하시면 이 TODO를 작업 단위로 쪼개 이슈 템플릿 형태로 정리해드릴게요.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/main.ts` at line 20, The inline TODO comment "// TODO: 헤더 컴포넌트를 분리, 이벤트 리스너를 컴포넌트 책임으로 변경" should be converted into a tracked issue: create a ticket in your issue tracker describing "extract Header component and move event listeners into component responsibility", include acceptance criteria and task breakdown, then replace the TODO in src/main.ts with a short reference to the new issue ID or URL (e.g., "ISSUE-1234: see tracker") so the intent is preserved but work is tracked centrally; update any related PR description or project board to link the new issue.src/apis/rate/api.ts (1)
16-29: 쓰기 경로는 한 곳으로 모아두는 편이 유지보수에 유리합니다.
createRate와updateRate가 지금은 완전히 같은 동작이라, 저장 형식이 바뀌면 두 군데를 같이 수정해야 합니다. 공통 저장 helper 하나로 묶을지 먼저 검토해보시면 좋겠습니다.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/apis/rate/api.ts` around lines 16 - 29, createRate and updateRate duplicate the same write logic; extract the shared persistence into a single helper (e.g., saveRate or setMovieRate) that accepts movieId and rate, uses getRates() and RATE_KEY to compose and write the JSON string to localStorage, then have both createRate and updateRate call that helper (update references to getRates, RATE_KEY, createRate, updateRate, and the helper name to keep behavior centralized).src/dom/components/SearchSeeMoreButton.ts (1)
1-24: 이 컴포넌트의 내보낸 함수들이 현재 코드에서 사용되지 않고 있습니다.
renderSearchSeeMoreButton과removeSearchSeeMoreButton함수는 파일 외부에서 어떤 곳에서도 호출되지 않고 있습니다. 무한 스크롤로 통합된 현재 흐름과 별개의 "더 보기" 경로가 유지되고 있는 상황입니다.다음을 생각해볼 만합니다:
- 이 컴포넌트가 실제로는 필요한 코드인가요? (예: 향후 feature flag로 활성화할 계획이 있다면?)
- 만약 현재 필요 없다면, 제거하여 코드베이스를 단순하게 유지하는 것이 나을까요?
불필요하다면 정리를 고려해 보세요.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/dom/components/SearchSeeMoreButton.ts` around lines 1 - 24, The exported functions renderSearchSeeMoreButton and removeSearchSeeMoreButton (and related symbols SEARCH_SEE_MORE_BUTTON_ID, createSearchSeeMoreButtonTemplate, searchSeeMoreButton) are unused; either remove this file entirely or wrap the functionality behind a clear feature-flag/prop so it is only kept if planned for future use—e.g., add a flag like enableSeeMore and gate renderSearchSeeMoreButton so callers must opt-in, or delete the file and its exports to simplify the codebase; ensure any imports elsewhere are removed or updated accordingly.src/dom/components/MovieModal.ts (1)
89-96: 모달 hide 시 참조 상태도 함께 정리해 주세요.
hideMovieModal()에서 DOM만 제거되고modalElement참조는 남습니다. hide 단계에서modalElement = null까지 맞춰두면showMovieModal()오동작 가능성을 줄일 수 있습니다.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/dom/components/MovieModal.ts` around lines 89 - 96, hideMovieModal currently removes the DOM node but leaves the modalElement reference alive which can cause showMovieModal to misbehave; after modalElement?.remove() and document.body.classList.remove("modal-open") set modalElement = null to fully clear the reference, and update showMovieModal to guard with a null-check (or recreate the element) before calling modalElement.showModal() to avoid calling methods on a stale reference.cypress/e2e/MovieRate.cy.ts (1)
9-121: 중복 setup 시나리오를 테스트 유틸로 묶어보면 유지보수가 쉬워집니다.
beforeEach와openModal + 별 클릭패턴이 반복됩니다. 공통 동작을 커스텀 커맨드/헬퍼로 추출하면 테스트 의도가 더 또렷해지고 변경 비용이 줄어듭니다.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@cypress/e2e/MovieRate.cy.ts` around lines 9 - 121, Many tests repeat the same setup and modal interactions; extract that duplicated logic into reusable helpers or Cypress custom commands to improve maintainability. Create a setup helper (used in place of the repeated beforeEach) that clears localStorage, calls interceptPopularPage1(), visits "/", and waits for "@getPopularPage1", and create modal helpers or custom commands (e.g., openModal(), selectRating(index) or selectRating(starIndex) that clicks "#rate-button-container button".eq(...), closeModal() for "#closeModal") and replace repeated sequences in the tests to call these helpers instead of duplicating the steps.cypress/e2e/InfiniteScroll.cy.ts (1)
82-84: 에러 테스트의 검증 강도를 한 단계 높일 수 있습니다.현재는
alert호출 여부만 확인합니다. 호출 횟수/메시지까지 함께 검증하면 회귀를 더 잘 잡아낼 수 있습니다.Also applies to: 104-106
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@cypress/e2e/InfiniteScroll.cy.ts` around lines 82 - 84, Increase assertion strength for the error tests by verifying alertStub was called the expected number of times and with the expected message when the "@getPopularError" route stub resolves: replace the loose expect(alertStub).to.have.been.called with stronger checks such as expect(alertStub).to.have.been.calledOnce (or calledTwice if appropriate) and expect(alertStub).to.have.been.calledWith(expectedMessage) (or use a regex/partial match) in the block handling cy.wait("@getPopularError"); repeat the same stricter assertions for the other occurrence around lines 104-106 so both error-path tests validate call count and payload.src/dom/compositions/Search.ts (1)
7-8:Home↔Search양방향 의존은 분리하는 편이 좋습니다.서로의
remove*를 직접 호출하는 구조는 변경 전파 범위를 키웁니다. 공통 정리 책임을 별도 레이어(예: page-shell cleanup 유틸)로 분리하면 페이지 조합 로직이 더 단순해집니다.Also applies to: 13-16, 84-91
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/dom/compositions/Search.ts` around lines 7 - 8, The Search module's direct import of removeHome creates a bidirectional dependency between Home and Search; extract the shared cleanup responsibilities into a new neutral utility (e.g., pageShellCleanup) and have both modules call that utility instead of calling each other's remove* functions. Create a new module that exports cleanup functions (e.g., removeHome, removeSearch or a single cleanup(registry) API) and update Search (references to removeHome) and Home (references to removeSearch) to import from that utility, or alternatively register cleanup callbacks with a central cleanup manager used by both; this removes the Home↔Search coupling and centralizes page teardown logic.src/dom/components/SearchThumbnailList.ts (1)
12-39: 리스트 초기화/생성 로직이 중복되어 있습니다.Line 12-23, Line 25-39의 공통 흐름(기존 리스트 제거 → 템플릿 삽입 → 요소 재조회)을 작은 내부 헬퍼로 분리해보면 어떨까요? 로딩/실데이터 렌더 함수의 책임이 더 분명해집니다.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/dom/components/SearchThumbnailList.ts` around lines 12 - 39, The removal/insertion/requery logic duplicated in renderSearchThumbnailLoading and renderSearchThumbnailList should be extracted into a small helper (e.g., initSearchThumbnailList(parent) or ensureSearchThumbnailList(parent)) that: checks and removes the existing searchThumbnailList, calls createSearchThumbnailListTemplate via parent.insertAdjacentHTML, reassigns searchThumbnailList = document.getElementById(SEARCH_THUMBNAIL_LIST_ID) and returns the element (or null). Replace the duplicated blocks in renderSearchThumbnailLoading and renderSearchThumbnailList with a single call to that helper and then call renderMovieItemsLoading or renderMovieItems only if the returned element is truthy (and movies.length > 0 for renderSearchThumbnailList).src/dom/components/MyRate.ts (1)
31-40: 별점 값 범위를 0~5로 정규화하면 예외 상황에 더 강해집니다.
getRate결과가 손상된 값(예: 9, -1)일 때 Line 39-40 렌더 수량이 깨질 수 있습니다. 저장/렌더 전에rate를 clamp하는 방어 로직을 한 단계 넣어보면 어떨까요?Also applies to: 58-62
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/dom/components/MyRate.ts` around lines 31 - 40, Clamp the raw rate returned by getRate before using it: replace direct uses of response?.rate (used to set rate for createMyRateTemplate and renderRateButtons) with a normalized value computed by bounding it to 0..5 (and optionally converting to an integer via Math.round/Math.floor as your UI expects). Update the code paths that call renderRateButtons with "filled" and "empty" (and the duplicate block around lines 58-62) to use this clampedRate variable instead of the raw rate so rendering counts can never be negative or exceed 5; reference symbols: getRate, response?.rate, rate, clampedRate, createMyRateTemplate, renderRateButtons, MY_RATE_ID, RATE_BUTTON_CONTAINER_ID.
🤖 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/apis/movie/type.ts`:
- Around line 18-31: The MovieDetail interface declares belongs_to_collection as
BelongsToCollection but TMDB can return null; update the type to
belongs_to_collection: BelongsToCollection | null in the MovieDetail interface
and adjust any code that assumes a non-null belongs_to_collection (e.g.,
checks/usages in functions that access MovieDetail.belongs_to_collection) to
handle the nullable case; also review related consumers of MovieDetail to avoid
non-null assertions after this change.
In `@src/apis/rate/api.ts`:
- Around line 5-7: getRates currently calls JSON.parse on
localStorage.getItem(RATE_KEY) without handling malformed JSON which can break
getRate/createRate/updateRate; wrap the JSON.parse in a try/catch inside
getRates, return an empty object {} on parse errors, and (optionally) reset
localStorage.setItem(RATE_KEY, "{}") and/or log the error to preserve future
reads; reference the getRates function and RATE_KEY so the fix is applied
exactly where parsing occurs.
In `@src/dom/components/Banner.ts`:
- Around line 36-38: 배경 이미지 설정에서 movie.backdrop_path가 null/empty인 경우를 처리하세요: 수정
대상은 Banner 컴포넌트의 bannerElement 스타일 설정 코드(참조: bannerElement, movie,
backdrop_path, import.meta.env.VITE_TMDB_IMAGE_BASE_URL). movie.backdrop_path가
유효한 문자열인지 검사하고, 유효하면 기존처럼 backgroundImage를 설정하고 그렇지 않으면 기본 이미지 URL을 사용하거나
backgroundImage를 제거/기본 클래스(또는 투명 배경)로 분기해 깨진 배경을 방지하도록 구현하세요.
In `@src/dom/components/MovieModal.ts`:
- Around line 59-62: The current code calls renderMyRate(MY_RATE_CONTAINER_ID,
movie?.id ?? -1) which can send a bogus -1 movie id to the rating API; change
the guard so renderMyRate is only invoked when movie?.id is a valid value.
Specifically, locate the block that queries MY_RATE_CONTAINER_ID and replace the
unconditional call with a conditional that checks movie and movie.id (e.g.,
truthy or Number.isInteger check) before calling renderMyRate(myRateContainer,
movie.id) to avoid saving a -1 key.
In `@src/dom/components/MyRate.ts`:
- Around line 14-23: The template returned by the component is missing the
closing div tags for the opened containers (the outer div with id ${MY_RATE_ID}
and the inner wrapper around the rate button/comment/score). Update the template
returned by the function in MyRate.ts to add the matching closing </div> tags
before the end of the template literal so the DOM structure for MY_RATE_ID and
the inner container that includes RATE_BUTTON_CONTAINER_ID, userRate and
rateConfig is properly closed.
In `@src/dom/components/RateButton.ts`:
- Around line 3-5: The RateButton component renders a plain <button> with a
decorative <img> (alt="") so screen readers get no label; update the <button> in
RateButton.ts to include a descriptive accessible name (e.g., aria-label={`Rate
${type} star${type > 1 ? 's' : ''}`} or a localized equivalent) and set
type="button" to prevent accidental form submission; keep the image as
decorative (alt="") or replace with meaningful text if you prefer visible
labels.
In `@src/dom/compositions/Home.ts`:
- Around line 76-83: The empty-state message used in renderHomeEmpty is
incorrect for the home (popular movies) context: update renderHomeEmpty so that
after calling removeHome() and removeSearch() it still finds the resultSection
and calls renderEmptyContainer(resultSection, "<home-specific message>") with a
home-appropriate string (e.g., "인기 영화가 없습니다." or similar) instead of "검색 결과가
없습니다."; locate renderHomeEmpty and replace the literal passed to
renderEmptyContainer accordingly, ensuring other calls to renderEmptyContainer
remain unchanged.
In `@src/dom/eventHandler/handleMovieItemClick.ts`:
- Around line 11-19: The current check in handleMovieItemClick only guards
against null/undefined for movieId but misses empty-string and non-numeric
cases; before calling getMovieDetail, ensure movieId is not an empty string and
that Number(movieId) produces a valid finite number (e.g., reject when movieId
=== "" or Number.isNaN(Number(movieId)) / !Number.isFinite(Number(movieId))). If
invalid, show the existing alert ("영화 정보를 불러올 수 없습니다.") and return; otherwise
pass the validated numeric id into getMovieDetail.
In `@src/dom/eventHandler/handleMovieSearch.ts`:
- Around line 19-23: The code both triggers a full navigation
(window.location.href = url.toString()) and immediately calls
renderSearchPage("init"), risking duplicate renders; choose one flow and remove
the other: either keep the navigation flow (keep window.location.href and
sessionStorage.setItem("page","1") then return immediately and remove await
renderSearchPage("init")) or keep the in-place render flow (call
sessionStorage.setItem and await renderSearchPage("init") but do not change
window.location.href); update the function containing these lines accordingly
(remove the conflicting call to renderSearchPage or the assignment to
window.location.href depending on the chosen flow).
In `@src/dom/eventHandler/handleSeeMore.ts`:
- Around line 5-8: The handler currently increments the page in sessionStorage
before calling renderHomePage("append"), which can skip a page if the append
fails; change the flow so the page is only incremented after a confirmed
successful load: update renderHomePage to return a success indicator (resolve
true/false or throw on error) or make renderHomePage("append") reject on
failure, then in handleSeeMore (and the similar block at the other location)
call await renderHomePage("append") first and only call
sessionStorage.setItem("page", String(prevPage + 1)) when that call succeeds;
ensure you reference renderHomePage and sessionStorage.getItem/setItem in these
handlers.
In `@src/dom/shared/MovieItem.ts`:
- Around line 35-39: renderMovieItems currently calls
parent.addEventListener("click", handleMovieItemClick) every time it's invoked,
causing duplicate handlers on repeated appends; fix by ensuring the click
listener is only registered once — either move the addEventListener call out of
renderMovieItems into the initial mount/init flow, or call
parent.removeEventListener("click", handleMovieItemClick) before
parent.addEventListener(...) inside renderMovieItems so handleMovieItemClick is
never attached multiple times.
- Around line 4-21: The createMovieItemTemplate function currently interpolates
external fields (movie.title, movie.vote_average, movie.poster_path, movie.id)
directly into an HTML string which can lead to XSS when later inserted with
insertAdjacentHTML; replace this HTML-string approach by constructing the LI and
its child elements via DOM APIs: createElement for li, div, img, p, strong,
etc., set element.textContent for movie.title and movie.vote_average, set
img.src using a safe base URL + movie.poster_path (validate/normalize the path)
and set id via element.id or dataset after sanitizing, and ensure any insertion
points that currently call insertAdjacentHTML (also around the other affected
locations) use appendChild/replaceChild with these constructed nodes instead of
raw HTML strings.
In `@src/pages/search.ts`:
- Line 20: The current page parsing (const page =
Number(sessionStorage.getItem("page") || 1)) can yield NaN or invalid values;
replace it with a defensive parse-and-validate that reads
sessionStorage.getItem("page"), converts to a number (e.g., parseInt or Number),
then check Number.isInteger(page) && page >= 1 and if that fails set page = 1;
ensure the validated page variable is what you pass to the API and/or store back
to sessionStorage if needed.
In `@styles/main.css`:
- Around line 245-268: The media query for (max-width: 1440px) is being negated
by a global rule `#wrap` { min-width: 1440px }; update the same media block to
override that by resetting `#wrap`'s min-width (e.g. set min-width: 0 or
min-width: initial) so the responsive rules (such as .logo, .movie-search,
.top-rated-container and its h3 clamping) can take effect; locate the `#wrap`
selector and add the override inside the existing `@media` (max-width: 1440px)
block to ensure the min-width is removed at that breakpoint.
In `@styles/modal.css`:
- Line 31: The Stylelint comment-whitespace-inside error is caused by the inline
comment wrapping the transition declaration; update the comment around
"transition: opacity 0.3s ease, visibility 0.3s ease;" so it follows the rule
(add a space after /* and before */) or remove the comment entirely; e.g.,
change the existing "/*transition: opacity 0.3s ease, visibility 0.3s ease;*/"
style comment to use proper spacing.
- Line 45: The CSS rule setting "outline: none;" for the modal close button
removes the keyboard focus indicator; remove that declaration from the
close-button selector in styles/modal.css or replace it by adding an explicit
focus-visible style (e.g., add a ":focus-visible" rule for the same selector
that provides a visible outline or ring via outline/box-shadow) so keyboard
users can see focus on the close button.
In `@styles/thumbnail.css`:
- Around line 5-6: grid-template-columns에서 repeat(auto-fill, 200px)를 사용하면 마지막 행에
빈 트랙이 남아 실제 카드들이 왼쪽으로 치우쳐 보입니다; 이 문제는 grid-template-columns의 repeat(auto-fill,
200px) 값을 repeat(auto-fit, 200px)으로 변경하여 남는 트랙을 접고 카드들을 가운데 정렬되도록 하면 해결됩니다.
---
Nitpick comments:
In `@cypress/e2e/InfiniteScroll.cy.ts`:
- Around line 82-84: Increase assertion strength for the error tests by
verifying alertStub was called the expected number of times and with the
expected message when the "@getPopularError" route stub resolves: replace the
loose expect(alertStub).to.have.been.called with stronger checks such as
expect(alertStub).to.have.been.calledOnce (or calledTwice if appropriate) and
expect(alertStub).to.have.been.calledWith(expectedMessage) (or use a
regex/partial match) in the block handling cy.wait("@getPopularError"); repeat
the same stricter assertions for the other occurrence around lines 104-106 so
both error-path tests validate call count and payload.
In `@cypress/e2e/MovieRate.cy.ts`:
- Around line 9-121: Many tests repeat the same setup and modal interactions;
extract that duplicated logic into reusable helpers or Cypress custom commands
to improve maintainability. Create a setup helper (used in place of the repeated
beforeEach) that clears localStorage, calls interceptPopularPage1(), visits "/",
and waits for "@getPopularPage1", and create modal helpers or custom commands
(e.g., openModal(), selectRating(index) or selectRating(starIndex) that clicks
"#rate-button-container button".eq(...), closeModal() for "#closeModal") and
replace repeated sequences in the tests to call these helpers instead of
duplicating the steps.
In `@src/apis/rate/api.ts`:
- Around line 16-29: createRate and updateRate duplicate the same write logic;
extract the shared persistence into a single helper (e.g., saveRate or
setMovieRate) that accepts movieId and rate, uses getRates() and RATE_KEY to
compose and write the JSON string to localStorage, then have both createRate and
updateRate call that helper (update references to getRates, RATE_KEY,
createRate, updateRate, and the helper name to keep behavior centralized).
In `@src/dom/components/MovieModal.ts`:
- Around line 89-96: hideMovieModal currently removes the DOM node but leaves
the modalElement reference alive which can cause showMovieModal to misbehave;
after modalElement?.remove() and document.body.classList.remove("modal-open")
set modalElement = null to fully clear the reference, and update showMovieModal
to guard with a null-check (or recreate the element) before calling
modalElement.showModal() to avoid calling methods on a stale reference.
In `@src/dom/components/MyRate.ts`:
- Around line 31-40: Clamp the raw rate returned by getRate before using it:
replace direct uses of response?.rate (used to set rate for createMyRateTemplate
and renderRateButtons) with a normalized value computed by bounding it to 0..5
(and optionally converting to an integer via Math.round/Math.floor as your UI
expects). Update the code paths that call renderRateButtons with "filled" and
"empty" (and the duplicate block around lines 58-62) to use this clampedRate
variable instead of the raw rate so rendering counts can never be negative or
exceed 5; reference symbols: getRate, response?.rate, rate, clampedRate,
createMyRateTemplate, renderRateButtons, MY_RATE_ID, RATE_BUTTON_CONTAINER_ID.
In `@src/dom/components/SearchSeeMoreButton.ts`:
- Around line 1-24: The exported functions renderSearchSeeMoreButton and
removeSearchSeeMoreButton (and related symbols SEARCH_SEE_MORE_BUTTON_ID,
createSearchSeeMoreButtonTemplate, searchSeeMoreButton) are unused; either
remove this file entirely or wrap the functionality behind a clear
feature-flag/prop so it is only kept if planned for future use—e.g., add a flag
like enableSeeMore and gate renderSearchSeeMoreButton so callers must opt-in, or
delete the file and its exports to simplify the codebase; ensure any imports
elsewhere are removed or updated accordingly.
In `@src/dom/components/SearchThumbnailList.ts`:
- Around line 12-39: The removal/insertion/requery logic duplicated in
renderSearchThumbnailLoading and renderSearchThumbnailList should be extracted
into a small helper (e.g., initSearchThumbnailList(parent) or
ensureSearchThumbnailList(parent)) that: checks and removes the existing
searchThumbnailList, calls createSearchThumbnailListTemplate via
parent.insertAdjacentHTML, reassigns searchThumbnailList =
document.getElementById(SEARCH_THUMBNAIL_LIST_ID) and returns the element (or
null). Replace the duplicated blocks in renderSearchThumbnailLoading and
renderSearchThumbnailList with a single call to that helper and then call
renderMovieItemsLoading or renderMovieItems only if the returned element is
truthy (and movies.length > 0 for renderSearchThumbnailList).
In `@src/dom/compositions/Search.ts`:
- Around line 7-8: The Search module's direct import of removeHome creates a
bidirectional dependency between Home and Search; extract the shared cleanup
responsibilities into a new neutral utility (e.g., pageShellCleanup) and have
both modules call that utility instead of calling each other's remove*
functions. Create a new module that exports cleanup functions (e.g., removeHome,
removeSearch or a single cleanup(registry) API) and update Search (references to
removeHome) and Home (references to removeSearch) to import from that utility,
or alternatively register cleanup callbacks with a central cleanup manager used
by both; this removes the Home↔Search coupling and centralizes page teardown
logic.
In `@src/main.ts`:
- Line 20: The inline TODO comment "// TODO: 헤더 컴포넌트를 분리, 이벤트 리스너를 컴포넌트 책임으로 변경"
should be converted into a tracked issue: create a ticket in your issue tracker
describing "extract Header component and move event listeners into component
responsibility", include acceptance criteria and task breakdown, then replace
the TODO in src/main.ts with a short reference to the new issue ID or URL (e.g.,
"ISSUE-1234: see tracker") so the intent is preserved but work is tracked
centrally; update any related PR description or project board to link the new
issue.
🪄 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: 9c2a5386-97d6-4c5e-859b-7755a4284a97
⛔ Files ignored due to path filters (7)
public/images/logo.pngis excluded by!**/*.pngpublic/images/modal_button_close.pngis excluded by!**/*.pngpublic/images/search.pngis excluded by!**/*.pngpublic/images/star_empty.pngis excluded by!**/*.pngpublic/images/star_filled.pngis excluded by!**/*.pngpublic/images/woowacourse_logo.pngis excluded by!**/*.pngpublic/images/으아아행성이.pngis excluded by!**/*.png
📒 Files selected for processing (49)
README.mdcypress/e2e/InfiniteScroll.cy.tscypress/e2e/MainRender.cy.tscypress/e2e/MovieModal.cy.tscypress/e2e/MovieRate.cy.tscypress/e2e/ReturnToMain.cy.tscypress/e2e/SeeMoreButton.cy.tscypress/e2e/spec.tscypress/fixtures/movieDetail.jsoncypress/fixtures/popularMoviesLastPage.jsoncypress/fixtures/searchMoviesLastPage.jsonindex.htmlsrc/apis/movie/api.tssrc/apis/movie/type.tssrc/apis/rate/api.tssrc/apis/rate/type.tssrc/apis/search/api.tssrc/apis/search/type.tssrc/constants/rates.tssrc/dom/components/Banner.tssrc/dom/components/EmptyContainer.tssrc/dom/components/ErrorContainer.tssrc/dom/components/MainSeeMoreButton.tssrc/dom/components/MovieModal.tssrc/dom/components/MyRate.tssrc/dom/components/PopularThumbnailList.tssrc/dom/components/RateButton.tssrc/dom/components/SearchSeeMoreButton.tssrc/dom/components/SearchThumbnailList.tssrc/dom/compositions/Home.tssrc/dom/compositions/Search.tssrc/dom/eventHandler/handleMovieItemClick.tssrc/dom/eventHandler/handleMovieSearch.tssrc/dom/eventHandler/handleSeeMore.tssrc/dom/render/renderBanner.tssrc/dom/render/renderLoadingUI.tssrc/dom/render/renderMainUI.tssrc/dom/render/renderResultSectionContent.tssrc/dom/render/renderSearchUI.tssrc/dom/render/renderThumbnailList.tssrc/dom/shared/MovieItem.tssrc/main.tssrc/pages/home.tssrc/pages/search.tsstyles/main.cssstyles/modal.cssstyles/reset.cssstyles/thumbnail.csstemplates/modal.html
💤 Files with no reviewable changes (8)
- src/dom/render/renderBanner.ts
- src/dom/render/renderLoadingUI.ts
- templates/modal.html
- src/dom/render/renderSearchUI.ts
- src/dom/render/renderMainUI.ts
- src/dom/render/renderThumbnailList.ts
- src/dom/render/renderResultSectionContent.ts
- cypress/e2e/SeeMoreButton.cy.ts
| export interface MovieDetail extends Omit<Movie, "genre_ids"> { | ||
| belongs_to_collection: BelongsToCollection; | ||
| budget: number; | ||
| genres: Genre[]; | ||
| homepage: string; | ||
| imdb_id: string; | ||
| origin_country: string[]; | ||
| production_companies: ProductionCompany[]; | ||
| production_countries: ProductionCountry[]; | ||
| revenue: number; | ||
| runtime: number; | ||
| spoken_languages: SpokenLanguage[]; | ||
| status: string; | ||
| tagline: string; |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "[1] 타입 선언 확인"
rg -n "belongs_to_collection:|logo_path:" src/apis/movie/type.ts
echo
echo "[2] 실제 fixture nullable 값 확인"
rg -n "\"belongs_to_collection\"\\s*:\\s*null|\"logo_path\"\\s*:\\s*null" cypress/fixtures/movieDetail.jsonRepository: woowacourse/javascript-movie-review
Length of output: 230
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "[1] 확인: movieDetail.json 파일에서 logo_path 필드 존재 여부"
rg -n "logo_path" cypress/fixtures/movieDetail.json | head -20
echo
echo "[2] ProductionCompany 타입 정의 확인 (logo_path를 포함하는 인터페이스)"
rg -A 5 "interface ProductionCompany" src/apis/movie/type.ts
echo
echo "[3] belongs_to_collection 주변 타입 정의 전체 확인"
sed -n '1,100p' src/apis/movie/type.ts | cat -nRepository: woowacourse/javascript-movie-review
Length of output: 127
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "[1] fixture 파일 존재 여부 및 크기 확인"
ls -lh cypress/fixtures/movieDetail.json 2>/dev/null || echo "파일 없음"
echo
echo "[2] fixture 파일 전체 내용 확인"
cat cypress/fixtures/movieDetail.json | head -50
echo
echo "[3] type.ts 파일에서 ProductionCompany 인터페이스 정의"
rg -A 10 "interface ProductionCompany" src/apis/movie/type.tsRepository: woowacourse/javascript-movie-review
Length of output: 1153
TMDB 응답에서 belongs_to_collection이 null이 될 수 있으므로 타입을 수정하세요.
Line 19의 belongs_to_collection은 실제 TMDB 응답(fixture 확인)에서 null이 오지만, 현재 타입은 BelongsToCollection으로 선언되어 있습니다. 이를 BelongsToCollection | null로 수정해야 실제 데이터와 타입이 일치합니다.
Line 48의 logo_path는 현재 fixture에서 production_companies가 비어있어 실제 nullable 여부를 직접 확인할 수 없으므로, 필요시 TMDB API 문서를 참고하여 별도로 검토해 주세요.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/apis/movie/type.ts` around lines 18 - 31, The MovieDetail interface
declares belongs_to_collection as BelongsToCollection but TMDB can return null;
update the type to belongs_to_collection: BelongsToCollection | null in the
MovieDetail interface and adjust any code that assumes a non-null
belongs_to_collection (e.g., checks/usages in functions that access
MovieDetail.belongs_to_collection) to handle the nullable case; also review
related consumers of MovieDetail to avoid non-null assertions after this change.
| const getRates = (): Record<string, number> => { | ||
| return JSON.parse(localStorage.getItem(RATE_KEY) ?? "{}"); | ||
| }; |
There was a problem hiding this comment.
손상된 localStorage 값 하나로 별점 UI 전체가 깨질 수 있습니다.
여기서 JSON.parse 예외를 잡지 않으면 RATE_KEY 값이 한 번만 깨져도 getRate/createRate/updateRate가 전부 실패합니다. 파싱 실패를 {}로 복구하는 방어선을 getRates()에 두는 편이 훨씬 견고합니다.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/apis/rate/api.ts` around lines 5 - 7, getRates currently calls JSON.parse
on localStorage.getItem(RATE_KEY) without handling malformed JSON which can
break getRate/createRate/updateRate; wrap the JSON.parse in a try/catch inside
getRates, return an empty object {} on parse errors, and (optionally) reset
localStorage.setItem(RATE_KEY, "{}") and/or log the error to preserve future
reads; reference the getRates function and RATE_KEY so the fix is applied
exactly where parsing occurs.
| if (bannerElement && movie) { | ||
| bannerElement.style.backgroundImage = `url(${import.meta.env.VITE_TMDB_IMAGE_BASE_URL}/w1280${movie.backdrop_path})`; | ||
| } |
There was a problem hiding this comment.
배경 이미지 경로 null 케이스를 처리해 주세요.
TMDB 응답에서 backdrop_path가 비어 있으면 Line 37에서 잘못된 URL이 만들어져 배경이 깨질 수 있습니다. 기본 배경 이미지(또는 배경 미설정) 분기를 추가하는 것이 안전합니다.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/dom/components/Banner.ts` around lines 36 - 38, 배경 이미지 설정에서
movie.backdrop_path가 null/empty인 경우를 처리하세요: 수정 대상은 Banner 컴포넌트의 bannerElement
스타일 설정 코드(참조: bannerElement, movie, backdrop_path,
import.meta.env.VITE_TMDB_IMAGE_BASE_URL). movie.backdrop_path가 유효한 문자열인지 검사하고,
유효하면 기존처럼 backgroundImage를 설정하고 그렇지 않으면 기본 이미지 URL을 사용하거나 backgroundImage를 제거/기본
클래스(또는 투명 배경)로 분기해 깨진 배경을 방지하도록 구현하세요.
| const myRateContainer = document.getElementById(MY_RATE_CONTAINER_ID); | ||
| if (myRateContainer) { | ||
| renderMyRate(myRateContainer, movie?.id ?? -1); | ||
| } |
There was a problem hiding this comment.
영화 상세 데이터가 없을 때 -1 별점 저장이 발생할 수 있습니다.
Line 61에서 movie?.id ?? -1를 전달하면 실패/빈 상태에서도 별점 API가 -1 키를 저장할 수 있습니다. movie?.id가 유효할 때만 renderMyRate를 호출하도록 가드가 필요합니다.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/dom/components/MovieModal.ts` around lines 59 - 62, The current code
calls renderMyRate(MY_RATE_CONTAINER_ID, movie?.id ?? -1) which can send a bogus
-1 movie id to the rating API; change the guard so renderMyRate is only invoked
when movie?.id is a valid value. Specifically, locate the block that queries
MY_RATE_CONTAINER_ID and replace the unconditional call with a conditional that
checks movie and movie.id (e.g., truthy or Number.isInteger check) before
calling renderMyRate(myRateContainer, movie.id) to avoid saving a -1 key.
| return ` | ||
| <div class="my-rate" id="${MY_RATE_ID}"> | ||
| <h3>내 별점</h3> | ||
| <div> | ||
| <div class="rate-button-container" id="${RATE_BUTTON_CONTAINER_ID}" aria-label="별점 ${userRate}점"></div> | ||
| <span class="comment">${rateConfig?.comment}</span> | ||
| <span class="score">(${rateConfig?.score}/10)</span> | ||
| </div> | ||
| `; | ||
| }; |
There was a problem hiding this comment.
템플릿 마크업 닫힘 태그가 하나 빠져 있습니다.
Line 15에서 시작한 div.my-rate가 명시적으로 닫히지 않습니다. 브라우저가 보정하더라도 DOM 구조가 예측과 달라질 수 있어 닫힘 태그를 맞춰두는 편이 안전합니다.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/dom/components/MyRate.ts` around lines 14 - 23, The template returned by
the component is missing the closing div tags for the opened containers (the
outer div with id ${MY_RATE_ID} and the inner wrapper around the rate
button/comment/score). Update the template returned by the function in MyRate.ts
to add the matching closing </div> tags before the end of the template literal
so the DOM structure for MY_RATE_ID and the inner container that includes
RATE_BUTTON_CONTAINER_ID, userRate and rateConfig is properly closed.
|
|
||
| const keyword = | ||
| new URLSearchParams(window.location.search).get("keyword") || ""; | ||
| const page = Number(sessionStorage.getItem("page") || 1); |
There was a problem hiding this comment.
page 파싱 결과 검증이 필요합니다.
Line 20은 저장값이 손상된 경우 NaN을 그대로 API에 보낼 수 있습니다. Number.isInteger(page)와 범위(1 이상) 체크 후 기본값으로 보정하는 방어 로직을 넣어 주세요.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/pages/search.ts` at line 20, The current page parsing (const page =
Number(sessionStorage.getItem("page") || 1)) can yield NaN or invalid values;
replace it with a defensive parse-and-validate that reads
sessionStorage.getItem("page"), converts to a number (e.g., parseInt or Number),
then check Number.isInteger(page) && page >= 1 and if that fails set page = 1;
ensure the validated page variable is what you pass to the API and/or store back
to sessionStorage if needed.
| @media (max-width: 1440px) { | ||
| .logo { | ||
| margin: 0 auto; | ||
| } | ||
|
|
||
| .movie-search { | ||
| top: 90px; | ||
| } | ||
|
|
||
| .top-rated-container { | ||
| position: absolute; | ||
| bottom: 64px; | ||
| max-width: 800px; | ||
| min-width: 0; | ||
| } | ||
|
|
||
| .top-rated-container h3 { | ||
| display: -webkit-box; | ||
| -webkit-box-orient: vertical; | ||
| -webkit-line-clamp: 2; | ||
| line-clamp: 2; | ||
| overflow: hidden; | ||
| } | ||
| } |
There was a problem hiding this comment.
반응형 규칙이 #wrap의 최소 너비에 의해 사실상 무력화됩니다.
Line 55의 #wrap { min-width: 1440px; }가 유지되어 있어, 1440px 이하에서 이번 미디어쿼리 변경이 있어도 가로 스크롤이 계속 발생할 수 있습니다. 같은 브레이크포인트에서 min-width를 해제/재정의해 주세요.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@styles/main.css` around lines 245 - 268, The media query for (max-width:
1440px) is being negated by a global rule `#wrap` { min-width: 1440px }; update
the same media block to override that by resetting `#wrap`'s min-width (e.g. set
min-width: 0 or min-width: initial) so the responsive rules (such as .logo,
.movie-search, .top-rated-container and its h3 clamping) can take effect; locate
the `#wrap` selector and add the override inside the existing `@media` (max-width:
1440px) block to ensure the min-width is removed at that breakpoint.
There was a problem hiding this comment.
#wrap 선택자 사용 안 함 -> 해당 사항 없습니닷
| z-index: 2; | ||
| position: relative; | ||
| width: 1000px; | ||
| /*transition: opacity 0.3s ease, visibility 0.3s ease;*/ |
There was a problem hiding this comment.
스타일린트 오류(comment-whitespace-inside)를 수정해주세요.
Line 31 주석 공백 규칙 위반으로 린트가 실패합니다.
수정 예시
- /*transition: opacity 0.3s ease, visibility 0.3s ease;*/
+ /* transition: opacity 0.3s ease, visibility 0.3s ease; */📝 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.
| /*transition: opacity 0.3s ease, visibility 0.3s ease;*/ | |
| /* transition: opacity 0.3s ease, visibility 0.3s ease; */ |
🧰 Tools
🪛 Stylelint (17.6.0)
[error] 31-31: Expected whitespace after "/*" (comment-whitespace-inside)
(comment-whitespace-inside)
[error] 31-31: Expected whitespace before "*/" (comment-whitespace-inside)
(comment-whitespace-inside)
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@styles/modal.css` at line 31, The Stylelint comment-whitespace-inside error
is caused by the inline comment wrapping the transition declaration; update the
comment around "transition: opacity 0.3s ease, visibility 0.3s ease;" so it
follows the rule (add a space after /* and before */) or remove the comment
entirely; e.g., change the existing "/*transition: opacity 0.3s ease, visibility
0.3s ease;*/" style comment to use proper spacing.
| color: white; | ||
| font-size: 20px; | ||
| cursor: pointer; | ||
| outline: none; |
There was a problem hiding this comment.
닫기 버튼 포커스 표시를 제거하면 키보드 접근성이 떨어집니다.
Line 45의 outline: none; 때문에 포커스 위치가 보이지 않습니다. 기본 아웃라인을 유지하거나 :focus-visible 스타일을 추가해주세요.
수정 예시
.close-modal {
position: absolute;
margin: 0;
padding: 0;
top: 40px;
right: 24px;
background: none;
border: none;
color: white;
font-size: 20px;
cursor: pointer;
- outline: none;
}
+
+.close-modal:focus-visible {
+ outline: 2px solid var(--color-bluegray-10);
+ outline-offset: 2px;
+ border-radius: 8px;
+}📝 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.
| outline: none; | |
| .close-modal { | |
| position: absolute; | |
| margin: 0; | |
| padding: 0; | |
| top: 40px; | |
| right: 24px; | |
| background: none; | |
| border: none; | |
| color: white; | |
| font-size: 20px; | |
| cursor: pointer; | |
| } | |
| .close-modal:focus-visible { | |
| outline: 2px solid var(--color-bluegray-10); | |
| outline-offset: 2px; | |
| border-radius: 8px; | |
| } |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@styles/modal.css` at line 45, The CSS rule setting "outline: none;" for the
modal close button removes the keyboard focus indicator; remove that declaration
from the close-button selector in styles/modal.css or replace it by adding an
explicit focus-visible style (e.g., add a ":focus-visible" rule for the same
selector that provides a visible outline or ring via outline/box-shadow) so
keyboard users can see focus on the close button.
| grid-template-columns: repeat(auto-fill, 200px); | ||
| justify-content: center; |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
# First, locate and check the thumbnail.css file
fd thumbnail.cssRepository: woowacourse/javascript-movie-review
Length of output: 99
🏁 Script executed:
# Read the thumbnail.css file to verify the grid configuration
cat -n styles/thumbnail.cssRepository: woowacourse/javascript-movie-review
Length of output: 1766
마지막 행의 카드가 왼쪽으로 치우칠 수 있습니다.
Line 5의 auto-fill은 고정 너비(200px) 트랙을 최대한 생성하되 빈 트랙도 유지합니다. 카드 개수가 열 개수보다 적은 마지막 행에서는 justify-content: center가 그리드 구조 자체를 가운데 정렬하므로, 실제 카드들은 왼쪽으로 치우쳐 보입니다. 남는 트랙을 접기 위해 auto-fit으로 변경하면 더 나은 가운데 정렬을 얻을 수 있습니다.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@styles/thumbnail.css` around lines 5 - 6, grid-template-columns에서
repeat(auto-fill, 200px)를 사용하면 마지막 행에 빈 트랙이 남아 실제 카드들이 왼쪽으로 치우쳐 보입니다; 이 문제는
grid-template-columns의 repeat(auto-fill, 200px) 값을 repeat(auto-fit, 200px)으로
변경하여 남는 트랙을 접고 카드들을 가운데 정렬되도록 하면 해결됩니다.
| // TODO: 이벤트 핸들러, 분리해야 하나? | ||
| myRateElement?.addEventListener("click", async (e) => { | ||
| if (e.target instanceof HTMLElement) { | ||
| const rateButtonContainer = document.getElementById( | ||
| RATE_BUTTON_CONTAINER_ID, | ||
| ) as HTMLElement; | ||
| const clickedButton = e.target.closest("button"); | ||
|
|
There was a problem hiding this comment.
이벤트 핸들러의 위치
돔 관련 로직과 너무 밀접해서 외부에 두는 것이 오히려 응집도가 낮아지는 것 같다는 느낌이 들었습니다. 그렇다고 순수하게 UI 로직이냐 하면 그것도 아니라서, 어느 레이어에 두어야 할지 감이 안 잡힙니다.
외부로 분리하는 게 좋았을까요?
There was a problem hiding this comment.
이 부분은 컴포넌트의 책임 정의에 대한 답변과 유사한 맥락으로 느껴졌습니다~
이 컴포넌트를 순수한 UI로 볼지, 아니면 비즈니스 로직까지 함께 가지는 응집도 높은 UI로 볼지에 따라 판단이 달라질 수 있을 것 같아요.
말씀해주신 것처럼 UI 부분만 분리해볼 수도 있겠지만, 현재 로직 규모만 보면 꼭 분리해야 할 정도로 방대하거나 큰 문제가 있어 보이지는 않았습니다.
There was a problem hiding this comment.
조이의 답변에서 거듭 현재 규모, 코드의 길이를 확인하시는 것을 보니, 확실히 설계란 그 맥락이 중요하다는 생각이 드네요!
| `; | ||
| }; | ||
|
|
||
| export const renderMyRate = async (parent: HTMLElement, movieId: number) => { |
There was a problem hiding this comment.
컴포넌트의 책임
컴포넌트는 관련 데이터를 조회할 책임까지 가지고 있는 게 일반적일까요? 아니면 UI만 담당하는 게 일반적일까요? 또는 팀 내에서 정의하기 나름인 문제일까요? renderMyRate 함수를 구현하는 과정에서, thumbnailList처럼 외부에서 rate를 조회하여 넘겨주어야 할지, movieId를 받아서 직접 rate를 조회해야 할지 고민되었습니다.
현재는 create/update 함수를 호출하기 위해 movieId를 받고 있는데, 컴포넌트의 책임을 위반하진 않았는지, 일관성을 깨버린 건 아닌지 고민이었습니다.
There was a problem hiding this comment.
일반적이라기보단 이 컴포넌트를 어떤 용도로 정의할건지 설계 포인트가 필요할 것 같아요. 순수한 UI 형태의 컴포넌트로 볼건지, 비즈니스 로직을 처리하는 UI로 볼건지요~! 지금 상태에서도 사실 내용물이 길지는 않아서 꼭 변경해야한다고 생각되는 정도는 안 것 같습니다 👍
그래서 이 부분은 일반적으로 어떻게 해야한다는 방향이 있기보다는, 이번 미션의 학습 목표에 맞춰 어디까지 분리할지의 문제에 초점을 두어야 할 것 같습니다.
There was a problem hiding this comment.
그러면 하나의 프로젝트에서 순수 UI와 비즈니스 로직이 포함된 UI를 같이 사용하는 것에 대해서는 어떻게 생각하시나요?
단순히 어떤 게 더 좋을까 보다는, 현재 thumbnailList에서 처리하는 방식과 myRate에서 처리하는 방식이 달라지면서 생겼던 고민이었던 것 같아요.
일관된 컨벤션을 규정하고 작성해야 할까요? 아니면 느슨하게 예외를 두고 처리해도 될까요? 지금 생각하기로는 언제는 내부에서 조회하고 언제는 외부에서 전달받고 하면 혼란을 야기할 수도 있을 것 같거든요!
There was a problem hiding this comment.
상황 : 이 기능은 빠른 배포를 위해 임시로 local storage 로 구현하기로 하였다. 다음번 배포에 서버 API 로 교체할 예정이다. 프론트엔드 개발자는 local storage, 서버 API 를 쉽고, 안전하게 갈아끼울 수 있는 구조로 개발하여야 한다.
위와 같은 조건에 따라 아직 서버 api는 아니지만 apis 폴더 안에 해당 기능을 구현했습니다. 사실 기존 개발 습관대로면 그냥 localStorage 접근 로직을 컴포넌트 안에 그대로 두고, 나중에 api 개발이 완료되면 수정했을 것 같아요. 그런데 여기에 많은 의미가 담겨 있다는 코치의 말을 듣고 더 신경 써서 추상화를 해 두었습니다.
이렇게 뒤로 미루는 건 안 좋은 개발 습관일까요? ㅋㅋㅋ 이 요구사항을 어떻게 받아들여야 할지 여전히 잘 모르겠습니다. 이미 결정된 확장에 대해 어떻게, 얼마나 대응해야 할까요? 저는 오히려 구현 및 수정 방향이 확실하고, 그 비용이 크지 않은 경우에는 지금 단계에서 고려하지 않는다!고 결정해 왔던 것 같습니다.
There was a problem hiding this comment.
서비스의 요구사항을 만족해야 하는 것이 첫 번째 우선순위라고 생각해서, 저는 오히려 이 부분을 신경 써서 추상화하신 방향이 좋다고 느껴졌습니다~
지금 당장 구현체가 localStorage이더라도, 사용처가 저장 방식의 세부 구현을 직접 알지 않도록 감싸 둔 점이 이후 서버 API로 교체될 때 변경 범위를 줄여줄 수 있다고 생각해요.
그래서 이 요구사항을 고려한 설계/구현은 막연한 미래 대비라기보다, 가능성이 높은 변경에 대한 선제 대응에 더 가깝다고 느껴졌습니다.
| export const renderHomeLoading = () => { | ||
| removeHome(); | ||
| removeSearch(); | ||
|
|
There was a problem hiding this comment.
순환 의존성
Home <-> Search 컴포지션 간의 순환 의존성을 어떻게 해소할 수 있을까요? 돔을 초기화하기 위해 매 render 함수 실행 시작에 각 컴포지션의 remove 함수를 호출하는데, 이 로직을 페이지 단으로 올려야 할지 고민이 들었습니다.
그렇게 할 경우, 썸네일 리스트를 append하는 경우에는 초기화가 필요하지 않기 때문에 추가적인 분기가 필요합니다. 이를 위해 페이지 단에도 render 함수와 append 함수를 따로 둘지를 고민했는데, 그것도 그다지 좋은 해결 방안이라고는 느껴지지 않는 것 같습니다.
이미 구조적인 다른 결함으로 인해 이런 문제가 발생한 걸까요?
이것 외에 더 합리적인 방법은 뭐가 있을까요?
There was a problem hiding this comment.
두 컴포지션이 서로의 remove를 직접 호출하다 보니 순환 의존이 생기는 것 같습니다.
핸들러 쪽에서 말씀주신 것처럼 옵저버를 추가하는 것도 하나의 방법이 될 수 있어 보이네요. 혹은 화면 전환 책임을 main이나 pages 같은 더 상위 레벨로 올려서, 라우팅처럼 조율하는 방식으로도 더 단순하게 풀릴 수 있을 것 같습니다. 예를 들면 페이지 전환 상태를 home, search 쪽에 내려주는 방식처럼요.
다만 지금도 파일 구조가 잘 정리되어 있어서, 이것을 구조적 결함이라고 볼 정도는 아니라고 느껴졌습니다. 그래서 반드시 바꿔야 한다기보다, 학습 목표를 더 달성해보고 싶고 필요하다고 느껴질 때 시도해보셔도 좋은 방향 정도로 생각해주시면 될 것 같아요.
| export const handleMainSeeMore = async () => { | ||
| const mainThumbnailList = document.getElementById("main-thumbnail-list"); | ||
| const url = new URL(window.location.href); | ||
| const params = url.searchParams; | ||
| const prevPage = Number(params.get("page") || 1); | ||
| const prevPage = Number(sessionStorage.getItem("page") || 1); | ||
| sessionStorage.setItem("page", String(prevPage + 1)); | ||
|
|
||
| params.set("page", String(prevPage + 1)); | ||
| url.search = params.toString(); | ||
| window.history.pushState({}, "", url.toString()); | ||
|
|
||
| try { | ||
| const popularMovies = await getPopularMovies({ | ||
| page: prevPage + 1, | ||
| language: "ko-KR", | ||
| }); | ||
| const isLastPage = popularMovies.page === popularMovies.total_pages; | ||
| const movies = popularMovies.results; | ||
|
|
||
| renderThumbnailList({ | ||
| movies, | ||
| thumbnailListElement: mainThumbnailList, | ||
| }); | ||
|
|
||
| renderResultSectionContent({ | ||
| isLoading: false, | ||
| isError: false, | ||
| isLastPage, | ||
| movies, | ||
| }); | ||
| } catch (error) { | ||
| let errorMessage = "알 수 없는 에러가 발생했습니다."; | ||
| if (error instanceof TMDBError) { | ||
| errorMessage = "TMDB에서 데이터를 불러오는 중 에러가 발생했습니다"; | ||
| } | ||
| window.alert(errorMessage); | ||
| } | ||
| await renderHomePage("append"); |
There was a problem hiding this comment.
순환 의존성
페이지와 이벤트 핸들러 간의 순환 의존성은 어떻게 해소할 수 있을까요? 또는, 과연 없앨 필요가 있는 문제일까요?
지난 로또 웹 미션에서 옵저버 패턴을 사용했는데, 여기서도 그렇게 처리하면 단방향 흐름을 제어할 수 있을 것 같습니다. 하지만 그렇게까지 해야 할까 싶은 생각도 듭니다. 이에 대한 조이의 생각이 궁금해요!
There was a problem hiding this comment.
HOME에 남겨주신 내용과 같은 질문이라고 보이며, 이벤트 핸들러가 page 상태와 렌더 흐름까지 함께 알고 있어서 생기는 문제처럼 느껴졌습니다.
그래서 이 부분도 옵저버 패턴으로 풀 수는 있다고 생각합니다. 실제로 이벤트 핸들러가 page나 다른 레이어를 직접 알지 않고, 페이지 상태 변화만 발행하도록 만들면 의존 방향을 더 단방향에 가깝게 정리할 수 있어서요.
다만 현재 규모에서는 꼭 새로운 패턴을 추가하지 않더라도, main이 화면 상태를 소유하고 handler는 “다음 페이지 요청”만 전달하도록 정리하는 것만으로도 충분히 많은 부분이 개선될 수 있을 것 같습니다.
그래서 이번 단계에서는 꼭 바꿔야 하는 문제라기보다, 더 단순한 구조를 지향해보고 싶다면 시도해볼 만한 포인트라고 생각합니다. 다만 지금 상태에서도 계층별 역할은 충분히 잘 나누어 두셨다고 느껴졌어요~!
There was a problem hiding this comment.
우선 검색 핸들러에서는 불필요한 페이지 렌더 의존성을 제거하고, window.location.href를 변경하는 것만으로 자연스럽게 main이 페이지를 관리하는 로직으로 넘어가도록 수정했습니다. 0924d6d
그런데 이 더보기 핸들러에서는 어떻게 구현을 해야 할지 아직 감이 안 잡히네요...
조이가 말씀 주신 것처럼 handler가 다음 페이지 요청만 전달하도록 하기 위해서도 결국에는 main이나 page 단에서의 특정 함수를 호출해야만 할 것 같아서요. (옵저버 패턴을 걸거나 커스텀 이벤트를 호출하지 않는 이상은요)
그렇게 수정해도 순환 의존성이 생기는 꼴이니까 의미가 있는 수정인가 싶으면서, 하나의 모듈 안에서도 의존성이 생기는 걸 의도한 함수와 그렇지 않은 함수를 구분하는 것만으로도 유의미한 분리인 걸까 싶은 고민이 듭니다.
제가 이해한 방향이 맞을까요?
| @media (max-width: 1024px) { | ||
| .modal { | ||
| width: 100vw; | ||
| margin: auto 0 0 0; | ||
| border-radius: 16px 16px 0 0; | ||
| } | ||
|
|
There was a problem hiding this comment.
breakpoint 관리 방식
현업에서 bp를 어떻게 관리하는지 궁금합니다. 지금은 1440px과 640px을 임의로 설정하여 값으로 처리하고 있는데(1024도 있었네요!!), lg-md-sm에 대한 상수를 관리하는 것이 일반적인가요?
단순 desktop, tablet, mobile로 나뉘지 않고 부가적인 bp가 생기는 경우에는 어떻게 처리하는지도 궁금합니다. 이전 프로젝트에서 bp가 4-5개씩 있었는데, 분기 지점을 명시적으로 선언하기 어렵다고 느꼈습니다.
There was a problem hiding this comment.
breakpoint는 실제로는 레이아웃이 변하는 지점이라는 기술적 기준도 있지만, 팀에서는 디자이너/기획과 함께 보는 공통 정책이 되는 경우도 많다고 생각하고 있어요. 그래서 반복적으로 쓰이는 전역 breakpoint는 토큰처럼 관리하고, 특정 컴포넌트에서만 필요한 세부 조정은 로컬하게 두는 식으로 섞어 가는 경우가 많았던 것 같습니다.
말씀하신 부가적인 breakpoint가 전역적으로 여러 곳에서 반복되는 값인지, 아니면 특정 UI에서만 필요한 값인지에 따라 선언 방식도 달라질 수 있을 것 같아요. 이런 차이를 먼저 구분해본 뒤, 선언할지 여부를 함께 개발하는 분들과 협의해보시면 좋은 부분이라고 생각되어지네요.
| export const renderMovieModal = ( | ||
| parent: HTMLElement, | ||
| movie: MovieDetail | null = null, | ||
| ) => { | ||
| if (modalElement) { | ||
| // TODO: 매번 remove하지 않으려면 상태 기반으로 데이터가 변경되도록? | ||
| modalElement.remove(); | ||
| } |
There was a problem hiding this comment.
현재는 사실상 모달 자체 cancel 이벤트가 발생하더라도 항상 모달 요소를 제거하고 새로 렌더링합니다. 이 경우 동일 모달을 재호출하더라도 api 요청을 새로 보내야 하는 문제가 있고, 서버 속도에 따라 UX에 치명적인 구현이 될 수도 있다고 생각해요. 동일한 돔 구조를 계속 넣었다 빼야 하니 그것 또한 하나의 비용으로 볼 수 있겠고요.
이전에는 오히려 api 응답을 캐싱하면 동일 요청에 대한 문제를 해결할 수 있고, 모달이 돔에 남아 있으면서 발생하는 enabled 처리 비용을 생각했을 때 아예 돔에서 지우는 게 낫겠다고 생각했었는데요,
이에 대한 조이의 생각은 어떠신가요?
평소 개발하실 때 어떤 방향을 선호하시는지, 그 이유가 궁금해요!
There was a problem hiding this comment.
아마 모달을 하나 두고 닫힐 때마다 여러 속성을 감추거나 내부 상태를 계속 관리하는 비용이 더 크다고 생각하셔서, 단순하게 remove하는 방식을 택하신 것 같다고 이해했어요.
다만 서버 속도에 따라 UX가 느려질 수 있는 부분은 remove 방식이냐, 하나의 모달을 유지하면서 데이터를 교체하느냐와는 별개로 동일하게 존재할 것 같습니다. 유지 방식에서도 모달을 먼저 띄운 뒤 로딩 상태나 스켈레톤 UI를 보여주고, 응답에 맞춰 내용을 교체하는 흐름은 충분히 가능해 보여서요.
그래서 현재 방식도 trade-off를 고려한 선택으로 이해되지만, 꼭 remove여야만 하는 구조라고 느껴지지는 않았습니다. 하나의 모달을 유지하면서 데이터를 교체하는 방향도 함께 고려해볼 수 있을 것 같아요. 다만 어느 한쪽이 정답이라기보다는, 상황에 따라 장단점을 보고 선택할 문제에 더 가깝다고 생각합니다~
gustn99
changed the title
coolchaem
left a comment
coolchaem
left a comment
There was a problem hiding this comment.
아지 안녕하세요~!
2단계 리뷰도 맡게 된 조이입니다.
리팩토링을 거치면서 역할과 책임에 따라 파일/모듈 분리가 많이 정리된 것 같아요.
리팩토링 전/후를 정리해주신 내용도 리뷰에 큰 도움이 되어 먼저 감사하다는 말씀드리고 싶습니다 👏
그리고 리팩토링을 하며 알게 된 점까지 함께 적어주셔서, 고민하신 과정과 궁금하신 부분이 잘 전달되었던 것 같아요.
논의하고 싶은 부분 중 다수는 코드리뷰 답 코멘트로 남겨두었고, 추가로 리팩토링 감각과 리뷰 요청 태도에 대해 질문 주신 부분에 답변드릴게요.
1. 리팩터링 감각
저는 리팩터링을 볼 때 작은 단위부터 설계하기보다는, 의존성을 한 번에 얼마나 끊을 수 있느냐를 더 기준으로 보는 편입니다.
기준은 크게 두 가지 정도인데요.
첫 번째는, 구조 설계상 부여한 책임을 벗어난 역할이 있는지, 있다면 어느 부분에서 분리가 필요한지를 보는 것입니다.
두 번째는, 함수가 과도하게 커졌는지 혹은 설계한 계층 간 의존 규칙을 어기고 있는지를 import 흐름을 따라 역추적해보는 편이에요.
그래서 이번에도 renderResultSectionContent 전체를 한 번에 바꾸기보다는, 먼저 함수를 분리해 책임의 경계선을 찾아가며 단위를 나누신 점이 좋은 접근이라고 느껴졌습니다.
보통 숲을 보느냐, 나무를 보느냐라는 표현을 하기도 하잖아요. 저는 조금 더 숲을 보고 접근하는 타입의 개발자라, 아지의 방식과 제 방식이 완전히 같지는 않을 수도 있을 것 같습니다. 그래서 어떤 리팩토링 기준을 가질지에 대해서는, 이번 경험을 바탕으로 본인만의 색깔을 조금씩 찾아가셔도 좋을 것 같아요.
2. 리뷰를 요청할 때 임하는 태도
이 부분은 정답이 있는 문제는 아니라서, 리뷰어인 제가 어떤 입장에서 리뷰를 보고 있는지 참고하는 정도로 받아들여주시면 좋을 것 같습니다~!
제가 우테코 FE 리뷰어로 지원했을 때도, 리뷰이를 통해 저 역시 무언가를 배울 수 있지 않을까 하는 마음이 컸어요. 제 연차가 아주 낮은 편은 아니지만, 새로운 기술적인 시도나 제가 생각하지 못한 접근 방식에 대해 기대하는 마음으로 지원했습니다.
그리고 이번 아지의 리팩토링 과정은 꽤 정교했고, 그 과정의 연결선까지 잘 보여주셔서 저 역시 제 리팩토링 철학이 조금 더 단단해지는 느낌을 받았어요.
그래서 말씀드리자면, 제가 드리는 말씀은 정답이라기보다 설계 방향을 함께 고민하거나, 혹은 조금 더 먼저 일해본 사람의 관점에서 방향을 한 번 더 비춰드리는 정도로 받아들여주시면 좋을 것 같습니다. 그렇게 생각하고 질문을 정리하시면, 어떤 방향으로 리뷰를 요청하면 좋을지도 점점 더 감이 오실 것 같아요.
그리고 이번 코드는 충분히 깔끔하게 잘 정리되어 있어서, “내가 정답에 맞게 가고 있나?”를 너무 크게 걱정하지 않으셔도 괜찮을 것 같습니다.
아래처럼 기준을 잡고 계신 점도 정말 좋았고, 이후에도 이렇게 설계 기준과 고민의 맥락을 함께 보여주시면 충분히 좋은 리뷰 요청이 될 것 같아요 👍
이 구조가 해결하려는 문제가 뭐냐?
이 구조의 trade-off는 뭐냐?
언제 이 구조를 쓰면 안 되냐?
한 번 더 다듬어보면 훨씬 좋아질 수 있는 부분들이 보여서, 이번에는 request changes로 남겨두었습니다!
| `; | ||
| }; | ||
|
|
||
| export const renderMyRate = async (parent: HTMLElement, movieId: number) => { |
There was a problem hiding this comment.
일반적이라기보단 이 컴포넌트를 어떤 용도로 정의할건지 설계 포인트가 필요할 것 같아요. 순수한 UI 형태의 컴포넌트로 볼건지, 비즈니스 로직을 처리하는 UI로 볼건지요~! 지금 상태에서도 사실 내용물이 길지는 않아서 꼭 변경해야한다고 생각되는 정도는 안 것 같습니다 👍
그래서 이 부분은 일반적으로 어떻게 해야한다는 방향이 있기보다는, 이번 미션의 학습 목표에 맞춰 어디까지 분리할지의 문제에 초점을 두어야 할 것 같습니다.
There was a problem hiding this comment.
서비스의 요구사항을 만족해야 하는 것이 첫 번째 우선순위라고 생각해서, 저는 오히려 이 부분을 신경 써서 추상화하신 방향이 좋다고 느껴졌습니다~
지금 당장 구현체가 localStorage이더라도, 사용처가 저장 방식의 세부 구현을 직접 알지 않도록 감싸 둔 점이 이후 서버 API로 교체될 때 변경 범위를 줄여줄 수 있다고 생각해요.
그래서 이 요구사항을 고려한 설계/구현은 막연한 미래 대비라기보다, 가능성이 높은 변경에 대한 선제 대응에 더 가깝다고 느껴졌습니다.
| @media (max-width: 1024px) { | ||
| .modal { | ||
| width: 100vw; | ||
| margin: auto 0 0 0; | ||
| border-radius: 16px 16px 0 0; | ||
| } | ||
|
|
There was a problem hiding this comment.
breakpoint는 실제로는 레이아웃이 변하는 지점이라는 기술적 기준도 있지만, 팀에서는 디자이너/기획과 함께 보는 공통 정책이 되는 경우도 많다고 생각하고 있어요. 그래서 반복적으로 쓰이는 전역 breakpoint는 토큰처럼 관리하고, 특정 컴포넌트에서만 필요한 세부 조정은 로컬하게 두는 식으로 섞어 가는 경우가 많았던 것 같습니다.
말씀하신 부가적인 breakpoint가 전역적으로 여러 곳에서 반복되는 값인지, 아니면 특정 UI에서만 필요한 값인지에 따라 선언 방식도 달라질 수 있을 것 같아요. 이런 차이를 먼저 구분해본 뒤, 선언할지 여부를 함께 개발하는 분들과 협의해보시면 좋은 부분이라고 생각되어지네요.
| export const handleMainSeeMore = async () => { | ||
| const mainThumbnailList = document.getElementById("main-thumbnail-list"); | ||
| const url = new URL(window.location.href); | ||
| const params = url.searchParams; | ||
| const prevPage = Number(params.get("page") || 1); | ||
| const prevPage = Number(sessionStorage.getItem("page") || 1); | ||
| sessionStorage.setItem("page", String(prevPage + 1)); | ||
|
|
||
| params.set("page", String(prevPage + 1)); | ||
| url.search = params.toString(); | ||
| window.history.pushState({}, "", url.toString()); | ||
|
|
||
| try { | ||
| const popularMovies = await getPopularMovies({ | ||
| page: prevPage + 1, | ||
| language: "ko-KR", | ||
| }); | ||
| const isLastPage = popularMovies.page === popularMovies.total_pages; | ||
| const movies = popularMovies.results; | ||
|
|
||
| renderThumbnailList({ | ||
| movies, | ||
| thumbnailListElement: mainThumbnailList, | ||
| }); | ||
|
|
||
| renderResultSectionContent({ | ||
| isLoading: false, | ||
| isError: false, | ||
| isLastPage, | ||
| movies, | ||
| }); | ||
| } catch (error) { | ||
| let errorMessage = "알 수 없는 에러가 발생했습니다."; | ||
| if (error instanceof TMDBError) { | ||
| errorMessage = "TMDB에서 데이터를 불러오는 중 에러가 발생했습니다"; | ||
| } | ||
| window.alert(errorMessage); | ||
| } | ||
| await renderHomePage("append"); |
There was a problem hiding this comment.
HOME에 남겨주신 내용과 같은 질문이라고 보이며, 이벤트 핸들러가 page 상태와 렌더 흐름까지 함께 알고 있어서 생기는 문제처럼 느껴졌습니다.
그래서 이 부분도 옵저버 패턴으로 풀 수는 있다고 생각합니다. 실제로 이벤트 핸들러가 page나 다른 레이어를 직접 알지 않고, 페이지 상태 변화만 발행하도록 만들면 의존 방향을 더 단방향에 가깝게 정리할 수 있어서요.
다만 현재 규모에서는 꼭 새로운 패턴을 추가하지 않더라도, main이 화면 상태를 소유하고 handler는 “다음 페이지 요청”만 전달하도록 정리하는 것만으로도 충분히 많은 부분이 개선될 수 있을 것 같습니다.
그래서 이번 단계에서는 꼭 바꿔야 하는 문제라기보다, 더 단순한 구조를 지향해보고 싶다면 시도해볼 만한 포인트라고 생각합니다. 다만 지금 상태에서도 계층별 역할은 충분히 잘 나누어 두셨다고 느껴졌어요~!
| export const renderHomeLoading = () => { | ||
| removeHome(); | ||
| removeSearch(); | ||
|
|
There was a problem hiding this comment.
두 컴포지션이 서로의 remove를 직접 호출하다 보니 순환 의존이 생기는 것 같습니다.
핸들러 쪽에서 말씀주신 것처럼 옵저버를 추가하는 것도 하나의 방법이 될 수 있어 보이네요. 혹은 화면 전환 책임을 main이나 pages 같은 더 상위 레벨로 올려서, 라우팅처럼 조율하는 방식으로도 더 단순하게 풀릴 수 있을 것 같습니다. 예를 들면 페이지 전환 상태를 home, search 쪽에 내려주는 방식처럼요.
다만 지금도 파일 구조가 잘 정리되어 있어서, 이것을 구조적 결함이라고 볼 정도는 아니라고 느껴졌습니다. 그래서 반드시 바꿔야 한다기보다, 학습 목표를 더 달성해보고 싶고 필요하다고 느껴질 때 시도해보셔도 좋은 방향 정도로 생각해주시면 될 것 같아요.
| myRateElement.remove(); | ||
| } | ||
|
|
||
| // TODO: 에러 핸들링 필요한가 |
There was a problem hiding this comment.
저장소 구현을 컴포넌트에서 직접 알지 않도록 apis/rate로 분리한 점은, 이후 서버 API로 교체될 때 유리한 구조라고 느껴졌습니다.
다만 현재는 JSON.parse가 실패하면 별점 기능 전체가 함께 깨질 수 있어서, getRates 단계에서 try...catch로 기본값을 복구해두면 더 안전할 것 같아요.
혹은 이 지점에서 에러가 났을 때 사용자에게 보여줄 UI를 별도로 두는 방향도 가능할 것 같아 한 번 확인해보시면 좋을 것 같습니다.
| // TODO: 이벤트 핸들러, 분리해야 하나? | ||
| myRateElement?.addEventListener("click", async (e) => { | ||
| if (e.target instanceof HTMLElement) { | ||
| const rateButtonContainer = document.getElementById( | ||
| RATE_BUTTON_CONTAINER_ID, | ||
| ) as HTMLElement; | ||
| const clickedButton = e.target.closest("button"); | ||
|
|
There was a problem hiding this comment.
이 부분은 컴포넌트의 책임 정의에 대한 답변과 유사한 맥락으로 느껴졌습니다~
이 컴포넌트를 순수한 UI로 볼지, 아니면 비즈니스 로직까지 함께 가지는 응집도 높은 UI로 볼지에 따라 판단이 달라질 수 있을 것 같아요.
말씀해주신 것처럼 UI 부분만 분리해볼 수도 있겠지만, 현재 로직 규모만 보면 꼭 분리해야 할 정도로 방대하거나 큰 문제가 있어 보이지는 않았습니다.
| export const renderMovieModal = ( | ||
| parent: HTMLElement, | ||
| movie: MovieDetail | null = null, | ||
| ) => { | ||
| if (modalElement) { | ||
| // TODO: 매번 remove하지 않으려면 상태 기반으로 데이터가 변경되도록? | ||
| modalElement.remove(); | ||
| } |
There was a problem hiding this comment.
아마 모달을 하나 두고 닫힐 때마다 여러 속성을 감추거나 내부 상태를 계속 관리하는 비용이 더 크다고 생각하셔서, 단순하게 remove하는 방식을 택하신 것 같다고 이해했어요.
다만 서버 속도에 따라 UX가 느려질 수 있는 부분은 remove 방식이냐, 하나의 모달을 유지하면서 데이터를 교체하느냐와는 별개로 동일하게 존재할 것 같습니다. 유지 방식에서도 모달을 먼저 띄운 뒤 로딩 상태나 스켈레톤 UI를 보여주고, 응답에 맞춰 내용을 교체하는 흐름은 충분히 가능해 보여서요.
그래서 현재 방식도 trade-off를 고려한 선택으로 이해되지만, 꼭 remove여야만 하는 구조라고 느껴지지는 않았습니다. 하나의 모달을 유지하면서 데이터를 교체하는 방향도 함께 고려해볼 수 있을 것 같아요. 다만 어느 한쪽이 정답이라기보다는, 상황에 따라 장단점을 보고 선택할 문제에 더 가깝다고 생각합니다~
| export const renderMovieItems = (parent: HTMLElement, movies: Movie[]) => { | ||
| const itemsHTML = movies.map(createMovieItemTemplate).join(""); | ||
| parent.insertAdjacentHTML("beforeend", itemsHTML); | ||
| parent.addEventListener("click", handleMovieItemClick); | ||
| }; |
There was a problem hiding this comment.
CodeRabbit이 짚어준 것처럼, 이벤트를 등록했다면 removeEventListener를 어느 시점에 함께 고려할지도 한 번 검토해보시면 좋겠네요~
실제로 해제가 되지 않으면 클릭 핸들러가 두 번 이상 실행되는 상황도 충분히 생길 수 있어서, 당장 눈에 띄는 버그가 없더라도 등록과 해제 시점을 함께 보는 습관이 도움이 될 수 있을 것 같습니다.
gustn99
left a comment
gustn99
left a comment
There was a problem hiding this comment.
좋은 피드백 감사합니다 조이!
아직 말씀 주신 로컬스토리지 JSON.parse 에러 핸들링, 이벤트 핸들러 클린업 관련해서는 더 고민하고 수정해야 하는데요, 그 부분을 살펴보기 전에 더 논의하고 싶은 부분이 있어 먼저 코멘트를 남깁니다! 다른 내용도 되도록 내일 밤 안으로 반영해볼게요!
| `; | ||
| }; | ||
|
|
||
| export const renderMyRate = async (parent: HTMLElement, movieId: number) => { |
There was a problem hiding this comment.
그러면 하나의 프로젝트에서 순수 UI와 비즈니스 로직이 포함된 UI를 같이 사용하는 것에 대해서는 어떻게 생각하시나요?
단순히 어떤 게 더 좋을까 보다는, 현재 thumbnailList에서 처리하는 방식과 myRate에서 처리하는 방식이 달라지면서 생겼던 고민이었던 것 같아요.
일관된 컨벤션을 규정하고 작성해야 할까요? 아니면 느슨하게 예외를 두고 처리해도 될까요? 지금 생각하기로는 언제는 내부에서 조회하고 언제는 외부에서 전달받고 하면 혼란을 야기할 수도 있을 것 같거든요!
| // TODO: 이벤트 핸들러, 분리해야 하나? | ||
| myRateElement?.addEventListener("click", async (e) => { | ||
| if (e.target instanceof HTMLElement) { | ||
| const rateButtonContainer = document.getElementById( | ||
| RATE_BUTTON_CONTAINER_ID, | ||
| ) as HTMLElement; | ||
| const clickedButton = e.target.closest("button"); | ||
|
|
There was a problem hiding this comment.
조이의 답변에서 거듭 현재 규모, 코드의 길이를 확인하시는 것을 보니, 확실히 설계란 그 맥락이 중요하다는 생각이 드네요!
| export const handleMainSeeMore = async () => { | ||
| const mainThumbnailList = document.getElementById("main-thumbnail-list"); | ||
| const url = new URL(window.location.href); | ||
| const params = url.searchParams; | ||
| const prevPage = Number(params.get("page") || 1); | ||
| const prevPage = Number(sessionStorage.getItem("page") || 1); | ||
| sessionStorage.setItem("page", String(prevPage + 1)); | ||
|
|
||
| params.set("page", String(prevPage + 1)); | ||
| url.search = params.toString(); | ||
| window.history.pushState({}, "", url.toString()); | ||
|
|
||
| try { | ||
| const popularMovies = await getPopularMovies({ | ||
| page: prevPage + 1, | ||
| language: "ko-KR", | ||
| }); | ||
| const isLastPage = popularMovies.page === popularMovies.total_pages; | ||
| const movies = popularMovies.results; | ||
|
|
||
| renderThumbnailList({ | ||
| movies, | ||
| thumbnailListElement: mainThumbnailList, | ||
| }); | ||
|
|
||
| renderResultSectionContent({ | ||
| isLoading: false, | ||
| isError: false, | ||
| isLastPage, | ||
| movies, | ||
| }); | ||
| } catch (error) { | ||
| let errorMessage = "알 수 없는 에러가 발생했습니다."; | ||
| if (error instanceof TMDBError) { | ||
| errorMessage = "TMDB에서 데이터를 불러오는 중 에러가 발생했습니다"; | ||
| } | ||
| window.alert(errorMessage); | ||
| } | ||
| await renderHomePage("append"); |
There was a problem hiding this comment.
우선 검색 핸들러에서는 불필요한 페이지 렌더 의존성을 제거하고, window.location.href를 변경하는 것만으로 자연스럽게 main이 페이지를 관리하는 로직으로 넘어가도록 수정했습니다. 0924d6d
그런데 이 더보기 핸들러에서는 어떻게 구현을 해야 할지 아직 감이 안 잡히네요...
조이가 말씀 주신 것처럼 handler가 다음 페이지 요청만 전달하도록 하기 위해서도 결국에는 main이나 page 단에서의 특정 함수를 호출해야만 할 것 같아서요. (옵저버 패턴을 걸거나 커스텀 이벤트를 호출하지 않는 이상은요)
그렇게 수정해도 순환 의존성이 생기는 꼴이니까 의미가 있는 수정인가 싶으면서, 하나의 모듈 안에서도 의존성이 생기는 걸 의도한 함수와 그렇지 않은 함수를 구분하는 것만으로도 유의미한 분리인 걸까 싶은 고민이 듭니다.
제가 이해한 방향이 맞을까요?
2 participants
🎯 미션 소개
🕵️ 셀프 리뷰(Self-Review)
제출 전 체크 리스트
리뷰 요청 & 논의하고 싶은 내용
1) 이번 단계에서 가장 많이 고민했던 문제와 해결 과정에서 배운 점
step1에서는 사실 코드의 구조적인 부분을 크게 고려하지 못했습니다. step1 수업에서 응집도/결합도에 대한 내용을 다루면서 구조적인 재편성이 필요하다고 느꼈고, 3일이 넘는 시간을 리팩터링에 할애했습니다.
(원정대 등 다른 활동들 사이에서 시간 관리를 잘 하지 못한 탓도 있었습니다 😭)차라리 갈아엎고 처음부터 시작하고픈 마음도 몇 번이나 들었지만, 이번에는 꼭 점진적으로 개선하는 경험을 해내고 싶었습니다. 그 과정에서 지난 미션 코멘트로 리팩터링의 진입점에 대한 질문을 드리게 되었고, 조이의 답변이 정말 큰 도움이 되었습니다.리팩터링 전
리팩터링 후
무분별한 돔 접근을 막기 위해 나 자신만 알게 한다는 목표로 각 컴포넌트를 분리, 컴포넌트를 렌더링하고자 하는 부모 요소를 파라미터로 전달받도록 구현했습니다. (이전에 말씀드린 renderThumbnailList 함수와 비슷한 구조를 다른 요소들에게도 적용해야겠다는 생각이 출발점이었습니다.) 나아가서 자식 요소에 대한 돔 접근을 최소화하기 위해 해당 컴포넌트가 자신을 remove할 수 있도록 설계했습니다.
각 페이지의 상태별로 컴포넌트를 조합하는 컴포지션 레이어를 두었고, 최종적으로 페이지 -> 컴포지션 단방향 흐름을 구현했습니다.
이 과정에서 class를 도입해야 할까 하는 의문이 들었는데, 둘 이상의 개별 상태를 가지는 컴포넌트 인스턴스를 만들 일이 없었기 때문에 현재 단계에서는 모듈 기반의 싱글톤 패턴으로도 충분하다고 판단했습니다.
a. 작은 단위의 수정이란?
step1 코드에서 가장 문제라고 느낀 지점은 흩어져 있는 돔 선택 로직과 너무 거대한 renderResultSectionContent 함수였습니다. 새로운 돔 요소가 추가되거나 기존 요소가 수정되는 경우, 해당 파일 + 부모 및 자식 파일 + renderResultSectionContent까지 연쇄적으로 수정될 가능성이 있었기 때문입니다. 낮은 응집도와 높은 결합도의 문제라고 판단했습니다.
renderResultSectionContent 함수가 가지고 있는 거대한 책임을 각 페이지, 요소 단으로 내리기 위해 컴포넌트 단위로 분할해야겠다는 생각을 했고, 작은 단위부터 리팩터링하려면 작은 컴포넌트부터 만들어야겠다고 생각했습니다. 그런데 컴포넌트를 만들고 해당 컴포넌트에 대한 의존성을 추가했더니, 거대한 renderResultSectionContent 함수와 그 외 로직들 안에 서로 다른 방식의 코드가 섞이면서 오히려 더 건드리기 어려운 상태가 되었습니다.
그 상태에서 조금 오래 방황한 것 같습니다. 그러던 중 조이의 피드백을 받고 renderResultSectionContent 함수를 작은 단위로 쪼개는 것부터 시작했습니다. 각 함수가 관리하는 단위에 따라서 가장 파생되는 변경이 적은 요소를 컴포넌트로 분리했고, 수정된 코드와 아직 수정되지 않은 코드 사이의 무결성이 보장되면서 그 이후로는 훨씬 수월하게 작업할 수 있었습니다.
작은 단위라는 게 단순히 구현의 영역이 아니라 영향을 주는 범위에 대한 이야기라는 것을 깨달은 짜릿한 순간이었네요!
b. 어디까지가 내 주관일까?
수업을 듣고 혼자 고민도 많이 하고, AI에 자문도 구해 보고, 다른 크루들의 코드도 살펴보며 여러 구현 방식에 대해 고민했습니다. class 도입에 대해, page 단위의 도입에 대해, template 방식에 대해... 여러 방식을 시도하고 또 채택하기도 하면서 가장 고민되었던 지점은 그저 편승하고 있는 건 아닐까 하는 것이었습니다.
혼자 생각했으면 절대 도달하지 못했을 상태... 다른 코드들을 보고 다른 크루들의 의견을 들어서 구현이 가능했는데, 여기에 과연 내 주관이 들어간 게 맞을까, 하는 고민이요. 더 나아가서는 리액트 등 라이브러리, 프레임워크의 철학을 가져오지 말라는 요구사항이 있었는데, 컴포넌트라거나, 개별 요소가 생명주기를 결정하는 등 이런 결정에 과연 그러한 패턴의 영향이 없었을까 하는 고민도 들었습니다.
꼭 이 미션만의 문제가 아니라, AI를 활용하는 여러 상황에서 이런 고민은 자연스럽게 따라오는 것 같습니다.
그래도 이번에 그 기준을 잡아보았어요.
여기에 대답할 수 있다면 충분히 학습하고, 사고하여 결정했다고 생각하기로요!
2) 이번 리뷰를 통해 논의하고 싶은 부분
코드를 보면 더 이해가 쉬울 만한 것들은 코멘트로 한 번 더 남겨두었으니 참고 부탁드립니다!
그 외 코드 상 구현에 관련된 것들도 함께 남겨두었습니다.
작은 단위라는 것에 대한 생각은 좀 바로잡았지만, 여전히 그걸 어떻게 캐치해야 할지 감이 안 잡히는 것 같습니다. 조이라면 이 리팩터링을 어떤 흐름으로 접근하셨을 것 같나요? 구체적인 그 사고 과정을 배우고 싶습니다.
Home <-> Search 컴포지션 간의 순환 의존성을 어떻게 해소할 수 있을까요? 돔을 초기화하기 위해 매 render 함수 실행 시작에 각 컴포지션의 remove 함수를 호출하는데, 이 로직을 페이지 단으로 올려야 할지 고민이 들었습니다. 이것 외에 더 좋은 방법이 있을까요?
또, 페이지와 이벤트 핸들러 간의 순환 의존성은 어떻게 해소할 수 있을까요? 또는, 과연 없앨 필요가 있는 문제일까요? 지난 로또 웹 미션에서 옵저버 패턴을 사용했는데, 여기서도 그렇게 처리하면 단방향 흐름을 제어할 수 있을 것 같습니다. 하지만 그렇게까지 해야 할까 싶은 생각도 듭니다. 이에 대한 조이의 생각이 궁금해요.
현재 코드에는 이벤트 핸들러가 eventHandler 폴더로 분리된 것도, 컴포지션/컴포넌트 파일 내에 그대로 남아 있는 것도 있습니다. 그 이유는... 돔 관련 로직과 너무 밀접해서 외부에 두는 것이 오히려 응집도가 낮아지는 것 같다는 느낌이 들었습니다. 그렇다고 순수하게 UI 로직이냐 하면 그것도 아니라서, 어느 레이어에 두어야 할지 감이 안 잡힙니다.
컴포넌트는 관련 데이터를 조회할 책임까지 가지고 있는 게 일반적일까요? 아니면 UI만 담당하는 게 일반적일까요? 또는 팀 내에서 정의하기 나름인 문제일까요? renderMyRate 함수를 구현하는 과정에서, thumbnailList처럼 외부에서 rate를 조회하여 넘겨주어야 할지, movieId를 받아서 직접 rate를 조회해야 할지 고민되었습니다. 현재는 movieId를 받는 방식으로 구현되어 있어요.
현업에서 bp를 어떻게 관리하는지 궁금합니다. 지금은 1440px과 640px을 임의로 설정하여 값으로 처리하고 있는데, lg-md-sm에 대한 상수를 관리하는 것이 일반적인가요? 단순 desktop, tablet, mobile로 나뉘지 않고 부가적인 bp가 생기는 경우에는 어떻게 처리하는지도 궁금합니다. 이전 프로젝트에서 bp가 4-5개씩 있었는데, 분기 지점을 명시적으로 선언하기 어렵다고 느꼈습니다.
✅ 리뷰어 체크 포인트