[2단계 - 상세 정보 & UI/UX 개선하기] 안톨리니 미션 제출합니다.#301
[2단계 - 상세 정보 & UI/UX 개선하기] 안톨리니 미션 제출합니다.#301Antoliny0919 wants to merge 42 commits intowoowacourse:antoliny0919from
Conversation
- main 로직 분리 - 에러처리 로직 세분화 - API 에러 메시지 테스트 케이스 추가
Walkthrough이 풀 리퀘스트는 영화 목록 및 상세정보 기능을 재구축하는 광범위한 리팩토링을 구성합니다. 주요 변경사항은 다음을 포함합니다: 페이지 계층 구조 도입(Index, Search, MovieDetail 모듈), 무한 스크롤 기반 페이지네이션(IntersectionObserver), 네이티브 dialog 요소를 사용한 영화 상세정보 모달, 장르 API 및 별점 로컬스토리지 지원, 렌더러 전문화(IndexRenderer, SearchRenderer, MovieDetailRenderer), 상태 관리 단순화, 반응형 CSS 추가 등입니다. Cypress 테스트 스위트도 새로운 구조를 반영하도록 확장되었습니다. Estimated code review effort🎯 4 (Complex) | ⏱️ ~75 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
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/render.ts (1)
42-46:⚠️ Potential issue | 🔴 Critical
innerHTML에 사용자 입력과 외부 데이터를 넣고 있습니다.검색어는 사용자 입력이고,
title/overview/genres는 외부 API 값입니다. 이 값을 그대로innerHTML에 넣으면 HTML이 실행 가능한 형태로 DOM에 삽입되어 XSS 경로가 생깁니다. 이 구간은 마크업이 필요하지 않으니textContent기반으로 바꾸는 편이 안전합니다.Also applies to: 81-87
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/render.ts` around lines 42 - 46, Replace unsafe DOM writes that use innerHTML with textContent to prevent XSS: in renderSearchSectionHeading(title: string) change heading.innerHTML = `"${title}"검색 결과` to assign a safe textContent variant, and likewise update the other occurrences mentioned (the block around lines 81-87 where title/overview/genres are written) to use textContent (or createTextNode) instead of innerHTML; if you truly need markup, explicitly sanitize/escape the values before inserting. Ensure you target the renderSearchSectionHeading function and the corresponding render function that writes title/overview/genres.
🧹 Nitpick comments (3)
.gitignore (1)
27-27: 파일 끝에 개행 문자를 추가하고 다른 Cypress 아티팩트도 고려하세요.현재 파일이 개행 문자 없이 끝나고 있습니다. POSIX 표준에 따라 텍스트 파일은 개행 문자로 끝나야 합니다. 또한
cypress/screenshots외에cypress/videos와cypress/downloads도 일반적으로 무시되는 Cypress 테스트 아티팩트입니다.📝 제안하는 수정사항
.env cypress/screenshots +cypress/videos +cypress/downloads +🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In @.gitignore at line 27, The .gitignore currently ends without a trailing newline and only includes "cypress/screenshots"; add a newline at EOF and include the other common Cypress artifacts by adding entries for "cypress/videos" and "cypress/downloads" (ensure each entry is on its own line and that "cypress/screenshots" remains separated by newlines so the file ends with a single final newline).src/styles/main.css (1)
35-42:button.primary는 고정 폭보다 유연 폭이 더 안전합니다.Line 40의 고정
width: 80px은 폰트 확대/문구 변경 시 텍스트 잘림 가능성이 있습니다.min-width+ 좌우 패딩 기반으로 두면 재사용성이 좋아집니다.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/styles/main.css` around lines 35 - 42, The button.primary rule uses a fixed width (width: 80px) which can cause text clipping; update the button.primary CSS to remove width: 80px and instead set a sensible min-width (e.g., min-width) and rely on horizontal padding (padding-left / padding-right) so the button grows with content; keep existing font-weight, background-color and border-radius, and ensure box-sizing/content sizing is preserved for correct padding behavior.cypress/e2e/spec.cy.ts (1)
336-340: 고정 시간 대기는 이 테스트를 불안정하게 만듭니다.Line 338의
cy.wait(500)는 CI 환경 속도에 따라 간헐 실패를 만들 수 있습니다. 여기서는 스크롤 뒤에 발생하는 추가 요청에 alias를 붙여 기다리거나, 추가 렌더링이 끝났음을 보여 주는 DOM 조건을 직접 기다리는 쪽이 더 안정적입니다.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@cypress/e2e/spec.cy.ts` around lines 336 - 340, Replace the fixed 500ms wait in the test "더 가져오기 이후에 존재했던 영화를 클릭시 자세한 정보가 담긴 모달이 렌더링된다." by waiting for a deterministic condition: either add and wait on a network alias for the request triggered by scrolling (use cy.intercept(...) and cy.wait(alias) after cy.get("#end-of-thumbnail-list").scrollIntoView()) or wait for a DOM change that indicates thumbnails loaded (e.g., assert the presence/length of ".thumbnail-list li" or that a specific new thumbnail exists) before clicking the first ".thumbnail-list li" and asserting ".modal" is visible.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@index.html`:
- Around line 57-60: Add accessible names: give the <dialog
id="modalBackground"> an aria-labelledby that points to its visible title
element (create an h2/h1 inside the modal with a unique id), ensure the close
button (id="closeModal") has an accessible name (either visible text or
aria-label like "Close dialog"), and for each star rating control (the star
buttons/inputs used to set rating) add clear aria-label attributes (e.g.,
aria-label="1 star", "2 stars", etc.). Treat the inner star/images as decorative
by removing verbose alt text and marking them aria-hidden="true" (or empty
alt="") so screen readers use the button aria-labels instead. Ensure the title
id matches the dialog's aria-labelledby and that each star control is focusable
and operable via keyboard.
In `@src/api.ts`:
- Around line 74-77: The code builds the search URL by interpolating query and
page into the template string (const url =
`${API_PATH.SEARCH_MOVIE}?query=${query}&page=${pageNum}&language=ko-KR`), which
breaks if query contains special characters; fix by encoding query (or build the
whole query string via URLSearchParams) before constructing the URL so
characters like &, =, ? and # are escaped—update the place where `url` is
composed (the search request in src/api.ts that calls handleResponseError and
response.json()) to use encodeURIComponent(query) or new URLSearchParams({
query, page: String(pageNum), language: 'ko-KR' }) to produce a safe query
string.
In `@src/component.ts`:
- Line 11: poster_path가 앞에 슬래시를 포함할 경우 IMAGE_PATH와 결합하여 중복 슬래시가 생기므로, src 생성부에서
poster_path 또는 IMAGE_PATH의 슬래시를 정규화하여 결합하세요; 예를 들어 poster_path의 선행 '/'를 제거하거나
IMAGE_PATH의 후행 '/'를 제거한 뒤 `${IMAGE_PATH}/${poster_path}`로 조합하도록 변경하고,
noImagePlanetImg 사용 분기는 그대로 유지하세요 (참조 심볼: src, poster_path, IMAGE_PATH,
noImagePlanetImg).
In `@src/pages/movieDetail.ts`:
- Around line 59-63: In extractDetailMovieData, avoid assuming genres.find(...)
always returns a value: when mapping movie.genre_ids to names (movie.genre_ids
and State.genres), guard against undefined from genres.find and provide a safe
fallback (e.g., "Unknown" or omit) instead of using the non-null assertion;
update the mapping logic inside extractDetailMovieData to check the find result
for truthiness and return a stable string array even when State.genres is empty
or contains no match.
In `@src/render.ts`:
- Around line 82-83: The code builds a URL even when poster_path can be null,
causing requests to "/null"; update the logic around moviePoster (and
IMAGE_PATH/poster_path) to handle poster_path === null by using a placeholder
image or hiding the element instead of concatenating "/null" — e.g., inside the
existing HTMLImageElement branch for moviePoster, check poster_path and if null
set moviePoster.src to a configured placeholder (or set
moviePoster.hidden/style.display) otherwise set moviePoster.src =
`${IMAGE_PATH}/${poster_path}`.
In `@src/styles/modal.css`:
- Around line 39-40: The modal currently uses a fixed width (width: 1000px)
which causes horizontal overflow on small viewports; change the CSS for the
modal container to use responsive sizing instead (e.g., replace the fixed width
with max-width: 1000px and width: 100% so it caps at 1000px but shrinks on
smaller screens) and adjust/add the existing 768px media query to ensure any
padding/margins or font sizes inside the modal (selector that contains width:
1000px) are reduced appropriately for tablet/mobile breakpoints.
- Around line 4-6: The CSS rule targets body.modal-open but the class is never
added when opening the dialog, only removed when closing; update the modal open
flow to add document.body.classList.add('modal-open') when calling
dialog.showModal() (e.g., in the function that opens the dialog in
movieDetail.ts) and ensure the existing close/remove logic still calls
document.body.classList.remove('modal-open') when the dialog closes so the
body.modal-open rule actually takes effect.
---
Outside diff comments:
In `@src/render.ts`:
- Around line 42-46: Replace unsafe DOM writes that use innerHTML with
textContent to prevent XSS: in renderSearchSectionHeading(title: string) change
heading.innerHTML = `"${title}"검색 결과` to assign a safe textContent variant, and
likewise update the other occurrences mentioned (the block around lines 81-87
where title/overview/genres are written) to use textContent (or createTextNode)
instead of innerHTML; if you truly need markup, explicitly sanitize/escape the
values before inserting. Ensure you target the renderSearchSectionHeading
function and the corresponding render function that writes
title/overview/genres.
---
Nitpick comments:
In @.gitignore:
- Line 27: The .gitignore currently ends without a trailing newline and only
includes "cypress/screenshots"; add a newline at EOF and include the other
common Cypress artifacts by adding entries for "cypress/videos" and
"cypress/downloads" (ensure each entry is on its own line and that
"cypress/screenshots" remains separated by newlines so the file ends with a
single final newline).
In `@cypress/e2e/spec.cy.ts`:
- Around line 336-340: Replace the fixed 500ms wait in the test "더 가져오기 이후에 존재했던
영화를 클릭시 자세한 정보가 담긴 모달이 렌더링된다." by waiting for a deterministic condition: either
add and wait on a network alias for the request triggered by scrolling (use
cy.intercept(...) and cy.wait(alias) after
cy.get("#end-of-thumbnail-list").scrollIntoView()) or wait for a DOM change that
indicates thumbnails loaded (e.g., assert the presence/length of
".thumbnail-list li" or that a specific new thumbnail exists) before clicking
the first ".thumbnail-list li" and asserting ".modal" is visible.
In `@src/styles/main.css`:
- Around line 35-42: The button.primary rule uses a fixed width (width: 80px)
which can cause text clipping; update the button.primary CSS to remove width:
80px and instead set a sensible min-width (e.g., min-width) and rely on
horizontal padding (padding-left / padding-right) so the button grows with
content; keep existing font-weight, background-color and border-radius, and
ensure box-sizing/content sizing is preserved for correct padding behavior.
🪄 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: 13447cc1-3f2f-4f80-884c-c93926fb55c8
⛔ Files ignored due to path filters (1)
src/images/modal_button_close.pngis excluded by!**/*.png
📒 Files selected for processing (18)
.gitignoreREADME.mdcypress/e2e/spec.cy.tscypress/fixtures/genres.jsonindex.htmlsrc/api.tssrc/component.tssrc/constans/movie.tssrc/constants/movie.tssrc/main.tssrc/pages/index.tssrc/pages/movieDetail.tssrc/pages/search.tssrc/render.tssrc/state.tssrc/styles/main.csssrc/styles/modal.csssrc/styles/responsive.css
💤 Files with no reviewable changes (1)
- src/constans/movie.ts
| <dialog class="modal-background" id="modalBackground"> | ||
| <div class="modal"> | ||
| <button class="close-modal" id="closeModal" autofocus> | ||
| <img src="src/images/modal_button_close.png" alt="close modal"/> |
There was a problem hiding this comment.
모달과 별점 컨트롤에 접근 가능한 이름이 없습니다.
현재 <dialog>는 제목과 연결된 접근성 이름이 없고, 별점 버튼도 이미지 외에 읽을 수 있는 텍스트가 없어 스크린리더에서는 용도를 알기 어렵습니다. 이 상태면 모달의 맥락 파악과 별점 입력이 사실상 막히니, 모달은 제목과 연결하고 각 별점 버튼은 aria-label 같은 이름을 가져가도록 정리해 주세요. 내부 별 이미지는 장식용으로 처리하는 편이 안전합니다.
Also applies to: 87-94
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@index.html` around lines 57 - 60, Add accessible names: give the <dialog
id="modalBackground"> an aria-labelledby that points to its visible title
element (create an h2/h1 inside the modal with a unique id), ensure the close
button (id="closeModal") has an accessible name (either visible text or
aria-label like "Close dialog"), and for each star rating control (the star
buttons/inputs used to set rating) add clear aria-label attributes (e.g.,
aria-label="1 star", "2 stars", etc.). Treat the inner star/images as decorative
by removing verbose alt text and marking them aria-hidden="true" (or empty
alt="") so screen readers use the button aria-labels instead. Ensure the title
id matches the dialog's aria-labelledby and that each star control is focusable
and operable via keyboard.
| const url = `${API_PATH.SEARCH_MOVIE}?query=${query}&page=${pageNum}&language=ko-KR`; | ||
| const options = { | ||
| method: "GET", | ||
| headers: { | ||
| accept: "application/json", | ||
| Authorization: `Bearer ${API_KEY}`, | ||
| }, | ||
| }; | ||
| const response = await fetch(url, options); | ||
| if (!response.ok) throw new Error(`HTTP error! status: ${response.status}`); | ||
| const response = await fetch(url, defaultOptions); | ||
| handleResponseError(response); | ||
| return response.json(); |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
# First, let's find and read the src/api.ts file
head -100 src/api.ts | tail -50Repository: woowacourse/javascript-movie-review
Length of output: 1439
🏁 Script executed:
# Let's also check the exact lines 74-77
sed -n '70,80p' src/api.tsRepository: woowacourse/javascript-movie-review
Length of output: 451
🏁 Script executed:
# Let's see the full context of this function
sed -n '60,90p' src/api.tsRepository: woowacourse/javascript-movie-review
Length of output: 893
쿼리 파라미터를 URL 인코딩해야 합니다.
query를 템플릿 문자열에 직접 넣으면 &, =, ?, # 같은 특수문자가 포함될 경우 URL 파싱이 의도대로 동작하지 않을 수 있습니다. 예를 들어 사용자가 "a&page=10"을 검색하면 예상과 다른 쿼리스트링이 생성됩니다.
이 문제를 해결하기 위해 어떤 방법을 사용할 수 있을지 생각해보세요. JavaScript에서 URL의 쿼리 파라미터를 안전하게 인코딩하는 표준적인 방법이 있습니다. 문서를 참고하거나 URLSearchParams과 encodeURIComponent 같은 내장 도구를 조사해보시기 바랍니다.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/api.ts` around lines 74 - 77, The code builds the search URL by
interpolating query and page into the template string (const url =
`${API_PATH.SEARCH_MOVIE}?query=${query}&page=${pageNum}&language=ko-KR`), which
breaks if query contains special characters; fix by encoding query (or build the
whole query string via URLSearchParams) before constructing the URL so
characters like &, =, ? and # are escaped—update the place where `url` is
composed (the search request in src/api.ts that calls handleResponseError and
response.json()) to use encodeURIComponent(query) or new URLSearchParams({
query, page: String(pageNum), language: 'ko-KR' }) to produce a safe query
string.
| const src = poster_path | ||
| ? `${IMAGE_PATH}/${poster_path}` | ||
| : noImagePlanetImg; | ||
| const src = poster_path ? `${IMAGE_PATH}/${poster_path}` : noImagePlanetImg; |
There was a problem hiding this comment.
이미지 URL 결합 시 슬래시 정규화를 한 번 점검해 주세요.
Line 11은 poster_path가 /로 시작할 때 .../original//... 형태가 됩니다. 브라우저에서 열리더라도 캐시 키가 분산될 수 있어, 한쪽 슬래시를 정규화하는 방식이 더 안전합니다.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/component.ts` at line 11, poster_path가 앞에 슬래시를 포함할 경우 IMAGE_PATH와 결합하여 중복
슬래시가 생기므로, src 생성부에서 poster_path 또는 IMAGE_PATH의 슬래시를 정규화하여 결합하세요; 예를 들어
poster_path의 선행 '/'를 제거하거나 IMAGE_PATH의 후행 '/'를 제거한 뒤
`${IMAGE_PATH}/${poster_path}`로 조합하도록 변경하고, noImagePlanetImg 사용 분기는 그대로 유지하세요
(참조 심볼: src, poster_path, IMAGE_PATH, noImagePlanetImg).
| extractDetailMovieData(movie: Movie): [string[], number] { | ||
| const genres = State.genres; | ||
| const movieGenres = movie.genre_ids.map( | ||
| (genreId) => genres.find((genre) => genre.id === genreId)!.name, | ||
| ); |
There was a problem hiding this comment.
장르 매핑 실패 시 모달이 바로 깨질 수 있습니다.
여기서는 find() 결과를 바로 사용하고 있어서 장르 목록이 아직 비어 있거나, API가 예상 밖 genre_id를 주는 순간 런타임 에러가 납니다. 특히 초기 장르 요청이 실패한 뒤 검색 결과에서 모달을 열면 이 경로로 바로 터질 수 있으니, 매핑 실패 시의 fallback을 두는 편이 안전합니다.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/pages/movieDetail.ts` around lines 59 - 63, In extractDetailMovieData,
avoid assuming genres.find(...) always returns a value: when mapping
movie.genre_ids to names (movie.genre_ids and State.genres), guard against
undefined from genres.find and provide a safe fallback (e.g., "Unknown" or omit)
instead of using the non-null assertion; update the mapping logic inside
extractDetailMovieData to check the find result for truthiness and return a
stable string array even when State.genres is empty or contains no match.
There was a problem hiding this comment.
코드래빗 피드백 동의
genres에 해당 id가 없으면 런타임 에러가 날 것 같은데요. 보장이 되는 게 맞을까요? 만약 에러가 날 수 잇다면 이 경우를 어떻게 처리하면 좋을까요?
| if (moviePoster instanceof HTMLImageElement) | ||
| moviePoster.src = `${IMAGE_PATH}/${poster_path}`; |
There was a problem hiding this comment.
포스터가 없는 영화에서 깨진 이미지 요청이 나갑니다.
poster_path 타입이 string | null인데 현재는 그대로 URL을 조합하고 있어서 값이 없으면 /null 이미지를 요청하게 됩니다. 포스터가 없는 경우의 placeholder나 이미지 숨김 fallback을 같이 두는 편이 좋겠습니다.
🧰 Tools
🪛 ast-grep (0.42.1)
[warning] 83-83: Direct modification of innerHTML or outerHTML properties detected. Modifying these properties with unsanitized user input can lead to XSS vulnerabilities. Use safe alternatives or sanitize content first.
Context: movieVoteAverage.innerHTML = vote_average.toFixed(1)
Note: [CWE-79] Improper Neutralization of Input During Web Page Generation [REFERENCES]
- https://owasp.org/www-community/xss-filter-evasion-cheatsheet
- https://cwe.mitre.org/data/definitions/79.html
(dom-content-modification)
[warning] 83-83: Direct HTML content assignment detected. Modifying innerHTML, outerHTML, or using document.write with unsanitized content can lead to XSS vulnerabilities. Use secure alternatives like textContent or sanitize HTML with libraries like DOMPurify.
Context: movieVoteAverage.innerHTML = vote_average.toFixed(1)
Note: [CWE-79] Improper Neutralization of Input During Web Page Generation [REFERENCES]
- https://www.dhairyashah.dev/posts/why-innerhtml-is-a-bad-idea-and-how-to-avoid-it/
- https://cwe.mitre.org/data/definitions/79.html
(unsafe-html-content-assignment)
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/render.ts` around lines 82 - 83, The code builds a URL even when
poster_path can be null, causing requests to "/null"; update the logic around
moviePoster (and IMAGE_PATH/poster_path) to handle poster_path === null by using
a placeholder image or hiding the element instead of concatenating "/null" —
e.g., inside the existing HTMLImageElement branch for moviePoster, check
poster_path and if null set moviePoster.src to a configured placeholder (or set
moviePoster.hidden/style.display) otherwise set moviePoster.src =
`${IMAGE_PATH}/${poster_path}`.
| body.modal-open { | ||
| overflow: hidden; | ||
| } |
There was a problem hiding this comment.
배경 스크롤 잠금 규칙이 실제로 활성화되지 않습니다.
Line 4-6의 규칙은 body.modal-open 클래스가 추가될 때만 동작하는데, 현재 열기 경로(src/pages/movieDetail.ts의 dialog.showModal())에서는 add가 없고 닫기 경로에서 remove만 수행됩니다(src/render.ts Line 110-114). 이 상태면 “모달 열림 시 외부 스크롤 방지” 요구사항이 깨질 수 있습니다.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/styles/modal.css` around lines 4 - 6, The CSS rule targets
body.modal-open but the class is never added when opening the dialog, only
removed when closing; update the modal open flow to add
document.body.classList.add('modal-open') when calling dialog.showModal() (e.g.,
in the function that opens the dialog in movieDetail.ts) and ensure the existing
close/remove logic still calls document.body.classList.remove('modal-open') when
the dialog closes so the body.modal-open rule actually takes effect.
| width: 1000px; | ||
| } |
There was a problem hiding this comment.
모달 고정 폭 1000px로 작은 뷰포트에서 가로 깨짐이 발생할 수 있습니다.
Line 39의 width: 1000px은 태블릿/모바일에서 오버플로우를 유발할 가능성이 큽니다. 현재 반응형 파일에서도(768px 구간) 폭 자체는 줄이지 않아 문제가 남습니다.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/styles/modal.css` around lines 39 - 40, The modal currently uses a fixed
width (width: 1000px) which causes horizontal overflow on small viewports;
change the CSS for the modal container to use responsive sizing instead (e.g.,
replace the fixed width with max-width: 1000px and width: 100% so it caps at
1000px but shrinks on smaller screens) and adjust/add the existing 768px media
query to ensure any padding/margins or font sizes inside the modal (selector
that contains width: 1000px) are reduced appropriately for tablet/mobile
breakpoints.
Antoliny0919
changed the title
cys4585
left a comment
cys4585
left a comment
There was a problem hiding this comment.
안녕하세요 안톨리니!
2단계 미션도 고생하셨습니다.
동작 확인
배포 링크에서 동작 확인했어요. 전반적으로 잘 동작하고, 무한 스크롤, 모달, 별점, 반응형까지 잘 구현되어 있네요!
몇 가지 이슈가 있었어요:
- 네트워크를 끄고 스크롤하면 같은 페이지를 계속 요청해요. 원인이 뭔지 한번 디버깅해보면 좋겠어요!
- 모달이 열린 상태에서 배경이 스크롤돼요.
설계
main.ts를 줄이고 pages/ 디렉토리로 기능별 분리를 한 것 좋은 것 같습니다~! 각각의 역할이 깔끔하게 분리되어 있고, pages가 기능의 진입점으로서 이 도구들을 호출하는 구조가 읽기 좋았어요.
한 가지 개선하면 좋을 것 같다 싶은 건, 이벤트 핸들러(showPopularMovies 등)에 API 호출 + State 업데이트 + 렌더링 호출이 한 함수에 섞여 있어요. 이벤트 핸들러가 "비즈니스 로직 함수 호출 → 렌더링 함수 호출"만 하는 구조가 되면, showMoreMovies와 showMoreSearchMovies의 중복 패턴도 공통화할 수 있을 것 같고요. render.ts를 별도 모듈로 분리한 이유와 같은 원칙으로 볼 수 있을 것 같습니다. "DOM 조작은 render.ts의 관심사"인 것처럼, "데이터를 가져와서 상태를 업데이트하는 건 비즈니스 로직 모듈의 관심사"라고 볼 수 있지 않을까요?
안톨리니의 설계 의도를 설명해주는 것도 좋을 것 같습니다~!
index.ts의 이중 역할
제가 보기엔 index.ts가 두 가지 역할을 동시에 하고 있는 걸로 봤어요.
- init()에서 모든 이벤트 리스너를 셋업하고, MovieDetail.setUpEventListeners()도 호출하고, Search도 연결 — 이건 앱 전체를 부트스트랩하는 역할 (search/movieDetail보다 상위)
- showPopularMovies(), showMoreMovies() — 이건 홈 화면의 이벤트 핸들러 (search.ts와 같은 레이어)
이 두 역할이 한 파일에 있어서 index.ts가 "상위 레이어이면서 동시에 같은 레이어"처럼 보여요. init() 부분을 main.ts나 별도 app.ts로 옮기면 세 파일(index, search, movieDetail)이 같은 레이어에 놓이게 될 것 같은데, 어떻게 생각하시나요?
localStorage 접근
다른 데이터(영화, 장르)는 api.ts를 통해 가져오는데, 별점만 movieDetail에서 직접 localStorage에 접근하고 있는데요. 미션 요구사항에 "서버 API로 교체할 수 있는 구조"가 있는데, 지금 구조에서 서버 API로 교체하려면 movieDetail.ts를 직접 수정해야 할 것 같아요.
요 요구사항 관련해서 안톨리니의 의도와 생각이 궁금합니다!
PR 본문 질문 답변
1. 이벤트 위임
"구조 변경이 크다"고 느꼈다고 하셨는데, 어떤 부분 때문일까요? ul로 위임하고 id 넘겨주면 되지 않나요..? 그리고 setUpMovieDetail을 매번 호출하는 코드가 사라져서 단순해지는 부분도 있을 것 같네요.
2. IntersectionObserver 학습 깊이
사용하고 있는 범위 내에서 학습하면 된다고 생각합니다. 지금 코드에서 threshold: 0.8을 쓰고 있는데, 그럼 이 threshold가 뭔지, 이 값을 선택한 이유를 설명할 수 있으면 충분하다고 봐요.
3. 레벨2 준비
레벨1에서 배운 걸 흡수하는 게 중요할 것 같습니다. 개인적으로는 관심사 분리하는거? 그리고 내가 만든 미션의 설계를 그려보는거? 를 추천드리고 싶네요. 그리고 어떤 설계가 좋을지 고민해보고 리팩토링해보는 거?
근데 뭐... 계획한게 있으시다면 계획한대로 가시죠!
4. AI 에이전트 활용
AI 에이전트를 어떻게 활용하는지가 중요할 것 같은데요.. 자칫 잘못하면 생각하는 걸 위임해버릴 수 있어서.. 그걸 주의해야할 것 같습니다.
5. 설계 고민
저도 2단계 미션은 설계에 집중해서 봤는데요, 깊이 고민하지 않았다고 생각이 들진 않았어요. 얼추 이런 구조로 봤어요.
┌─────────┐
│ main.ts │ (진입점, 1줄)
└────┬────┘
│
┌────────▼────────┐
│ pages/*.ts │ ← 이벤트 수신 → 상태 업데이트
│ (index/search/ │ → UI 반영을
│ movieDetail) │ 한 함수 안에서 순서대로 실행
└──┬─────┬─────┬─┘
│ │ │
┌────────▼┐ ┌──▼──┐ ┌▼────────┐
│ api.ts │ │State│ │render.ts │
└─────────┘ └─────┘ └────┬─────┘
│
┌──────▼──────┐
│component.ts │
└─────────────┘
기능 관심사 단위로 짜른 부분이 인상적이었어요. 여기에 페이지가 추가되어도 다른 곳엔 영향을 안줄 것 같다는 장점이 있겠다 싶었어요. UI/비즈니스 로직도 잘 분리했고요.
조금 더 디벨롭하면 좋겠다 싶은 걸 중점적으로 코멘트를 남겼습니다(역할의 모호함이 있는 모듈, 비즈니스 레이어 하나 더 분리). 요걸 한번 고민해보면 좋을 것 같아요.
아 그리고 localStorage 추상화도 한번 고민해보면 좋겠습니다.
태도는... 음.. 소프트웨어는 어찌됐든 돌아가게 만들 수 있잖아요? 이게 장점이자 단점이 될 수 있다고 생각해요. 지금 당장 돌아가는 코드를 만드는 건 쉬우니까요. 근데 그 소프트웨어의 수명을 늘리고, 유지보수 가능하게 만들려면 설계를 잘 해야겠죠. 그래서 그런 관점으로 학습할 포인트를 찾는다는 마인드로 접근해보면 좋을 것 같습니다. 그냥 남들이 하니까 따라하는거 말고, 좋다고 하니까 하는거 말고, 이 설계 위에서 기능 확장할 때 어떻게 될까? 같은 생각을 시작으로 바라보면 좋을 것 같아요.
| import { Index } from "./pages/index.ts"; | ||
|
|
||
| async function loadInitialMovie() { | ||
| const app = document.querySelector("#app"); | ||
| if (app) { | ||
| Renderer.renderSkeleton( | ||
| ".thumbnail-list", | ||
| State.getRequestMovieCount() || ONCE_MOVIE_LIMIT, | ||
| ); | ||
| try { | ||
| const { results: movies, page } = | ||
| await getPopularMovies(INITIAL_PAGE_NUM); | ||
| State.setNextPageNum(page + 1); | ||
| State.setRequestMovieCount(movies.length); | ||
| MovieRenderer.renderInitialMovies(movies); | ||
| const loadMoreButton = document.querySelector(".load-more-button"); | ||
| if (loadMoreButton) | ||
| loadMoreButton.addEventListener("click", loadMoreMovies); | ||
| } catch (err) { | ||
| MovieRenderer.renderError(); | ||
| } | ||
| } | ||
| } | ||
|
|
||
| async function loadMoreMovies() { | ||
| Renderer.renderSkeleton(".thumbnail-list", State.getRequestMovieCount()); | ||
| Renderer.hideLoadMoreButton(); | ||
| try { | ||
| const { results: movies, page } = await getPopularMovies( | ||
| State.getNextPageNum(), | ||
| ); | ||
| State.setNextPageNum(page + 1); | ||
| MovieRenderer.renderLoadMoreMovies(movies); | ||
| } catch (err) { | ||
| MovieRenderer.renderError(); | ||
| Renderer.clearBanner(); | ||
| } | ||
| } | ||
|
|
||
| async function loadSearchMovies(query: string) { | ||
| Renderer.renderSkeleton(".thumbnail-list", State.getRequestMovieCount()); | ||
| Renderer.hideLoadMoreButton(); | ||
| try { | ||
| const { results: movies, page } = await getSearchMovies( | ||
| query, | ||
| INITIAL_PAGE_NUM, | ||
| ); | ||
| const loadMoreButton = document.querySelector(".load-more-button"); | ||
| MovieRenderer.renderSearchResult(movies, query); | ||
| State.setNextSearchPageNum(page + 1); | ||
| if (loadMoreButton) { | ||
| loadMoreButton.removeEventListener("click", loadMoreMovies); | ||
| loadMoreButton.addEventListener("click", () => | ||
| loadMoreSearchMovies(query), | ||
| ); | ||
| } | ||
| } catch (err) { | ||
| MovieRenderer.renderError(); | ||
| } | ||
| } | ||
|
|
||
| async function loadMoreSearchMovies(query: string) { | ||
| Renderer.renderSkeleton(".thumbnail-list", State.getRequestMovieCount()); | ||
| Renderer.hideLoadMoreButton(); | ||
| try { | ||
| const { results: movies, page } = await getSearchMovies( | ||
| query, | ||
| State.getNextSearchPageNum(), | ||
| ); | ||
| State.setNextSearchPageNum(page + 1); | ||
| MovieRenderer.renderLoadMoreSearchMovies(movies); | ||
| } catch (err) { | ||
| MovieRenderer.renderError(); | ||
| } | ||
| } | ||
|
|
||
| const searchForm = document.querySelector(".search-form"); | ||
| searchForm?.addEventListener("submit", (event) => { | ||
| event.preventDefault(); | ||
| const input = searchForm.querySelector("input"); | ||
| if (input) { | ||
| const searchValue = input.value; | ||
| loadSearchMovies(searchValue); | ||
| } | ||
| }); | ||
|
|
||
| addEventListener("load", loadInitialMovie); | ||
| Index.init(); |
| init() { | ||
| this.setUpInitialContent(); | ||
| this.setUpEventListeners(); | ||
| }, |
There was a problem hiding this comment.
index.ts가 init()에서 앱 전체를 부트스트랩하면서(MovieDetail.setUpEventListeners(), setUpSearchForm() 등), 동시에 showPopularMovies()로 홈 화면 기능도 담당하는 걸로 보이네요. 그래서 그런지 index.ts가 search.ts, movieDetail.ts보다 상위 레이어인 것 같기도 하고, 같은 레이어인 것 같기도 해요. init() 부분을 main.ts로 옮기면 세 파일이 같은 레이어에 놓이게 될 것 같은데, 어떻게 생각하나요?
| async showPopularMovies() { | ||
| const app = document.querySelector("#app"); | ||
| if (app) { | ||
| State.isLoading = true; | ||
| Renderer.renderSkeleton( | ||
| ".thumbnail-list", | ||
| State.requestMovieCount || ONCE_MOVIE_LIMIT, | ||
| ); | ||
| try { | ||
| const [{ results: movies, page, total_pages }, { genres }] = | ||
| await Promise.all([getPopularMovies(INITIAL_PAGE_NUM), getGenres()]); | ||
| State.nextPageNum = page + 1; | ||
| State.totalPages = total_pages; | ||
| State.requestMovieCount = movies.length; | ||
| State.genres = genres; | ||
| IndexRenderer.renderInitialMovies(movies); | ||
| MovieDetail.setUpMovieDetail(movies); | ||
| } catch (err) { | ||
| Renderer.renderError(err); | ||
| } finally { | ||
| State.isLoading = false; | ||
| } | ||
| } | ||
| }, |
There was a problem hiding this comment.
API 호출 + State 업데이트 + 렌더링이 한 함수에 있는데, 비즈니스 로직(API + State)을 별도 함수로 분리하면 어떨까요? 안톨리니 생각이 궁금합니다~
| async showMoreMovies() { | ||
| State.isLoading = true; | ||
| Renderer.renderSkeleton(".thumbnail-list", State.requestMovieCount); | ||
| try { | ||
| const { | ||
| results: movies, | ||
| page, | ||
| total_pages, | ||
| } = await getPopularMovies(State.nextPageNum); | ||
| State.nextPageNum = page + 1; | ||
| State.totalPages = total_pages; | ||
| Renderer.renderLoadMoreMovies(movies); | ||
| MovieDetail.setUpMovieDetail(movies); | ||
| } catch (err) { | ||
| Renderer.renderError(err); | ||
| Renderer.clearBanner(); | ||
| } finally { | ||
| State.isLoading = false; | ||
| } | ||
| }, |
There was a problem hiding this comment.
search.ts의 showMoreSearchMovies()와 거의 같은 패턴이네요. isLoading → skeleton → API → State 업데이트 → render → setUpMovieDetail. 이 중복은 그대로 두는게 좋을지, 추출하는 게 좋을지.. 안톨리니 생각이 궁금합니다!
| getMyRating(key: string) { | ||
| return Number(localStorage.getItem(key)); | ||
| }, | ||
|
|
||
| setMyRating(key: string, rating: string) { | ||
| localStorage.setItem(key, rating); | ||
| }, |
There was a problem hiding this comment.
미션 요구사항에 "localStorage, 서버 API를 쉽고 안전하게 갈아끼울 수 있는 구조"가 있는데요. 지금은 movieDetail이 localStorage를 직접 알고 있어서, 서버 API로 교체하려면 이 파일을 직접 수정해야할 것 같아요. movieDetail이 저장소의 구현 방식을 몰라도 되게 만들 수 있을까요? 어떤 구조가 되면 "갈아끼울 수 있는" 상태가 될지 생각해보면 좋겠어요!
| const [{ results: movies, page, total_pages }, { genres }] = | ||
| await Promise.all([getPopularMovies(INITIAL_PAGE_NUM), getGenres()]); |
There was a problem hiding this comment.
여기서 Promise.all을 사용한 이유가 궁금해요! 순차적으로 호출하는 것과는 어떤 차이가 있을까요? 그리고 Promise.all에서 하나가 실패하면 어떻게 되는지도 궁금합니다~
There was a problem hiding this comment.
MovieRenderer에서 IndexRenderer, SearchRenderer, MovieDetailRenderer, Renderer로 역할별로 잘 나눴네요! 👍
| const defaultOptions = { | ||
| method: "GET", | ||
| headers: { | ||
| accept: "application/json", | ||
| Authorization: `Bearer ${API_KEY}`, | ||
| }, | ||
| }; |
There was a problem hiding this comment.
defaultOptions로 공통 헤더를 공유하는 거 깔끔하네요! 👍
| function handleResponseError(response: Response) { | ||
| if (!response.ok) { | ||
| if (response.status === 401) { | ||
| throw new Error("인증에 실패했습니다. API 키를 확인해주세요."); | ||
| } | ||
| if (response.status === 404) { | ||
| throw new Error("요청한 정보를 찾을 수 없습니다"); | ||
| } | ||
| if (response.status >= 500) { | ||
| throw new Error("오류가 발생했습니다. 다시 시도해주세요."); | ||
| } | ||
| } | ||
| } |
There was a problem hiding this comment.
몇 가지 빠진 시나리오가 있는 것 같아요. "유저가 이 앱에서 할 수 있는 모든 행동"을 기준으로 테스트가 있는지 점검해보면 좋을 것 같습니다!
2 participants
🎯 미션 소개
🕵️ 셀프 리뷰(Self-Review)
제출 전 체크 리스트
리뷰 요청 & 논의하고 싶은 내용
1) 이번 단계에서 가장 많이 고민했던 문제와 해결 과정에서 배운 점
영화 상세정보가 담긴 모달을 만들때 많은 고민을 했다.
처음에는 기존 방식처럼 Component를 만들고 해당 컴포넌트 전체를 렌더링하는 형식으로 구현했다.
영화를 클릭하면 매번 컴포넌트가 렌더링되고 닫을때 제거되는 식이다.
하지만 이 방식으로 구현했을때 문제점이 많았다.
일단 이벤트리스너와 관련된 로직을 분리할 수 없었다.
이벤트리스너외에도 많은 로직들이 결합될 수 밖에 없는 상태가 되었다.
그래서 뼈대 HTML을 두고 변경할 부분만 매번 바꾸는 식으로 했더니 원하는대로 로직을 분리할 수 있었다.
영화가 로드될때마다 각 영화에 이벤트리스너를 등록하여 영화 상세정보가 담긴 모달이 열릴 수 있도록 했다.
이 과정에서 영화를 더 가져왔을때 기존 렌더링 방식을 유지했다.
기존 렌더링 방식은
innerHTML에 추가된 영화를 덧붙이는 식(+=)이었다.innerHTML을 덧붙이는(+=) 방식을 사용했을때 렌더링되는건 문제없었지만 전체를 다시 파싱해서 DOM을 재생성하기에 이전 포스트의 이벤트리스너가 초기화되는 현상이 존재했다.innerHTML을 사용했을때 새로운 문자를 덧붙였을때 전체 DOM을 재생성 한다는걸 처음 알았다.안그래도
innerHTML이 XSS공격에 취약하기에 어느정도는 지양하는 태도를 가져야 할거 같다.2) 이번 리뷰를 통해 논의하고 싶은 부분
안녕하세요 수야 ! 잘 지내고 있으신가요 ? 갑자기 날씨가 많이 따뜻해졌어요 :-)
개인적으로 Step2는 어려우면서도 간단했던거 같아요 !
이번에도 잘 부탁 드립니다 🔥
가져온 영화(li)에 각각 EventListener를 등록하고 있는형태입니다. Ul에 이벤트를 하나만 등록하여 이벤트 위임 형식으로 구현하는게 더 나았을까요? 사실 이벤트 위임 형태로 가면 당연히 더 효율적이라고 느껴지지만 현재 구조에서 많은 부분을 수정해야할것처럼 느껴지긴 합니다. 그중에서 Movie를 어딘가(state)에 저장한 다음에 가져오는 방식으로 수정해야 할거 같아서 수정하지 못했던거 같아요. 이런 변경이 현재 설계하고는 조금 거리가 멀어지는거 같아서 마음에 안들었습니다. 수야라면 어떻게 했을거 같나요 ?
IntersectionObserver를 기존에 몇번 사용해봐서 이런게 있고 어떨때 사용해야하지에 대한 정도는 알고있습니다. 정확한 인터페이스에 대한 이해는 없는데 수야는 이런 기능에 대해서 공부할때 어느정도 수준으로 파악하나요? 물론 딥다이브가 필요하냐?에 따라 다르다는 생각이 드는데intersectionObserver같은 경우는 그 정도는 아니라고 생각됩니다.(제가 잘못 생각하고 있는걸까요?) 근데.. 또 생각해보면intersectionObserver가 프론트엔드에겐 많이 사용되어서 정확히 알아야될거 같기도 하고요 🤔레벨1이 거의 끝났습니다 🙌 열심히 달려온거 같아요 ! 수야가 지금 저와 같은 상황에 있는 사람이라고 했을때, 즉 레벨2 시작하기전에 어떤걸 하면 좋을까요? 레벨1에서 배웠던 것들에 대한 회고? 레벨2에서 배울것들을 미리 예습? 참고로 저는 React를 사용해본 경험이 거의 없습니다. 방학때 어떤 학습을 하면 좋을지 고민이 되네요 ! 물론 어느정도 생각이 있고 예정되있는게 있지만 또... 제가 생각하지 못한 좋은게 있을수도 있으니까요☺️
중간에 미션을 진행하면서 코드에 대한 설계, 방향을 피드백 받을 수 있는 AI에이전트를 만들면 좋겠다는 생각이 들었습니다. 이 AI 에이전트를 만들어낼때 본인이 생각하는 중요한 설계 철학이 들어가야할거 같아요. 혹시 이런 에이전트를 사용하여 적극적으로 검토 받는 방식에 대해서 어떻게 생각하시나요? 학습단계에서 괜찮을까요? 수야가 만약 이러한 에이전트를 만든다고 했을때 AI 에이전트에게 어떤 설계철학을 부여할거 같나요 ?
마지막으로 .. 미션을 할때마다 내가 설계에 대해서 깊이 있게 생각하지 않은건가? 라는 생각이 많이 듭니다. 아마도 레벨1에서 겪은 가장 큰 고민인거 같네요. 오픈소스에서 겪었던 경험으로 어느정도 제가 코드를 작성할때 어떤 철학을 가지고 작성해야할지 자리잡았다고 생각했습니다. 이전에 너무나도 설계에 대한 생각이 많았어서 차마 진행조차도 못했던적이 많았거든요. 좋은 능력을 가진 분들이 작성하는 방식을 보면서 러프하지만 빈틈없는 정말 아름다운 코드 작성방식을 배웠던거 같습니다. 보면서 따라가려고 하지만 어렵더라구요 ㅎ_ㅎ 노력중입니다. 결국 코드를 작성하는것도 MBTI처럼 형태가 나눠질뿐 정답은 없다는 생각이 듭니다. 레거시 상태에서 유지보수할때는 이미 어느정도 틀이 있기에 답이 있을 수 있지만 지금처럼 1부터 구현할때는 조금 본인의 성향이 많이 드러나는거 같더라구요. 또 다른 생각은 DX 툴과 웹 프로젝트를 만들때 다른 마인드를 가져야 하나? 라는 생각도 듭니다. 어쩌면 똑같이 코드를 작성하는건데도요 ! 또 남들과 비교하면 안되지만 크루원들이 설계와 관련해서 정말 많이 고민하는 모습들을 보며 난 너무 고민안하는거 아닌가? 라는 생각이 들때도 많습니다. 지금 당장 제가(학습하는 단계에서) 어떠한 태도를 가지면 좋을까요? 마치 고민상담처럼 질문이 조금 길어지고 어려워졌네요 수야 :) 편하게 답해주세요 !!
✅ 리뷰어 체크 포인트