[2단계 - 상세 정보 & UI/UX 개선하기] 윤돌 미션 제출합니다.#319
Open
yundol777 wants to merge 59 commits intowoowacourse:yundol777from
Open
[2단계 - 상세 정보 & UI/UX 개선하기] 윤돌 미션 제출합니다.#319yundol777 wants to merge 59 commits intowoowacourse:yundol777from
yundol777 wants to merge 59 commits intowoowacourse:yundol777from
Conversation
Walkthrough이 변경 사항은 영화 목록 로딩 방식을 클릭 기반의 "더보기 버튼"에서 무한 스크롤 기능으로 전환하고, 영화 상세 정보를 모달 UI로 표시하는 기능을 추가합니다. 사용자 별점 저장 기능을 로컬스토리지에 구현하며, 반응형 디자인을 PC/태블릿/모바일 화면에 맞춰 업데이트합니다. 관련 컨트롤러, 뷰 클래스, API 계층, 타입 정의 및 테스트 코드가 함께 수정되었습니다. 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 |
yundol777
changed the title
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 10
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
src/view/movieListView.ts (1)
28-35:⚠️ Potential issue | 🟠 MajorHTML 속성값에 따옴표 누락 - XSS 및 파싱 오류 가능성
id,src,alt속성에 따옴표가 없습니다. 영화 제목에 공백이나 특수문자(예:",>,&)가 포함된 경우 HTML 파싱이 깨지거나 XSS 취약점이 발생할 수 있습니다.어떤 속성들이 영향을 받을 수 있는지, 그리고 사용자 입력이 포함된 데이터를 HTML에 삽입할 때 어떤 점을 고려해야 하는지 생각해 보시겠어요?
💡 힌트
- <li id=${item.id} class="thumbnail-item"> + <li id="${item.id}" class="thumbnail-item"> <div class="item"> <img class="thumbnail" - src=${thumbnailImage + item.poster_path} - alt=${item.title} + src="${thumbnailImage + item.poster_path}" + alt="${item.title}"추가로
item.title에 대해 HTML 이스케이프 처리도 고려해 보세요.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/view/movieListView.ts` around lines 28 - 35, The template in movieListView.ts builds HTML with unquoted attributes (id, src, alt, onerror) and unescaped item values, which can break parsing or open XSS; fix by wrapping attribute values in quotes and escaping user-derived strings: ensure id, poster_path, title, and noImage are inserted as quoted attribute values (e.g., id="${...}", src="${...}", alt="${...}", onerror="${...}") and introduce/ reuse an HTML-escape helper (escapeHtml or escapeAttr) to sanitize item.title and any attribute content before interpolation; also sanitize item.id (or enforce a safe numeric/string id) and the onerror handler string to avoid injecting raw user content.src/styles/modal.css (1)
69-80:⚠️ Potential issue | 🟡 Minor이 구간은 현재 stylelint 오류를 그대로 남깁니다.
Line 75와 Line 79 앞의 빈 줄 때문에
declaration-empty-line-before가 발생하고 있습니다. lint를 CI에 걸어두었다면 이 파일 하나로도 파이프라인이 막힐 수 있습니다.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/styles/modal.css` around lines 69 - 80, The CSS for .modal-description contains forbidden blank lines before declarations which triggers stylelint's declaration-empty-line-before rule; remove the empty line before the "display" declaration and the empty line before the "height" declaration (i.e., collapse the stray blank lines so the property blocks are contiguous), save the file, and re-run stylelint/CI to confirm the error is resolved.
🧹 Nitpick comments (9)
types/state.d.ts (1)
8-12: 타입 정의에서 파라미터 이름 누락
setValue,setIsSearch,setCurrentMovie메서드의 파라미터에 이름이 없고 타입만 있습니다. TypeScript에서 유효하지만, 코드 가독성과 IDE 힌트를 위해 파라미터 이름을 명시하는 것이 좋습니다.파라미터 이름을 추가하면 다른 개발자가 이 인터페이스를 사용할 때 어떤 값을 전달해야 하는지 더 명확해집니다.
♻️ 개선 제안
- setValue(string): void; - setIsSearch(boolean): void; - setCurrentMovie(string): void; + setValue(nextValue: string): void; + setIsSearch(nextIsSearch: boolean): void; + setCurrentMovie(movieId: string): void;🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@types/state.d.ts` around lines 8 - 12, The interface method signatures setValue, setIsSearch, and setCurrentMovie currently only declare parameter types without names; update each signature to include a descriptive parameter name (e.g., setValue(value: string), setIsSearch(isSearch: boolean), setCurrentMovie(movieId: string)) so IDEs and readers get clear hints—modify the declarations for setValue, setIsSearch, and setCurrentMovie in the state interface accordingly.src/api/setRate.ts (1)
1-3: localStorage 키 네이밍과 에러 처리 고려사항현재
movieId를 직접 localStorage 키로 사용하고 있습니다. 몇 가지 생각해볼 점이 있습니다:
키 충돌 가능성: 다른 기능에서 같은 키를 사용하면 충돌이 발생할 수 있습니다. 프리픽스(예:
movie_rate_${movieId})를 붙이는 방법은 어떨까요?에러 처리: localStorage는 용량 초과, 프라이빗 브라우징 모드 등에서 예외를 던질 수 있습니다. PR 설명에서 "예외 처리 최소 적용"이라고 언급하셨는데, 향후 API로 전환할 때 이 부분도 함께 고려하면 좋을 것 같습니다.
비동기 래핑은 향후 서버 API 전환을 위한 좋은 구조입니다!
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/api/setRate.ts` around lines 1 - 3, Change the localStorage key to a namespaced/prefixed key (e.g., use a pattern like `movie_rate_${movieId}`) inside the setRate function to avoid collisions and validate inputs (ensure movieId and rate are non-empty). Wrap the localStorage.setItem call in a try/catch within setRate to handle storage errors (quota, private mode) and either log/return a failure indicator or rethrow a controlled error so callers can handle it; keep the function signature async but do not change its purpose. Use the existing function name setRate and the localStorage.setItem call as the location to apply these changes.types/view.d.ts (1)
21-28:renderRate메서드의 반환 타입 누락다른 메서드들은 반환 타입이 명시되어 있지만,
renderRate(rate: number)는 반환 타입이 없습니다. 일관성을 위해: void를 추가하는 것을 권장합니다.♻️ 개선 제안
- renderRate(rate: number); + renderRate(rate: number): void;🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@types/view.d.ts` around lines 21 - 28, The ModalViewType interface's renderRate method is missing an explicit return type; update the signature for renderRate(rate: number) to include : void so it matches the other methods in ModalViewType (open, close, render, spinnerRender, errorRender) and maintains consistent typing for the interface.src/view/movieBannerView.ts (2)
49-55: 로고 클릭 시window.location.reload()대신 다른 방식을 고려해 보세요.페이지 전체를 새로고침하는 것은 사용자 경험 측면에서 다소 무거운 동작입니다. 현재 앱 구조에서 상태를 초기화하고 인기 영화 목록을 다시 불러오는 방식으로 대체할 수 있을지 검토해 보시면 좋겠습니다. 물론 현재 요구사항이 새로고침이라면 이대로 유지해도 됩니다.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/view/movieBannerView.ts` around lines 49 - 55, In `#logoBinding`(), remove the window.location.reload() call and instead trigger a local state reset and re-fetch of the popular movies view: replace the click handler to call the view's state-reset and data-load routines (e.g. invoke this.resetState() or this.clearFilters() followed by this.loadPopularMovies()/this.renderPopularMovies(), or emit a "navigate:home" event that the controller listens to) so the app reinitializes the home/popular list without a full page reload; ensure you reference this.#headerBar and the logo element as in the existing handler when wiring the new calls.
41-47:render()호출 시 이벤트 리스너 중복 등록 가능성을 검토해 보세요.
render()가 호출될 때마다#binding()이 실행되어 새로운 이벤트 리스너가 등록됩니다. 현재 구조에서는innerHTML로 DOM을 교체하기 때문에 이전 요소와 리스너가 제거되지만, 다음 사항들을 점검해 보시면 좋겠습니다:
render()가 여러 번 호출되는 시나리오가 있는지- DOM 교체 없이 부분 업데이트로 변경될 가능성이 있는지
만약 그런 경우가 있다면 어떤 방식으로 리스너 정리를 할 수 있을지 생각해 보세요.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/view/movieBannerView.ts` around lines 41 - 47, render() currently calls the private `#binding`(id) which always adds a click listener to the element selected via this.#section?.querySelector(".detail"), risking duplicate handlers if render() can be called without replacing that element; update `#binding` so it first queries the element (detailButton) and removes any previously attached handler (using a stored handler reference property on the instance, e.g. this._detailHandler) via removeEventListener before assigning the new handler, then set this._detailHandler = the bound callback that calls this.#handle(id.toString()); alternatively, if the button is always recreated, explicitly use { once: true } or ensure render guarantees element replacement—apply the remove/add pattern to the methods using `#binding`, this.#section, detailButton, and `#handle`.README.md (1)
87-116: E2E 테스트 시나리오 문서가 실제 구현과 일치하지 않습니다.기능 체크리스트에서는 "더보기 버튼을 무한 스크롤로 대체한다"라고 명시되어 있지만, E2E 테스트 시나리오 섹션(라인 87-116)에서는 여전히 "더보기 버튼", "더보기 클릭" 등의 표현이 사용되고 있습니다. 무한 스크롤 기반으로 문서를 업데이트하는 것이 좋을 것 같습니다.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@README.md` around lines 87 - 116, Update the E2E test scenario section in README.md so it matches the implemented infinite-scroll behavior: replace occurrences of "더보기 버튼" and "더보기 클릭" (used in steps like the current "2. 더보기 클릭" and "5. 검색(더보기 없어지기)") with infinite-scroll actions (e.g., "스크롤하여 추가 데이터 로드" or "페이지 하단으로 스크롤"), adjust the steps to describe triggering loads via scrolling (scroll to bottom, wait for API call and skeleton, verify new items appended), and remove or rework any checks that expect a disappearing "더보기" button to instead assert that no more data loads when end-of-list is reached. Ensure all references to "더보기" in the E2E 시나리오 (lines showing list, skeleton, search more/no-more cases) are updated to use infinite-scroll terminology and corresponding assertions.src/view/modalView.ts (1)
153-164: 스크롤 잠금은 이미 정의한body.modal-open클래스로 맞추는 편이 안전합니다.
src/styles/modal.css는body.modal-open을 기준으로 잠금을 정의해두었는데, 여기서는 inlineoverflow를 직접 바꾸고 닫을 때"auto"로 고정 복구하고 있습니다. 이 방식은 원래 body overflow 상태를 덮어써서 이후 레이아웃이나 다른 오버레이와 충돌하기 쉽습니다. 지금 단계에서는 CSS에 이미 있는 훅을 그대로 써서 상태를 한 군데로 모아보면 어떨까요?🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/view/modalView.ts` around lines 153 - 164, The open/close methods currently set document.body.style.overflow directly which overrides prior body state; instead, modify open() and close() to add/remove the existing CSS hook class (e.g., "modal-open") on document.body while keeping the current checks for this.#modalSection and the existing this.#modalSection.classList.add/remove("active") behavior; update open() to document.body.classList.add("modal-open") and close() to document.body.classList.remove("modal-open") so the CSS in src/styles/modal.css controls scrolling and you avoid hardcoding overflow ("auto").src/controller/modalController.ts (1)
22-34: 에러 타입 분기는 메시지 매퍼로 한 단계 더 분리해 두면 좋겠습니다.지금 분기 구조가
handleResponseError와 거의 같아서, 에러 타입 → 사용자 메시지 변환만 별도 함수로 빼 두면 리스트/모달이 같은 규칙을 공유하기 쉬워집니다. 다음 단계에서 저장소 구현이나 에러 정책이 바뀔 때 수정 지점도 줄어듭니다.As per coding guidelines, "Promote modular thinking—breaking problems into reusable components."
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/controller/modalController.ts` around lines 22 - 34, Extract the error-type→user-message logic into a reusable mapper function (e.g., mapResponseErrorToMessage or getErrorMessageForResponseError) that accepts a ResponseError and returns the appropriate ERROR_MESSAGE constant (handling HTTP, NETWORK, and default cases), replace the inline branching in the catch block of modalController (where ResponseError is checked and modalView.errorRender is called) with a single call to that mapper and pass its result into modalView.errorRender, and export the mapper so other modules (like list controller or handleResponseError) can reuse the same mapping.src/styles/loading.css (1)
25-25: 반복 애니메이션에 reduced-motion 대응을 넣어 주세요.스켈레톤과 스피너가 둘 다 무한 반복이라
prefers-reduced-motion: reduce환경에서는 UX가 거칠 수 있습니다. 이 경우 애니메이션을 끄거나 단순화하는 분기 하나는 추가해 두는 게 좋겠습니다.Also applies to: 84-90
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/styles/loading.css` at line 25, The CSS uses infinite animations (e.g., the animation: skeleton-loading 1.2s infinite; rule and the spinner keyframes around lines referenced) which should respect prefers-reduced-motion; add a prefers-reduced-motion: reduce media query that disables or greatly reduces animations for the skeleton-loading animation and the spinner (set animation: none or very short duration/iterations and remove transform transitions) so users with reduced-motion enabled do not see continuous motion; apply the same change to the other animation block mentioned (lines ~84-90) so both skeleton and spinner honor reduced-motion.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@cypress/e2e/home.cy.ts`:
- Around line 34-39: The test opens the modal by clicking ".detail" which
triggers modalController -> getDetail to call the /movie/{id} API but you only
stubbed the popular list; add a cy.intercept for the detail endpoint (same
pattern used in modal.cy.ts: cy.intercept("GET",
"**/movie/1?language=ko-KR®ion=KR", {...})) before clicking the ".detail"
button so the detail request is stubbed and the assertions on
".modal-background" and ".modal" become deterministic.
In `@cypress/e2e/search.cy.ts`:
- Around line 3-30: The test currently intercepts the search API (alias
"searchMovies") but does not stub the movie detail endpoint, so the card click
that opens the modal can hit the real API; add a cy.intercept for the movie
detail GET (e.g., GET "**/movie/*" or specifically "**/movie/101*") before the
card click, return a deterministic stubbed detail response (including fields
used by the modal) and alias it (e.g., "getMovieDetails"), then perform the
click and assert the modal—this ensures the modal opening is fully isolated and
stable.
In `@index.html`:
- Around line 65-70: The modal markup lacks accessibility attributes; update the
container elements (ids: modalBackground, modal, modalContainer) and the close
control (id: closeModal) to be an accessible dialog: add role="dialog" and
aria-modal="true" to the modal element, include a visible or visually-hidden
heading inside modalContainer with a unique id and set aria-labelledby on the
modal to that id, and give the close button an accessible name (e.g.,
aria-label="Close" or provide meaningful alt text on the <img>) so assistive
tech can identify and operate the dialog and close control.
In `@src/controller/mainController.ts`:
- Around line 58-64: The search mode flag is set after calling searchController
which immediately starts infiniteScrollView, causing the sentinel to trigger
popular-list logic (`#handleAddButton` -> morePopularController) before search
state is true; move the state flip or block observer start: either call
this.#appState.setIsSearch(true) before invoking searchController(including the
call sites at the other affected blocks), or modify
searchController/infiniteScrollView.start so it waits until setIsSearch(true)
(or until the first search page completes) before starting the observer; ensure
references to searchController, this.#appState.setIsSearch,
infiniteScrollView.start, `#handleAddButton`, and morePopularController are
updated accordingly.
In `@src/controller/popularController.ts`:
- Around line 13-22: The infinite scroll isn't stopped when the first page is
also the last page; update the logic after awaiting getMovies(page) in
popularController to also check the pagination fields (e.g., popularMovies.page
and popularMovies.total_pages or similar) and call infiniteScrollView.stop()
when you've reached the last page (page >= total_pages) in addition to the
existing results.length === 0 branch; ensure you mirror the last-page check used
in morePopularController so movieBannerView.render and movieListView.render only
run when appropriate and no extra page request is triggered.
In `@src/styles/main.css`:
- Around line 191-218: The parent selector `#wrap` has a global min-width: 1440px
that prevents the child media-query rules (.header-bar, .background-container,
.title) from taking effect; override or remove that min-width within the
responsive media queries by resetting `#wrap` (for example set min-width: unset or
min-width: auto and width: 100%/max-width: 100%) inside the existing `@media`
(max-width: 1279px) and `@media` (max-width: 670px) blocks so the layout can
actually shrink on smaller screens.
- Around line 64-69: The .header-bar rule uses position: fixed so justify-self
has no effect; update the .header-bar styles to center it when fixed by either
setting left: 50% and transform: translateX(-50%) or by using left: 0; right: 0;
margin: 0 auto with a constrained max-width so the bar is centered on wide
viewports; remove justify-self from .header-bar since it’s ineffective when
position is fixed.
In `@src/styles/modal.css`:
- Around line 143-148: The .myrate-stars rule uses an invalid unitless gap value
("gap: 2") so browsers ignore it; update the CSS declaration in the
.myrate-stars selector to use a valid length unit (for example "gap: 2px" or
another appropriate unit) to restore the intended spacing between star items.
In `@src/view/infiniteScrollView.ts`:
- Around line 10-18: The stop() method currently calls
this.#observer.disconnect(), which tears down the entire IntersectionObserver
and prevents restart; change stop() to call
this.#observer.unobserve(this.#sentinel) so only the sentinel is unobserved and
start() (which calls this.#observer.observe(this.#sentinel)) can be called again
to resume watching; ensure you reference the private fields `#observer` and
`#sentinel` and avoid recreating the observer in start() so the instance is
reused.
In `@src/view/modalView.ts`:
- Around line 44-80: The modal currently builds HTML by directly assigning
untrusted values into this.#detailSection.innerHTML (using item.title,
item.overview, formatMovieMeta(...), USER_RATE[rateCount], etc.), which creates
an XSS risk; change the implementation to create DOM nodes and set textContent
for any untrusted strings instead of interpolating into innerHTML: build the
modal container elements with document.createElement, set attributes/static
markup as needed, insert safe image src only after validating item.poster_path,
call this.#renderStars(rateCount) to produce star elements (not HTML strings)
and append them, and use textContent for title, category (from formatMovieMeta
output after sanitizing or splitting into safe parts), overview, and the
USER_RATE label; only use innerHTML for strictly controlled/static markup if
absolutely necessary. Ensure code paths referencing this.#detailSection,
this.#renderStars, formatMovieMeta, USER_RATE, rateCount, and item.* are updated
to append safe nodes rather than injecting raw HTML.
---
Outside diff comments:
In `@src/styles/modal.css`:
- Around line 69-80: The CSS for .modal-description contains forbidden blank
lines before declarations which triggers stylelint's
declaration-empty-line-before rule; remove the empty line before the "display"
declaration and the empty line before the "height" declaration (i.e., collapse
the stray blank lines so the property blocks are contiguous), save the file, and
re-run stylelint/CI to confirm the error is resolved.
In `@src/view/movieListView.ts`:
- Around line 28-35: The template in movieListView.ts builds HTML with unquoted
attributes (id, src, alt, onerror) and unescaped item values, which can break
parsing or open XSS; fix by wrapping attribute values in quotes and escaping
user-derived strings: ensure id, poster_path, title, and noImage are inserted as
quoted attribute values (e.g., id="${...}", src="${...}", alt="${...}",
onerror="${...}") and introduce/ reuse an HTML-escape helper (escapeHtml or
escapeAttr) to sanitize item.title and any attribute content before
interpolation; also sanitize item.id (or enforce a safe numeric/string id) and
the onerror handler string to avoid injecting raw user content.
---
Nitpick comments:
In `@README.md`:
- Around line 87-116: Update the E2E test scenario section in README.md so it
matches the implemented infinite-scroll behavior: replace occurrences of "더보기
버튼" and "더보기 클릭" (used in steps like the current "2. 더보기 클릭" and "5. 검색(더보기
없어지기)") with infinite-scroll actions (e.g., "스크롤하여 추가 데이터 로드" or "페이지 하단으로
스크롤"), adjust the steps to describe triggering loads via scrolling (scroll to
bottom, wait for API call and skeleton, verify new items appended), and remove
or rework any checks that expect a disappearing "더보기" button to instead assert
that no more data loads when end-of-list is reached. Ensure all references to
"더보기" in the E2E 시나리오 (lines showing list, skeleton, search more/no-more cases)
are updated to use infinite-scroll terminology and corresponding assertions.
In `@src/api/setRate.ts`:
- Around line 1-3: Change the localStorage key to a namespaced/prefixed key
(e.g., use a pattern like `movie_rate_${movieId}`) inside the setRate function
to avoid collisions and validate inputs (ensure movieId and rate are non-empty).
Wrap the localStorage.setItem call in a try/catch within setRate to handle
storage errors (quota, private mode) and either log/return a failure indicator
or rethrow a controlled error so callers can handle it; keep the function
signature async but do not change its purpose. Use the existing function name
setRate and the localStorage.setItem call as the location to apply these
changes.
In `@src/controller/modalController.ts`:
- Around line 22-34: Extract the error-type→user-message logic into a reusable
mapper function (e.g., mapResponseErrorToMessage or
getErrorMessageForResponseError) that accepts a ResponseError and returns the
appropriate ERROR_MESSAGE constant (handling HTTP, NETWORK, and default cases),
replace the inline branching in the catch block of modalController (where
ResponseError is checked and modalView.errorRender is called) with a single call
to that mapper and pass its result into modalView.errorRender, and export the
mapper so other modules (like list controller or handleResponseError) can reuse
the same mapping.
In `@src/styles/loading.css`:
- Line 25: The CSS uses infinite animations (e.g., the animation:
skeleton-loading 1.2s infinite; rule and the spinner keyframes around lines
referenced) which should respect prefers-reduced-motion; add a
prefers-reduced-motion: reduce media query that disables or greatly reduces
animations for the skeleton-loading animation and the spinner (set animation:
none or very short duration/iterations and remove transform transitions) so
users with reduced-motion enabled do not see continuous motion; apply the same
change to the other animation block mentioned (lines ~84-90) so both skeleton
and spinner honor reduced-motion.
In `@src/view/modalView.ts`:
- Around line 153-164: The open/close methods currently set
document.body.style.overflow directly which overrides prior body state; instead,
modify open() and close() to add/remove the existing CSS hook class (e.g.,
"modal-open") on document.body while keeping the current checks for
this.#modalSection and the existing
this.#modalSection.classList.add/remove("active") behavior; update open() to
document.body.classList.add("modal-open") and close() to
document.body.classList.remove("modal-open") so the CSS in src/styles/modal.css
controls scrolling and you avoid hardcoding overflow ("auto").
In `@src/view/movieBannerView.ts`:
- Around line 49-55: In `#logoBinding`(), remove the window.location.reload() call
and instead trigger a local state reset and re-fetch of the popular movies view:
replace the click handler to call the view's state-reset and data-load routines
(e.g. invoke this.resetState() or this.clearFilters() followed by
this.loadPopularMovies()/this.renderPopularMovies(), or emit a "navigate:home"
event that the controller listens to) so the app reinitializes the home/popular
list without a full page reload; ensure you reference this.#headerBar and the
logo element as in the existing handler when wiring the new calls.
- Around line 41-47: render() currently calls the private `#binding`(id) which
always adds a click listener to the element selected via
this.#section?.querySelector(".detail"), risking duplicate handlers if render()
can be called without replacing that element; update `#binding` so it first
queries the element (detailButton) and removes any previously attached handler
(using a stored handler reference property on the instance, e.g.
this._detailHandler) via removeEventListener before assigning the new handler,
then set this._detailHandler = the bound callback that calls
this.#handle(id.toString()); alternatively, if the button is always recreated,
explicitly use { once: true } or ensure render guarantees element
replacement—apply the remove/add pattern to the methods using `#binding`,
this.#section, detailButton, and `#handle`.
In `@types/state.d.ts`:
- Around line 8-12: The interface method signatures setValue, setIsSearch, and
setCurrentMovie currently only declare parameter types without names; update
each signature to include a descriptive parameter name (e.g., setValue(value:
string), setIsSearch(isSearch: boolean), setCurrentMovie(movieId: string)) so
IDEs and readers get clear hints—modify the declarations for setValue,
setIsSearch, and setCurrentMovie in the state interface accordingly.
In `@types/view.d.ts`:
- Around line 21-28: The ModalViewType interface's renderRate method is missing
an explicit return type; update the signature for renderRate(rate: number) to
include : void so it matches the other methods in ModalViewType (open, close,
render, spinnerRender, errorRender) and maintains consistent typing for the
interface.
🪄 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: aa8f8c6e-bd16-4c70-99a9-ae2400d040eb
📒 Files selected for processing (30)
README.mdcypress/e2e/home.cy.tscypress/e2e/modal.cy.tscypress/e2e/search.cy.tsindex.htmlsrc/api/getDetail.tssrc/api/getRate.tssrc/api/setRate.tssrc/constants/rate.tssrc/controller/handleResponseError.tssrc/controller/mainController.tssrc/controller/modalController.tssrc/controller/moreButtonController.tssrc/controller/popularController.tssrc/controller/rateController.tssrc/controller/searchController.tssrc/state/appState.tssrc/styles/index.csssrc/styles/loading.csssrc/styles/main.csssrc/styles/modal.csssrc/styles/thumbnail.csssrc/utils/formatMovieMeta.tssrc/view/infiniteScrollView.tssrc/view/modalView.tssrc/view/movieBannerView.tssrc/view/movieListView.tstypes/api.d.tstypes/state.d.tstypes/view.d.ts
Comment on lines
+34
to
+39
| it("배너의 자세히 보기 버튼 클릭 시 모달이 열린다.", () => { | ||
| cy.wait("@page1"); | ||
|
|
||
| cy.get(".top-rated-movie .detail").click(); | ||
| cy.get(".modal-background").should("have.class", "active"); | ||
| cy.get(".modal").should("be.visible"); |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -e
printf '\n== cypress/e2e/home.cy.ts intercept 목록 ==\n'
rg -n --type=ts 'cy\.intercept\(' cypress/e2e/home.cy.ts
printf '\n== 모달/상세 fetch 관련 코드 ==\n'
rg -n --type=ts 'getDetail|movieDetail|modalController|/movie/' src cypress/e2e/home.cy.ts cypress/e2e/modal.cy.tsRepository: woowacourse/javascript-movie-review
Length of output: 1900
모달 오픈 테스트에서 상세 정보 API를 stub하지 않고 있습니다.
이 테스트는 ".detail" 버튼 클릭 시 모달을 여는 것을 검증하지만, 모달 열기는 modalController → getDetail 함수를 통해 /movie/{id} 엔드포인트로 실제 API 요청을 보냅니다. 인기 목록(/movie/popular)만 stub하고 상세 정보 요청을 stub하지 않으면, 테스트가 네트워크 지연이나 응답 시간에 따라 불안정해질 수 있습니다.
cypress/e2e/modal.cy.ts를 보면 동일한 상세 정보 요청을 line 18에서 cy.intercept("GET", "**/movie/1?language=ko-KR®ion=KR", { ... }) 방식으로 stub하고 있습니다. 현재 코드 (lines 34-39, 78-83)에서도 같은 방식으로 상세 정보 엔드포인트를 stub하도록 추가해야 테스트의 안정성을 높일 수 있습니다. modal.cy.ts의 stub 패턴을 참고하여 어떤 요청을 어떻게 고정해야 할지 검토해보세요.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@cypress/e2e/home.cy.ts` around lines 34 - 39, The test opens the modal by
clicking ".detail" which triggers modalController -> getDetail to call the
/movie/{id} API but you only stubbed the popular list; add a cy.intercept for
the detail endpoint (same pattern used in modal.cy.ts: cy.intercept("GET",
"**/movie/1?language=ko-KR®ion=KR", {...})) before clicking the ".detail"
button so the detail request is stubbed and the assertions on
".modal-background" and ".modal" become deterministic.
Comment on lines
+3
to
+30
| cy.intercept("GET", "**/search/movie*", { | ||
| statusCode: 200, | ||
| body: { | ||
| page: 1, | ||
| results: [ | ||
| { | ||
| id: 101, | ||
| title: "해리 포터와 마법사의 돌", | ||
| poster_path: "/harry1.jpg", | ||
| vote_average: 7.8, | ||
| }, | ||
| { | ||
| id: 102, | ||
| title: "해리 포터와 비밀의 방", | ||
| poster_path: "/harry2.jpg", | ||
| vote_average: 7.6, | ||
| }, | ||
| { | ||
| id: 103, | ||
| title: "해리 포터와 아즈카반의 죄수", | ||
| poster_path: "/harry3.jpg", | ||
| vote_average: 8.0, | ||
| }, | ||
| ], | ||
| total_pages: 1, | ||
| total_results: 3, | ||
| }, | ||
| }).as("searchMovies"); |
There was a problem hiding this comment.
모달 테스트가 실제 상세 API에 의존합니다.
여기서는 검색 API만 mock하고 있는데, Line 51의 카드 클릭은 상세 조회까지 이어집니다. 지금 상태면 네트워크 상태나 토큰 설정에 따라 테스트가 흔들릴 수 있어서, 클릭 전에 상세 endpoint도 함께 intercept해서 모달 열림을 완전히 고립된 상태로 검증하는 편이 안전합니다.
Also applies to: 48-52
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@cypress/e2e/search.cy.ts` around lines 3 - 30, The test currently intercepts
the search API (alias "searchMovies") but does not stub the movie detail
endpoint, so the card click that opens the modal can hit the real API; add a
cy.intercept for the movie detail GET (e.g., GET "**/movie/*" or specifically
"**/movie/101*") before the card click, return a deterministic stubbed detail
response (including fields used by the modal) and alias it (e.g.,
"getMovieDetails"), then perform the click and assert the modal—this ensures the
modal opening is fully isolated and stable.
Comment on lines
+65
to
+70
| <div class="modal-background" id="modalBackground"> | ||
| <div class="modal"> | ||
| <button class="close-modal" id="closeModal"> | ||
| <img src="./src/asset/images/modal_button_close.png" /> | ||
| </button> | ||
| <div class="modal-container" id="modalContainer"></div> |
There was a problem hiding this comment.
모달 접근성 정보가 빠져 있습니다.
이 구조만으로는 보조기기가 이 영역을 대화상자로 인식하지 못하고, 닫기 버튼도 접근 가능한 이름이 없습니다. role="dialog" / aria-modal / 제목 연결과 닫기 버튼의 aria-label 또는 이미지 alt 중 하나는 넣어 두는 편이 안전합니다.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@index.html` around lines 65 - 70, The modal markup lacks accessibility
attributes; update the container elements (ids: modalBackground, modal,
modalContainer) and the close control (id: closeModal) to be an accessible
dialog: add role="dialog" and aria-modal="true" to the modal element, include a
visible or visually-hidden heading inside modalContainer with a unique id and
set aria-labelledby on the modal to that id, and give the close button an
accessible name (e.g., aria-label="Close" or provide meaningful alt text on the
<img>) so assistive tech can identify and operate the dialog and close control.
Comment on lines
+58
to
64
| await searchController( | ||
| this.#appState, | ||
| this.#movieListView, | ||
| this.#infiniteScrollView, | ||
| ); | ||
|
|
||
| this.#appState.setIsSearch(true); |
There was a problem hiding this comment.
검색 모드 전환 시점이 늦어서 첫 인터섹션이 인기 목록 분기를 탈 수 있습니다.
searchController()는 내부에서 바로 infiniteScrollView.start()를 호출하는데, setIsSearch(true)는 그 뒤에 실행됩니다. 첫 검색 응답을 기다리는 동안 sentinel이 먼저 교차하면 #handleAddButton()이 아직 false를 읽고 morePopularController()로 들어가서 검색 결과에 인기 영화 페이지가 섞일 수 있습니다. 검색 상태를 먼저 확정할지, 아니면 첫 페이지가 끝날 때까지 observer를 막을지 한 번 점검해 주세요.
Also applies to: 72-79, 91-103
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/controller/mainController.ts` around lines 58 - 64, The search mode flag
is set after calling searchController which immediately starts
infiniteScrollView, causing the sentinel to trigger popular-list logic
(`#handleAddButton` -> morePopularController) before search state is true; move
the state flip or block observer start: either call
this.#appState.setIsSearch(true) before invoking searchController(including the
call sites at the other affected blocks), or modify
searchController/infiniteScrollView.start so it waits until setIsSearch(true)
(or until the first search page completes) before starting the observer; ensure
references to searchController, this.#appState.setIsSearch,
infiniteScrollView.start, `#handleAddButton`, and morePopularController are
updated accordingly.
Comment on lines
+13
to
22
| infiniteScrollView.start(); | ||
| movieListView.skeletonRender(SKELETON_NUMBER); | ||
| const popularMovies: movieResponse = await getMovies(page); | ||
| if (popularMovies.results.length === 0) { | ||
| movieListView.emptyRender(); | ||
| addButtonView.hide(); | ||
| infiniteScrollView.stop(); | ||
| return; | ||
| } | ||
| movieBannerView.render(popularMovies.results[0]); | ||
| movieListView.render(popularMovies.results); |
There was a problem hiding this comment.
첫 페이지가 마지막 페이지인 경우에도 옵저버를 멈춰주세요.
여기서는 results.length === 0만 보고 멈추고 있어서, 1페이지 응답이 곧 마지막 페이지인 케이스에서는 무한 스크롤이 계속 살아 있습니다. 그러면 단일 페이지 결과에서도 sentinel이 보일 때 불필요한 다음 페이지 요청이 한 번 더 나가게 됩니다. morePopularController처럼 여기서도 마지막 페이지 판정을 같이 두는 쪽이 안전해 보입니다.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/controller/popularController.ts` around lines 13 - 22, The infinite
scroll isn't stopped when the first page is also the last page; update the logic
after awaiting getMovies(page) in popularController to also check the pagination
fields (e.g., popularMovies.page and popularMovies.total_pages or similar) and
call infiniteScrollView.stop() when you've reached the last page (page >=
total_pages) in addition to the existing results.length === 0 branch; ensure you
mirror the last-page check used in morePopularController so
movieBannerView.render and movieListView.render only run when appropriate and no
extra page request is triggered.
Comment on lines
64
to
69
| .header-bar { | ||
| position: absolute; | ||
| position: fixed; | ||
| z-index: 10; | ||
| width: 1280px; | ||
| max-width: 1280px; | ||
| width: 100%; | ||
| justify-self: center; |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
# First, check if the file exists and examine the relevant lines
if [ -f "src/styles/main.css" ]; then
echo "=== Lines 48-69 (including wrapper context) ==="
sed -n '48,69p' src/styles/main.css | cat -n
echo ""
echo "=== Lines 191-218 (responsive breakpoints) ==="
sed -n '191,218p' src/styles/main.css | cat -n
else
echo "File not found at src/styles/main.css"
# Try to locate the file
find . -name "main.css" -type f
fiRepository: woowacourse/javascript-movie-review
Length of output: 1234
🌐 Web query:
CSS position fixed justify-self flexbox centering behavior
💡 Result:
CSS position: fixed removes an element from the normal document flow, taking it out of flexbox layout participation entirely. Flexbox properties like justify-self (or align-self) on a position: fixed flex item have no effect because the element no longer participates in flex container alignment. Key points: - position: fixed (like absolute) makes the element out-of-flow. Per CSS Flexible Box Layout spec: "An absolutely-positioned child of a flex container does not participate in flex layout." Fixed positioning follows the same principle. - The element's containing block becomes the viewport (initial containing block), not the flex container. - justify-self is ignored in flexbox layouts entirely (applies to grid, block, abs-pos). MDN: "In flexbox layouts, this property is ignored." - For centering a fixed-position element, use viewport-relative positioning: left: 50%; top: 50%; transform: translate(-50%, -50%); or make a fixed flex container with display: flex; justify-content: center; align-items: center; position: fixed; inset: 0;. Examples from sources confirm align-self: center fails on fixed flex items (e.g., Firefox issues), requiring manual centering. To center content inside a fixed element using flexbox, apply flex properties to the fixed element itself, not its children if they're also positioned.
Citations:
- 1: https://developer.mozilla.org/en-US/docs/Web/CSS/Reference/Properties/justify-self
- 2: https://developer.mozilla.org/en-US/docs/Web/CSS/justify-self
- 3: https://stackoverflow.com/questions/29036231/how-can-i-have-a-position-fixed-behaviour-for-a-flexbox-sized-element
- 4: https://developer.mozilla.org/en-US/docs/Web/CSS/Containing_Block
- 5: https://developer.mozilla.org/en-US/docs/Web/CSS/CSS_Box_Alignment/Box_Alignment_In_Block_Abspos_Tables
- 6: https://stackoverflow.com/questions/42526089/flexbox-align-self-center-doesnt-work-with-position-fixed-in-firefox
- 7: https://developer.mozilla.org/en-US/docs/Web/CSS/Guides/Flexible_box_layout/Aligning_items
- 8: https://css-tricks.com/snippets/css/a-guide-to-flexbox/
- 9: https://stackoverflow.com/questions/67452885/why-does-position-fixed-not-work-with-justify-content-space-between
- 10: https://stackoverflow.com/questions/36006173/exclude-element-with-fixed-positioning-from-justify-content-in-flex-layout
.header-bar는 justify-self로 중앙 정렬되지 않습니다.
position: fixed는 요소를 일반 문서 흐름에서 제거하므로 justify-self는 효과가 없습니다. width: 100%와 max-width: 1280px만으로는 넓은 화면에서 헤더 바가 뷰포트의 왼쪽에 고정됩니다. 중앙 정렬을 의도한다면 left: 50%; transform: translateX(-50%); 또는 left: 0; right: 0; margin: 0 auto;로 수정하세요.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/styles/main.css` around lines 64 - 69, The .header-bar rule uses
position: fixed so justify-self has no effect; update the .header-bar styles to
center it when fixed by either setting left: 50% and transform: translateX(-50%)
or by using left: 0; right: 0; margin: 0 auto with a constrained max-width so
the bar is centered on wide viewports; remove justify-self from .header-bar
since it’s ineffective when position is fixed.
Comment on lines
+191
to
+218
| @media (max-width: 1279px) { | ||
| .header-bar { | ||
| display: flex; | ||
| flex-direction: column; | ||
| gap: 24px; | ||
| } | ||
|
|
||
| .background-container { | ||
| padding: 80px 48px; | ||
| align-items: end; | ||
| } | ||
| } | ||
|
|
||
| @media (max-width: 670px) { | ||
| .header-bar { | ||
| max-width: 300px; | ||
| width: 100%; | ||
| } | ||
|
|
||
| .background-container { | ||
| height: 400px; | ||
| } | ||
|
|
||
| .title { | ||
| font-size: 24px; | ||
| font-weight: 600; | ||
| } | ||
| } |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
cat -n src/styles/main.css | head -50Repository: woowacourse/javascript-movie-review
Length of output: 1133
🏁 Script executed:
rg "#wrap" src/styles/main.css -A 3Repository: woowacourse/javascript-movie-review
Length of output: 298
🏁 Script executed:
wc -l src/styles/main.cssRepository: woowacourse/javascript-movie-review
Length of output: 102
🏁 Script executed:
cat -n src/styles/main.cssRepository: woowacourse/javascript-movie-review
Length of output: 4741
#wrap의 min-width: 1440px 규칙이 반응형 UI를 차단합니다.
추가된 반응형 규칙은 자식 요소들(.header-bar, .background-container, .title)만 수정하지만, 부모 컨테이너인 #wrap의 min-width: 1440px 속성은 여전히 유효합니다. 1440px 이하의 화면에서는 #wrap 자체가 최소 1440px 너비를 유지하므로 가로 스크롤이 강제되며, 반응형 UI가 실제로 작동하지 않습니다. 미디어 쿼리에서 #wrap의 min-width를 auto 또는 제거해야 합니다.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/styles/main.css` around lines 191 - 218, The parent selector `#wrap` has a
global min-width: 1440px that prevents the child media-query rules (.header-bar,
.background-container, .title) from taking effect; override or remove that
min-width within the responsive media queries by resetting `#wrap` (for example
set min-width: unset or min-width: auto and width: 100%/max-width: 100%) inside
the existing `@media` (max-width: 1279px) and `@media` (max-width: 670px) blocks so
the layout can actually shrink on smaller screens.
Comment on lines
+143
to
+148
| .myrate-stars { | ||
| width: 170px; | ||
| box-sizing: content-box; | ||
| display: flex; | ||
| gap: 2; | ||
| } |
There was a problem hiding this comment.
gap: 2는 무효 값이라 별 간격이 적용되지 않습니다.
gap에는 단위가 있는 길이 값이 필요해서 현재 선언은 브라우저가 무시합니다. 별점 간격이 의도와 다르다면 먼저 2px처럼 단위를 붙여 확인해보세요.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/styles/modal.css` around lines 143 - 148, The .myrate-stars rule uses an
invalid unitless gap value ("gap: 2") so browsers ignore it; update the CSS
declaration in the .myrate-stars selector to use a valid length unit (for
example "gap: 2px" or another appropriate unit) to restore the intended spacing
between star items.
Comment on lines
+10
to
+18
| start() { | ||
| if (!this.#sentinel) return; | ||
|
|
||
| this.#observer.observe(this.#sentinel); | ||
| } | ||
|
|
||
| stop() { | ||
| this.#observer.disconnect(); | ||
| } |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Description: Check how start/stop are called in controllers
rg -n "infiniteScrollView\.(start|stop)" --type=ts -C2Repository: woowacourse/javascript-movie-review
Length of output: 3509
🏁 Script executed:
cat -n src/view/infiniteScrollView.tsRepository: woowacourse/javascript-movie-review
Length of output: 573
start()와 stop() 메서드의 재시작 불가능 문제를 해결하세요.
현재 구현에서 stop()은 observer.disconnect()를 호출합니다. IntersectionObserver.disconnect()는 관찰을 완전히 해제하므로, 이후 start()를 다시 호출해도 작동하지 않습니다.
코드 분석 결과:
searchController.ts와popularController.ts에서 각각start()를 호출한 후 조건에 따라stop()을 호출합니다- 사용자가 다른 검색을 수행하거나 화면을 전환할 때,
start()가 다시 호출되는 시나리오가 있습니다 - 현재 구현에서는 첫 번째
stop()이후 무한 스크롤이 작동하지 않습니다
개선 방안:
stop() 메서드에서 disconnect() 대신 unobserve(this.#sentinel)를 사용하여 sentinel 요소에 대한 관찰만 해제하세요. 이렇게 하면 start()가 재호출될 때 observer를 재사용할 수 있습니다.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/view/infiniteScrollView.ts` around lines 10 - 18, The stop() method
currently calls this.#observer.disconnect(), which tears down the entire
IntersectionObserver and prevents restart; change stop() to call
this.#observer.unobserve(this.#sentinel) so only the sentinel is unobserved and
start() (which calls this.#observer.observe(this.#sentinel)) can be called again
to resume watching; ensure you reference the private fields `#observer` and
`#sentinel` and avoid recreating the observer in start() so the instance is
reused.
Comment on lines
+44
to
+80
| this.#detailSection.innerHTML = ` | ||
| <div class="modal-image"> | ||
| <img | ||
| src="https://image.tmdb.org/t/p/original/${item.poster_path}" | ||
| /> | ||
| </div> | ||
| <div class="modal-description"> | ||
| <div class="modal-title-section"> | ||
| <h2>${item.title}</h2> | ||
| <p class="category"> | ||
| ${formatMovieMeta(item.release_date, item.genres)} | ||
| </p> | ||
| <div class="rate-section"> | ||
| <p>평균</p> | ||
| <div class="rate"> | ||
| <img src="${filledStar}" class="star" /> | ||
| <p>${item.vote_average.toFixed(1)}</p> | ||
| </div> | ||
| </div> | ||
| </div> | ||
| <div class="modal-myrate-section"> | ||
| <h3 class="myrate-title">내 별점</h3> | ||
| <div class="myrate-section"> | ||
| <div class="myrate-stars"> | ||
| ${this.#renderStars(rateCount)} | ||
| </div> | ||
| <p class="myrate-comment">${USER_RATE[rateCount]} <span class="myrate-score">(${rateCount * 2}/10)</span></p> | ||
| </div> | ||
| </div> | ||
| <div class="modal-detail-section"> | ||
| <h3 class="detail-title">줄거리</h3> | ||
| <p class="detail"> | ||
| ${item.overview} | ||
| </p> | ||
| </div> | ||
| </div> | ||
| `; |
There was a problem hiding this comment.
외부 문자열을 innerHTML로 바로 주입하면 모달이 XSS 경로가 됩니다.
여기서는 item.title, item.overview, formatMovieMeta(...), message처럼 API/에러 경계에서 들어온 문자열을 그대로 HTML에 섞고 있습니다. 값 안에 마크업이나 이벤트 속성이 섞이면 모달 렌더링 과정에서 그대로 실행될 수 있습니다. 이 부분은 먼저 어떤 필드가 신뢰되지 않는지 분리하고, 그다음 텍스트는 textContent로 넣는 경로와 정말 HTML이 필요한 경로를 나눠서 정리해보는 편이 좋겠습니다.
Also applies to: 95-104
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/view/modalView.ts` around lines 44 - 80, The modal currently builds HTML
by directly assigning untrusted values into this.#detailSection.innerHTML (using
item.title, item.overview, formatMovieMeta(...), USER_RATE[rateCount], etc.),
which creates an XSS risk; change the implementation to create DOM nodes and set
textContent for any untrusted strings instead of interpolating into innerHTML:
build the modal container elements with document.createElement, set
attributes/static markup as needed, insert safe image src only after validating
item.poster_path, call this.#renderStars(rateCount) to produce star elements
(not HTML strings) and append them, and use textContent for title, category
(from formatMovieMeta output after sanitizing or splitting into safe parts),
overview, and the USER_RATE label; only use innerHTML for strictly
controlled/static markup if absolutely necessary. Ensure code paths referencing
this.#detailSection, this.#renderStars, formatMovieMeta, USER_RATE, rateCount,
and item.* are updated to append safe nodes rather than injecting raw HTML.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
🎯 미션 소개
🕵️ 셀프 리뷰(Self-Review)
제출 전 체크 리스트
리뷰 요청 & 논의하고 싶은 내용
1) 이번 단계에서 가장 많이 고민했던 문제와 해결 과정에서 배운 점
더보기 무한 스크롤 전환
이번 미션에서 더보기 버튼을 무한 스크롤로 전환하면서 역할과 책임에 따라 파일을 분리하는 것이 중요하다는 것을 깨달았습니다. 1차 미션에서 뷰와 컨트롤러를 분리해둔 덕분에, 뷰의 연결 부분만 수정하여 해당 로직만 연결함으로써 이전에 사용하던 컨트롤러를 그대로 사용할 수 있었습니다. 이러한 역할과 책임 분리가 잘 이루어질수록 수정과 유지보수가 쉬운 코드가 된다고 생각했고, 좋은 구조를 위해 더 고민해야겠다고 느꼈습니다.
반응형 UI 로딩 처리
반응형을 적용하여 다양한 기기에서 확인했을 때 이질감이 없도록 UI를 구현해보았습니다. 브라우저 사이즈를 줄여도 그에 맞게 자연스럽게 줄어들며 다양한 가로 길이에 대응할 수 있도록 구현했습니다. 가로 사이즈는 비교적 명확했지만, 세로 사이즈는 대응이 어려워 %와 스크롤을 활용하여 최대한 고려하며 구현했습니다.
이벤트 위임
리스트나 별점을 클릭했을 때 발생하는 기능을 구현하면서, 요소의 개수가 많기 때문에 각각에 이벤트를 적용하는 것은 비효율적이라고 생각했습니다. 이에 대해 찾아보던 중 이벤트 위임이 적절한 방법이라고 판단했고, 부모 태그에 이벤트를 등록한 뒤 클릭된 자식을 판별하는 방식으로 구현했습니다. 이를 통해 DOM 접근을 최소화할 수 있었습니다.
Intersection Observer
스크롤 이벤트를 사용하는 대신 Intersection Observer를 활용하여 특정 태그가 화면에 보일 때 데이터를 불러오도록 구현했습니다. 스크롤이 발생할 때마다 길이를 계산하는 과정을 줄일 수 있어 더 효율적으로 구현할 수 있었습니다.
2) 이번 리뷰를 통해 논의하고 싶은 부분
로컬 스토리지
localStorage를 사용하되, 서버 API로 쉽게 교체할 수 있는 구조로 개발하는 것을 목표로 했습니다. 하지만 이를 충분히 잘 활용하지는 못한 것 같습니다. 최소한의 예외 처리를 하고, 비동기 형태로 감싸두면 API로 변경하더라도 동작하지 않을까 생각하여 적용해보았지만 확신이 없습니다. 어떤 기준으로 접근해야 이 목표를 달성할 수 있을지 궁금합니다.
리팩토링 잘하는 법
리뷰를 받기 전에 스스로 리팩토링을 시도해봤지만 항상 어렵게 느껴집니다. 리뷰어의 피드백을 반영하는 것은 비교적 수월하지만, 작성한 코드를 스스로 보면서 어색한 부분을 찾아내는 과정이 특히 어렵습니다. 문제를 명확하게 인지하지 못하다 보니 해결 방법도 쉽게 떠오르지 않아 리팩토링이 잘 진행되지 않는 것 같습니다. 어떤 부분을 중점적으로 보며 리팩토링을 진행하시는지 궁금합니다.
현재 내가 채우면 좋을 부분
레벨 1이 이번 주로 마무리되는데, 아직 많이 부족하다고 느끼고 있습니다. 구조를 설계하는 것, CSS를 깔끔하게 작성하는 것, 식별자 이름을 명확하게 짓는 것, 사용자 경험을 고려하는 것, 테스트 등에서 조금씩 나아지고는 있지만 여전히 부족한 부분이 많다고 생각합니다. 레벨 2에서는 React를 사용할 것 같은데, 제 코드를 기준으로 방학 동안 보완하면 좋을 부분이나 학습하면 좋을 방향이 있다면 조언 부탁드립니다.
✅ 리뷰어 체크 포인트