[2단계 - 영화 목록 불러오기] 피트 미션 제출합니다. #294
[2단계 - 영화 목록 불러오기] 피트 미션 제출합니다. #294GamjaIsMine02 wants to merge 25 commits intowoowacourse:gamjaismine02from
Conversation
|
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영화 목록 페이지에 "더 보기" 버튼 클릭 방식을 무한 스크롤로 변경하고, 영화 포스터/제목 클릭 시 상세 정보 모달을 표시하는 기능을 추가했습니다. 모달에는 별점 5단계 평가 시스템이 포함되며, 사용자 평가는 로컬 스토리지에 저장되어 새로고침 후에도 유지됩니다. 반응형 디자인을 위해 데스크톱(5열), 태블릿(3열), 모바일(1열) 그리드와 각 뷰포트에 맞는 모달 배치가 추가되었습니다. 함께 모달 동작, 무한 스크롤, 별점 기능에 대한 종단 간 테스트 케이스들이 추가되었습니다. 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: 19
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/view/movieListView.ts (1)
14-28:⚠️ Potential issue | 🟠 Major외부 API 값을
innerHTML에 직접 넣는 건 위험합니다.
movie.title과poster_path를 템플릿 문자열로 그대로 삽입하고 있어서, 제목에 따옴표나 태그가 섞이면 마크업이 깨지거나 XSS 경로가 열릴 수 있습니다. 이 렌더링 경로는 DOM API 조립이나 안전한 escaping 쪽으로 한 번 더 생각해보는 편이 좋겠습니다.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/view/movieListView.ts` around lines 14 - 28, The current code assigns untrusted values (movie.title, movie.poster_path, movie.vote_average) directly into li.innerHTML which risks markup breakage and XSS; change the rendering in movieListView.ts to build the DOM using createElement/appendChild (or setElement.textContent) instead of innerHTML: create the container div.item, an img element with src set via setAttribute using a safe URL (use posterError if no poster_path) and alt set via img.alt or textContent for titles, set the vote-average span textContent to movie.vote_average ? movie.vote_average.toFixed(1) : "0", and set the title via textContent on a strong element; this ensures values are escaped and attributes are assigned safely rather than interpolated into HTML.
🧹 Nitpick comments (6)
src/view/getElementView.ts (1)
1-17: 주석 처리된 코드는 제거하는 것이 좋습니다.주석 처리된 이전 구현들은 버전 관리 시스템(Git)에서 히스토리로 추적할 수 있으므로, 코드베이스에 남겨둘 필요가 없습니다. 이런 주석 코드는 가독성을 떨어뜨리고 유지보수 시 혼란을 줄 수 있습니다.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/view/getElementView.ts` around lines 1 - 17, Remove the commented-out legacy helper functions getElement, getUListElement, and getInputElement from src/view/getElementView.ts; delete the entire commented blocks so only active, necessary code remains (rely on Git history for the old implementations) and ensure no residual commented code or TODOs referencing those function names are left behind.src/events/bindMovieEvent.ts (1)
13-16: 주석 처리된 코드는 제거를 권장합니다.이전 "더 보기" 버튼 관련 코드가 주석으로 남아있습니다. Git 히스토리에서 확인 가능하므로 제거해도 무방합니다.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/events/bindMovieEvent.ts` around lines 13 - 16, Remove the commented-out "more" button code block referencing getElement(".display-more-btn"), the moreBtn variable, and the onMore() call in bindMovieEvent.ts; since it's obsolete and retrievable from Git history, delete those three commented lines to clean up the file and leave no commented dead code behind.src/service/movieApi.ts (1)
21-24: 반환 타입 명시를 고려해보세요.다른 fetch 함수들(
fetchDefaultMovieList,fetchSearchMovieList)은Promise<Movie[]>와 같이 명시적인 반환 타입을 가지고 있습니다.fetchMovieDetail도 동일한 패턴을 따르면 타입 안전성이 향상될 것입니다.이 함수가 반환해야 할 데이터의 구조는 무엇일까요?
Movie인터페이스를 그대로 사용할 수 있을지, 아니면 상세 정보에 맞는 별도의 타입이 필요할지 고민해보시기 바랍니다.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/service/movieApi.ts` around lines 21 - 24, The function fetchMovieDetail currently lacks an explicit return type; add a precise Promise return type (e.g. Promise<Movie> or a new Promise<MovieDetail>) and update its signature accordingly (fetchMovieDetail(movieId: number): Promise<...>), creating a MovieDetail interface if the returned payload differs from Movie, and ensure the call to request is typed or the response is mapped to that interface so TypeScript enforces the shape.cypress/e2e/popularPageTest.cy.ts (1)
30-41: 주석 처리된 이전 테스트 코드는 정리하는 편이 좋습니다.동작이 바뀐 뒤 남아 있는 주석 테스트는 향후 유지보수 시 혼동을 줄 수 있어 삭제를 권장합니다.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@cypress/e2e/popularPageTest.cy.ts` around lines 30 - 41, Remove the commented-out test block for "더 보기 버튼을 누르면 다음 페이지 영화가 기존 목록 뒤에 추가된다." (the entire commented cy.get... block) from popularPageTest.cy.ts to avoid confusion and dead code; if you need to preserve its intent, move it to an archived notes file or a proper skipped test (e.g., use it.skip) but do not leave the large commented test in the spec file.templates/styles/main.css (1)
317-319: 동일 셀렉터 분산 정의는 한 곳으로 합치는 편이 좋습니다.
error-container스타일이 블록 두 곳으로 나뉘어 있어 추적 비용이 커집니다.margin-bottom도 기존.error-container블록에 합쳐 관리하는 것을 권장합니다.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@templates/styles/main.css` around lines 317 - 319, 현재 프로젝트에 .error-container 스타일이 분산되어 있어 추적이 어렵습니다; .error-container 선택자에 정의된 margin-bottom: 5rem을 기존 .error-container 블록으로 옮겨서 중복된 블록을 제거하고 하나의 선언으로 합치세요 (찾을 식별자: .error-container, 속성: margin-bottom).templates/styles/tablet.css (1)
12-15: 주석 처리된 CSS 선언은 제거를 권장합니다.Line 14의 주석 처리된
grid-template-columns는 의도가 불명확해 유지보수 시 혼동을 줄 수 있습니다. 필요 없다면 정리하는 편이 좋습니다.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@templates/styles/tablet.css` around lines 12 - 15, Remove the commented CSS declaration inside .thumbnail-list: delete the commented-out "grid-template-columns: repeat(3, 200px);" line so the .thumbnail-list block contains only active styles (margin and gap); if the grid behavior is needed, restore it as an active rule with proper intent documented near .thumbnail-list or add a brief comment explaining why it must remain.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@cypress/e2e/modalTest.cy.ts`:
- Around line 34-48: The test name claims to verify persistence across a browser
reload but the body only reopens the modal; update the "it" test (the spec
string) or better yet add an explicit reload step: after clicking ".close-modal"
and asserting the modal is closed, call cy.reload() to simulate a page refresh,
then reopen the thumbnail (cy.get(".thumbnail-list
li").eq(4).find(".thumbnail").click()) and wait "@getMovieDetail" and finally
assert ".rate-description" and ".rate-percentage" still show the saved rating;
you can also optionally assert localStorage contains the expected rating
key/value before or after reload to prove restoration.
In `@cypress/support/modalApi.ts`:
- Around line 20-26: 현재 cy.intercept handler가 movieDetails[id]가 없을 때도 200 +
undefined body를 반환하므로 mock 누락을 숨깁니다; in the intercept callback (the cy.intercept
handler) check whether movieDetails[id] exists and if not return a non-200 error
response (e.g., 404 with an explanatory body) or explicitly fail the request via
req.reply with an error, otherwise return the normal 200 body; reference the
intercept callback, movieDetails and req.reply to locate and update the logic.
In `@index.html`:
- Around line 62-66: Wrap the modal markup with proper ARIA/dialog semantics:
add role="dialog" and aria-modal="true" to the element with id="modal" (or the
same container referenced by id="modalBackground"), ensure the dialog has an
accessible name via aria-labelledby pointing to a visible heading inside the
dialog or aria-label on the dialog, and make the close control with
id="closeModal" keyboard-and-screenreader friendly by giving the button an
accessible name (e.g., aria-label="Close" or visible text) and ensure the <img>
inside has an appropriate alt attribute or is aria-hidden if decorative; also
consider adding tabindex="-1" on the dialog container so focus can be
programmatically moved into the dialog.
- Around line 85-116: The star rating is currently built from non-focusable
<img> elements (class "star" inside "star-container") so keyboard users cannot
Tab/Enter/Space to set a rating; replace those images with accessible, focusable
controls (preferably an input[type="radio"] group with the same name or button
elements with role="radio") inside the .star-container, give each control a
value and aria-label, mark the container as a radiogroup (role="radiogroup") and
update each option's aria-checked when selected, ensure Enter/Space keyboard
handlers are present and visual focus styles remain, and update
.rate-description and .rate-percentage from the new control change events.
In `@README.md`:
- Around line 19-20: Update README.md to make the UI behavior consistent:
replace any remaining "더 보기 버튼" descriptions in the checklist and requirements
(specifically the checklist items around the same section that reference lines
describing the button and items 74-77) with the infinite-scroll wording used
elsewhere, and remove or refactor the checklist entries that assume a
paging/button flow so the document consistently describes the infinite scroll
behavior and its acceptance criteria.
In `@src/controller/movieController.ts`:
- Around line 51-53: The clickMovie implementation currently finds movies by
title (state.movieList.find(...)) which can misidentify movies with duplicate
titles; change clickMovie to accept and use the movie's unique id (e.g.,
clickMovie: async (id: string) => { ... }) and look up the movie by comparing
movie.id === id, then use selectedMovie.id for subsequent detail/rating lookups;
update any callers to pass the id instead of title and adjust any UI event
handlers that invoke clickMovie so the id (not title) is forwarded.
In `@src/events/bindMovieEvent.ts`:
- Around line 17-20: Wrap the current scroll handler added via
window.addEventListener in a guarded/throttled check so onMore() cannot be
invoked repeatedly: verify or read the in-flight/loading flag used by
loadMoreMovies (or introduce one) and return early if a load is already pending,
or debounce/throttle the handler before calling onMore; add a short comment
explaining the purpose of the magic number (document.body.scrollHeight - 2) or
replace it with a named constant (e.g., BOTTOM_OFFSET) for clarity; and make the
listener removable by returning a cleanup/unsubscribe function or storing the
bound handler so it can be removed with window.removeEventListener during
unmount/navigation to avoid listener accumulation.
In `@src/events/bindRatingEvent.ts`:
- Around line 18-21: The code currently calls onRate(movieId, userRate) after
parsing Number(ratingContainer.getAttribute("data-movie-id")) and
Number(star.getAttribute("value")), which can yield 0 when attributes are
missing; add guards in bindRatingEvent to validate movieId and userRate before
calling onRate: read the attributes from ratingContainer and star, ensure
movieId is a finite positive integer (>0) and userRate is a finite number in the
valid rating range (e.g., 1–5), and return early (do not call onRate) when
either check fails; update references to ratingContainer, star, movieId,
userRate, and onRate accordingly.
In `@src/service/storageUserRate.ts`:
- Around line 8-12: After parsing saved, validate that the result is an
object-shaped "rating map" before returning; replace the current bare return
JSON.parse(saved) with logic that parses saved into a variable (e.g., parsed),
then check parsed is non-null, typeof parsed === "object", not an Array (use
!Array.isArray(parsed)), and optionally verify its values are numbers (e.g.,
Object.values(parsed).every(v => typeof v === "number")). If the checks fail or
an exception is thrown, return {}. Update the code in storageUserRate.ts around
the try/catch handling of saved to implement these validations and the safe
fallback.
In `@src/types.ts`:
- Line 9: The `genres` field in src/types.ts is currently declared as a
single-element tuple (`genres: [{ name: string }]`) but should be a
variable-length array; update the type to an array of genre objects (e.g.,
`genres: { name: string }[]`) and update any code that assumes a
fixed-one-element tuple usage in functions or interfaces referencing `genres` so
it handles multiple items (search for symbols referencing `genres` in your
codebase to adjust iteration/access accordingly); ensure the new type matches
the actual API response shape.
In `@src/view/modalView.ts`:
- Around line 34-41: The modal registers anonymous handlers repeatedly: the
close button handler on modalBackground.querySelector(".close-modal") and the
global addEventListener("keydown") are added each time the modal is shown,
causing handler accumulation; fix by converting those anonymous callbacks into
named handler functions (e.g., onCloseClick, onEscapeKey) and either register
them only once during initialization (not inside showDetailModal) or remove them
inside hideDetailModal via removeEventListener using the same function
references; ensure you reference modalBackground, the ".close-modal" element,
hideDetailModal, and the named handlers so removal or one-time registration
works correctly.
- Around line 18-24: The code calls methods on possibly undefined properties
(movieDetail.release_date.slice, movieDetail.genres.map,
movieDetail.vote_average.toFixed) so the nullish fallback never runs; update the
logic used in the functions that set category.textContent and rate.textContent
to first safely access values (use optional chaining or existence checks on
movieDetail.release_date, movieDetail.genres, movieDetail.vote_average), compute
safe strings/arrays (e.g., year = movieDetail.release_date?.slice(0,4) ?? "개봉 년도
없음"; genresText = movieDetail.genres?.map(g=>g.name).join(", ") ?? "장르 없음";
rateText = movieDetail.vote_average != null ?
movieDetail.vote_average.toFixed(1) : "0"), then assign those computed values to
category.textContent and rate.textContent so no method is called on undefined.
In `@src/view/movieListView.ts`:
- Around line 84-85: The overlay image source is set directly from
movie.backdrop_path which can be null causing broken images; update the logic
around the getElement(".overlay > img", HTMLImageElement) usage to check
movie.backdrop_path and if absent use a safe fallback (e.g., movie.poster_path
or a default placeholder URL) or hide/remove the overlay element instead; ensure
the change happens where img.src is assigned so you never set a URL containing
"null".
In `@src/view/userRatingView.ts`:
- Around line 7-8: Replace the typo in the rating text for the switch branch
handling case 1: change the returned string "최악이예요" to the correct "최악이에요" in
the file's rating text switch (the case 1 return in src/view/userRatingView.ts)
so the modal shows the natural phrasing; also search for any other occurrences
of "최악이예요" and update them to keep strings consistent.
In `@templates/styles/main.css`:
- Around line 89-96: Remove the empty blank line inside the .logo-container rule
to satisfy the stylelint declaration-empty-line-before rule: locate the
.logo-container CSS block and delete the extraneous blank line before the
closing brace so properties (gap: 1.5rem;) are directly followed by the closing
} with no intervening empty line.
In `@templates/styles/mobile.css`:
- Line 56: There is an extra blank line immediately before the declaration "gap:
0.5rem;" which triggers the stylelint rule declaration-empty-line-before; remove
the empty line so the declaration directly follows the previous property or
selector, or if the blank line is intentional, add a localized stylelint
directive (e.g., disable/enable for declaration-empty-line-before) around the
block to preserve formatting without breaking the linter.
In `@templates/styles/modal.css`:
- Around line 84-91: Stylelint flags a declaration-empty-line-before violation
in the CSS rule for .modal-description h2; fix it by inserting a single blank
line between the selector opening and the first declaration (i.e., add an empty
line before "width: 80%;") so the block satisfies the
declaration-empty-line-before rule, and apply the same blank-line fix to the
other similar selector block noted in the comment.
In `@templates/styles/tablet.css`:
- Around line 36-43: The .movie-detail CSS rule contains an extra blank line
before the closing brace which triggers the stylelint
declaration-empty-line-before rule; remove the empty line inside the
.movie-detail block so declarations are contiguous (no stray blank line between
the last declaration and the closing brace) to satisfy stylelint.
In `@templates/styles/thumbnail.css`:
- Around line 6-9: Remove the empty line before the `justify-content`
declaration in the `.thumbnail` grid rules: the block containing
`grid-template-columns: repeat(auto-fit, 200px);` and `gap: 70px;` should have
`justify-content: center;` directly after `gap` with no blank line to satisfy
the `declaration-empty-line-before` Stylelint rule.
---
Outside diff comments:
In `@src/view/movieListView.ts`:
- Around line 14-28: The current code assigns untrusted values (movie.title,
movie.poster_path, movie.vote_average) directly into li.innerHTML which risks
markup breakage and XSS; change the rendering in movieListView.ts to build the
DOM using createElement/appendChild (or setElement.textContent) instead of
innerHTML: create the container div.item, an img element with src set via
setAttribute using a safe URL (use posterError if no poster_path) and alt set
via img.alt or textContent for titles, set the vote-average span textContent to
movie.vote_average ? movie.vote_average.toFixed(1) : "0", and set the title via
textContent on a strong element; this ensures values are escaped and attributes
are assigned safely rather than interpolated into HTML.
---
Nitpick comments:
In `@cypress/e2e/popularPageTest.cy.ts`:
- Around line 30-41: Remove the commented-out test block for "더 보기 버튼을 누르면 다음
페이지 영화가 기존 목록 뒤에 추가된다." (the entire commented cy.get... block) from
popularPageTest.cy.ts to avoid confusion and dead code; if you need to preserve
its intent, move it to an archived notes file or a proper skipped test (e.g.,
use it.skip) but do not leave the large commented test in the spec file.
In `@src/events/bindMovieEvent.ts`:
- Around line 13-16: Remove the commented-out "more" button code block
referencing getElement(".display-more-btn"), the moreBtn variable, and the
onMore() call in bindMovieEvent.ts; since it's obsolete and retrievable from Git
history, delete those three commented lines to clean up the file and leave no
commented dead code behind.
In `@src/service/movieApi.ts`:
- Around line 21-24: The function fetchMovieDetail currently lacks an explicit
return type; add a precise Promise return type (e.g. Promise<Movie> or a new
Promise<MovieDetail>) and update its signature accordingly
(fetchMovieDetail(movieId: number): Promise<...>), creating a MovieDetail
interface if the returned payload differs from Movie, and ensure the call to
request is typed or the response is mapped to that interface so TypeScript
enforces the shape.
In `@src/view/getElementView.ts`:
- Around line 1-17: Remove the commented-out legacy helper functions getElement,
getUListElement, and getInputElement from src/view/getElementView.ts; delete the
entire commented blocks so only active, necessary code remains (rely on Git
history for the old implementations) and ensure no residual commented code or
TODOs referencing those function names are left behind.
In `@templates/styles/main.css`:
- Around line 317-319: 현재 프로젝트에 .error-container 스타일이 분산되어 있어 추적이 어렵습니다;
.error-container 선택자에 정의된 margin-bottom: 5rem을 기존 .error-container 블록으로 옮겨서 중복된
블록을 제거하고 하나의 선언으로 합치세요 (찾을 식별자: .error-container, 속성: margin-bottom).
In `@templates/styles/tablet.css`:
- Around line 12-15: Remove the commented CSS declaration inside
.thumbnail-list: delete the commented-out "grid-template-columns: repeat(3,
200px);" line so the .thumbnail-list block contains only active styles (margin
and gap); if the grid behavior is needed, restore it as an active rule with
proper intent documented near .thumbnail-list or add a brief comment explaining
why it must remain.
🪄 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: 8f7b168a-18b2-44f6-b28e-55bb4d6aec69
📒 Files selected for processing (29)
README.mdcypress/e2e/errorTest.cy.tscypress/e2e/modalTest.cy.tscypress/e2e/popularPageTest.cy.tscypress/e2e/searchPageTest.cy.tscypress/support/modalApi.tscypress/support/movieApi.tscypress/support/types.tsindex.htmlsrc/controller/movieController.tssrc/controller/ratingController.tssrc/events/bindMovieEvent.tssrc/events/bindRatingEvent.tssrc/main.tssrc/service/loadMovies.tssrc/service/movieApi.tssrc/service/storageUserRate.tssrc/types.tssrc/view/getElementView.tssrc/view/modalView.tssrc/view/movieListView.tssrc/view/textView.tssrc/view/userRatingView.tstemplates/styles/index.csstemplates/styles/main.csstemplates/styles/mobile.csstemplates/styles/modal.csstemplates/styles/tablet.csstemplates/styles/thumbnail.css
| it("닫기 버튼 또는 ESC키를 눌러 모달을 닫을 수 있고, 새로고침해도 사용자가 매긴 별점이 유지된다.", () => { | ||
| cy.get(".thumbnail-list li").eq(4).find(".thumbnail").click(); | ||
| cy.wait("@getMovieDetail"); | ||
|
|
||
| cy.get('.star-container .star[value="4"]').click(); | ||
|
|
||
| cy.get(".close-modal").click(); | ||
| cy.get(".modal-background").should("not.have.class", "active"); | ||
|
|
||
| cy.get(".thumbnail-list li").eq(4).find(".thumbnail").click(); | ||
| cy.wait("@getMovieDetail"); | ||
|
|
||
| cy.get(".rate-description").should("have.text", "재미있어요"); | ||
| cy.get(".rate-percentage").should("have.text", "(8/10)"); | ||
|
|
There was a problem hiding this comment.
테스트 시나리오 이름과 검증 내용이 불일치합니다.
케이스명은 “새로고침해도 별점 유지”인데, 본문에는 실제 reload 단계가 없어 “재오픈 시 유지”만 검증하고 있습니다. 새로고침 후에도 로컬 스토리지 복원이 되는지를 명시적으로 검증해 주세요.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@cypress/e2e/modalTest.cy.ts` around lines 34 - 48, The test name claims to
verify persistence across a browser reload but the body only reopens the modal;
update the "it" test (the spec string) or better yet add an explicit reload
step: after clicking ".close-modal" and asserting the modal is closed, call
cy.reload() to simulate a page refresh, then reopen the thumbnail
(cy.get(".thumbnail-list li").eq(4).find(".thumbnail").click()) and wait
"@getMovieDetail" and finally assert ".rate-description" and ".rate-percentage"
still show the saved rating; you can also optionally assert localStorage
contains the expected rating key/value before or after reload to prove
restoration.
| cy.intercept("GET", /\/movie\/\d+\?/, (req) => { | ||
| const id = Number(req.url.split("/movie/")[1].split("?")[0]); | ||
|
|
||
| req.reply({ | ||
| statusCode: 200, | ||
| body: movieDetails[id], | ||
| }); |
There was a problem hiding this comment.
누락된 상세 mock을 200 응답으로 돌려주지 않는 편이 좋습니다.
지금은 movieDetails[id]가 없더라도 200 + undefined body가 내려가서, 실제로는 mock 누락인데 앱 버그처럼 보이는 실패가 납니다. 이 헬퍼에서 없는 ID를 바로 드러내야 테스트 원인을 훨씬 빨리 찾을 수 있습니다.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@cypress/support/modalApi.ts` around lines 20 - 26, 현재 cy.intercept handler가
movieDetails[id]가 없을 때도 200 + undefined body를 반환하므로 mock 누락을 숨깁니다; in the
intercept callback (the cy.intercept handler) check whether movieDetails[id]
exists and if not return a non-200 error response (e.g., 404 with an explanatory
body) or explicitly fail the request via req.reply with an error, otherwise
return the normal 200 body; reference the intercept callback, movieDetails and
req.reply to locate and update the logic.
| <div class="modal-background" id="modalBackground"> | ||
| <div class="modal"> | ||
| <button class="close-modal" id="closeModal"> | ||
| <img src="./templates/images/modal_button_close.png" /> | ||
| </button> |
There was a problem hiding this comment.
모달에 dialog semantics와 닫기 버튼 이름이 빠져 있습니다.
현재 마크업만 보면 보조기기는 이 레이어를 모달 대화상자로 인지하기 어렵고, 닫기 버튼도 이미지뿐이라 용도를 제대로 읽어주지 못할 가능성이 큽니다. 새 핵심 UI인 만큼 dialog 역할과 명확한 접근성 이름은 같이 들어가는 편이 안전합니다.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@index.html` around lines 62 - 66, Wrap the modal markup with proper
ARIA/dialog semantics: add role="dialog" and aria-modal="true" to the element
with id="modal" (or the same container referenced by id="modalBackground"),
ensure the dialog has an accessible name via aria-labelledby pointing to a
visible heading inside the dialog or aria-label on the dialog, and make the
close control with id="closeModal" keyboard-and-screenreader friendly by giving
the button an accessible name (e.g., aria-label="Close" or visible text) and
ensure the <img> inside has an appropriate alt attribute or is aria-hidden if
decorative; also consider adding tabindex="-1" on the dialog container so focus
can be programmatically moved into the dialog.
| <div class="user-rate"> | ||
| <div class="star-container"> | ||
| <img | ||
| src="./templates/images/star_empty.png" | ||
| class="star" | ||
| value="1" | ||
| /> | ||
| <img | ||
| src="./templates/images/star_empty.png" | ||
| class="star" | ||
| value="2" | ||
| /> | ||
| <img | ||
| src="./templates/images/star_empty.png" | ||
| class="star" | ||
| value="3" | ||
| /> | ||
| <img | ||
| src="./templates/images/star_empty.png" | ||
| class="star" | ||
| value="4" | ||
| /> | ||
| <img | ||
| src="./templates/images/star_empty.png" | ||
| class="star" | ||
| value="5" | ||
| /> | ||
| </div> | ||
| <div class="rate-detail"> | ||
| <span class="rate-description"></span> | ||
| <span class="rate-percentage"></span> | ||
| </div> |
There was a problem hiding this comment.
별점 컨트롤이 키보드로는 조작되지 않습니다.
클릭 가능한 <img>만으로 별점을 만들면 Tab/Enter/Space 흐름에서 평점을 줄 수 없습니다. 이 기능은 핵심 사용자 시나리오라서, 최소한 포커스 가능한 버튼이나 라디오 그룹 semantics가 필요합니다.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@index.html` around lines 85 - 116, The star rating is currently built from
non-focusable <img> elements (class "star" inside "star-container") so keyboard
users cannot Tab/Enter/Space to set a rating; replace those images with
accessible, focusable controls (preferably an input[type="radio"] group with the
same name or button elements with role="radio") inside the .star-container, give
each control a value and aria-label, mark the container as a radiogroup
(role="radiogroup") and update each option's aria-checked when selected, ensure
Enter/Space keyboard handlers are present and visual focus styles remain, and
update .rate-description and .rate-percentage from the new control change
events.
| - [x] '더보기' 버튼을 누르면 영화 20개를 추가로 띄운다. [요구사항 변경] | ||
| - [x] 더 보기 기능을 페이징하는 방식에서 무한 스크롤 방식으로 변경한다. |
There was a problem hiding this comment.
문서 기준이 아직 버튼 방식과 무한 스크롤 방식으로 섞여 있습니다.
여기서는 무한 스크롤로 정리됐는데, 같은 README의 Line 66과 Line 72는 아직 "더 보기 버튼" 기준으로 서술돼 있습니다. 요구사항과 테스트 체크리스트가 서로 다른 기준을 가리키지 않도록 한쪽으로 맞춰두는 편이 좋겠습니다.
Also applies to: 74-77
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@README.md` around lines 19 - 20, Update README.md to make the UI behavior
consistent: replace any remaining "더 보기 버튼" descriptions in the checklist and
requirements (specifically the checklist items around the same section that
reference lines describing the button and items 74-77) with the infinite-scroll
wording used elsewhere, and remove or refactor the checklist entries that assume
a paging/button flow so the document consistently describes the infinite scroll
behavior and its acceptance criteria.
| .logo-container { | ||
| display: flex; | ||
| flex-direction: column; | ||
| justify-content: center; | ||
| align-items: center; | ||
|
|
||
| gap: 1.5rem; | ||
| } |
There was a problem hiding this comment.
Stylelint 에러(빈 줄) 수정이 필요합니다.
Line 95 앞 빈 줄로 declaration-empty-line-before 규칙 위반이 발생합니다. 공백 줄 제거로 린트 실패를 해소해 주세요.
🧰 Tools
🪛 Stylelint (17.6.0)
[error] 95-95: Unexpected empty line before declaration (declaration-empty-line-before)
(declaration-empty-line-before)
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@templates/styles/main.css` around lines 89 - 96, Remove the empty blank line
inside the .logo-container rule to satisfy the stylelint
declaration-empty-line-before rule: locate the .logo-container CSS block and
delete the extraneous blank line before the closing brace so properties (gap:
1.5rem;) are directly followed by the closing } with no intervening empty line.
| justify-content: center; | ||
| align-items: center; | ||
|
|
||
| gap: 0.5rem; |
There was a problem hiding this comment.
Stylelint 오류부터 정리하는 편이 좋겠습니다.
Line 56 앞의 빈 줄 때문에 declaration-empty-line-before가 발생하고 있습니다. 새로 추가한 반응형 스타일이라면 린트가 깨지지 않게 맞춰두는 편이 안전합니다.
🧰 Tools
🪛 Stylelint (17.6.0)
[error] 56-56: Unexpected empty line before declaration (declaration-empty-line-before)
(declaration-empty-line-before)
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@templates/styles/mobile.css` at line 56, There is an extra blank line
immediately before the declaration "gap: 0.5rem;" which triggers the stylelint
rule declaration-empty-line-before; remove the empty line so the declaration
directly follows the previous property or selector, or if the blank line is
intentional, add a localized stylelint directive (e.g., disable/enable for
declaration-empty-line-before) around the block to preserve formatting without
breaking the linter.
| .modal-description h2 { | ||
| width: 80%; | ||
| word-break: keep-all; | ||
|
|
||
| font-size: 2rem; | ||
| font-weight: 600; | ||
| line-height: 1.1; | ||
| margin: 0 0 8px; |
There was a problem hiding this comment.
Stylelint 에러(빈 줄 규칙) 수정이 필요합니다.
declaration-empty-line-before 위반이 보고된 위치라 CI 린트 실패 원인이 됩니다.
수정 예시
.modal-description h2 {
width: 80%;
word-break: keep-all;
-
font-size: 2rem;
font-weight: 600;
line-height: 1.1;
margin: 0 0 8px;
}
@@
.user-rate {
display: flex;
-
gap: 1rem;
}Also applies to: 109-113
🧰 Tools
🪛 Stylelint (17.6.0)
[error] 88-88: Unexpected empty line before declaration (declaration-empty-line-before)
(declaration-empty-line-before)
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@templates/styles/modal.css` around lines 84 - 91, Stylelint flags a
declaration-empty-line-before violation in the CSS rule for .modal-description
h2; fix it by inserting a single blank line between the selector opening and the
first declaration (i.e., add an empty line before "width: 80%;") so the block
satisfies the declaration-empty-line-before rule, and apply the same blank-line
fix to the other similar selector block noted in the comment.
| .movie-detail { | ||
| display: flex; | ||
| flex-direction: column; | ||
| justify-content: center; | ||
| align-items: center; | ||
|
|
||
| gap: 0.5rem; | ||
| } |
There was a problem hiding this comment.
Stylelint 경고 구간(빈 줄) 정리가 필요합니다.
Line 42 앞의 빈 줄 때문에 declaration-empty-line-before 규칙 위반이 발생합니다. 해당 공백 줄을 제거해 주세요.
🧰 Tools
🪛 Stylelint (17.6.0)
[error] 42-42: Unexpected empty line before declaration (declaration-empty-line-before)
(declaration-empty-line-before)
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@templates/styles/tablet.css` around lines 36 - 43, The .movie-detail CSS rule
contains an extra blank line before the closing brace which triggers the
stylelint declaration-empty-line-before rule; remove the empty line inside the
.movie-detail block so declarations are contiguous (no stray blank line between
the last declaration and the closing brace) to satisfy stylelint.
| grid-template-columns: repeat(auto-fit, 200px); | ||
| gap: 70px; | ||
|
|
||
| justify-content: center; |
There was a problem hiding this comment.
반응형 그리드 변경은 좋지만, 빈 줄을 제거해주세요.
auto-fit과 justify-content: center 조합으로 반응형 레이아웃이 잘 구현되었습니다. 다만, Stylelint에서 선언문 앞의 빈 줄(declaration-empty-line-before)에 대한 에러가 발생합니다. Line 8의 빈 줄을 제거해주세요.
💅 제안된 수정
.thumbnail-list {
margin: 0 auto 56px;
display: grid;
grid-template-columns: repeat(auto-fit, 200px);
gap: 70px;
-
justify-content: center;
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| grid-template-columns: repeat(auto-fit, 200px); | |
| gap: 70px; | |
| justify-content: center; | |
| grid-template-columns: repeat(auto-fit, 200px); | |
| gap: 70px; | |
| justify-content: center; |
🧰 Tools
🪛 Stylelint (17.6.0)
[error] 9-9: Unexpected empty line before declaration (declaration-empty-line-before)
(declaration-empty-line-before)
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@templates/styles/thumbnail.css` around lines 6 - 9, Remove the empty line
before the `justify-content` declaration in the `.thumbnail` grid rules: the
block containing `grid-template-columns: repeat(auto-fit, 200px);` and `gap:
70px;` should have `justify-content: center;` directly after `gap` with no blank
line to satisfy the `declaration-empty-line-before` Stylelint rule.
There was a problem hiding this comment.
안녕하세요 고생 많으셨어요.
E2E와 Storage 관련해서 고민해보시며 좋을 내용을 코멘트 남겨두었어요.
확인 후 궁금한 점이 있다면 편하게 DM 주세요!
질문에 대해 답변 드릴게요.
1. controller, bind, view 메서드들을 역할에 맞게 분리했습니다. 적절하게 분리가 되었는지, 부족하다면 어떤 부분에서 부족한지
해당 부분 답변을 드리기 전에 질문이 있어요.
피트의 경우 각각 어떤 역할을 기대했고, 각각 어떻게 상호작용 하길 기대했는지? (어디서 데이터와 view, 이벤트를 조합하고 어디서 관리하길 기대했는지) 등이 궁금해요.
그리고 왜 그렇게 나누었는지도 답변해주시면 해당 내용을 참고해서 답변 드려 볼게요.
2. 에러 핸들링에 대한 요구사항이 미션에 존재하는데, api 호출 등이 아닌 모달 show, hide, 별점 기능 등에 에러 핸들링이 필요할까요?
이 부분의 경우 에러 케이스가 발생 할 수 있고, 발생 빈도, 에러가 유저에게 어느정도 크리티컬 한지에 따라 다를 것 같아요.
보통의 경우 모달 자체에서 에러가 나는 경우는 흔치 않고, 별점 기능 또한 API와 연관되어 있기 때문에 API 관련 핸들링에 집중하는 편이에요.
3. 별점 이미지 alt
우선 alt의 경우 반드시 있는게 좋아요.
alt는 접근성과 SEO에 연관된 내용이라 생각해요. 그랬을 때 이미지에 대한 정보보단 점수에 대해 잘 설명하는것이 더 중요하다 생각합니다!
다만 alt 외에 aria-label 등을 잘 챙긴다면 더 좋을 것 같아요.
4. 반응형 디자인
세세하게 모든 반응형을 챙기면 좋겠지만 시간을 무한정 쓸 수 없기 때문에 Break Point를 정해두고 하는 편이에요.
저의 경우 서비스마다 상이하지만 3~4개 정도의 브레이크 포인트를 잡고 있어요. 많이 잡는 사람은 5개 이상도 잡긴 하더라구요.
반대로 극단적으로 Mobile First로 모바일 중심적으로 디자인을 잡기도 해요.
정답은 없지만 이런 상황 속 어떤 기준으로 브레이크 포인트를 넣으면 좋을지 고민해보시고 이에 맞게 선택과 집중을 하시면 좋겠어요!
5. E2E 테스트
E2E 테스트 관련해선 코멘트 남겨 두었어요! 살펴보시고 궁금한 점이 있다면 편하게 이야기해주세요.
6. 학습 방법
학습 방법은 정답이 없지만 피트가 하시는 방법은 좋은 접근이라고 생각해요. 시간이 지나면 까먹을 수 있는데 그 과정을 기록해 두는 것 이니까요!
저는 모르는 것 또는 개념을 발견했을 때 중요도, 상황에 따라 다르게 처리해요.
당장 이 개념을 반드시 알아야 하는 상황이 있고, 상황에 따라선 몰라도 쓸 수 있는 상황들이 있어요.
지금 내가 하려고 하는 것에 이게 어느정도 중요한 것인지? 꼭 알아야 하는 개념인지 고민해본 후 중요한 부분이면 지금 중요한 만큼만 접근해서 공부하는 편이에요. 이후 시간이 지난 후 더 자세히 공부하기도 하고, 애매하게 알게 된 개념은 계속 다른 공부를 하다보면 이제 키워드는 알기 때문에 계속 눈에 보이더라구요..! 그렇게 반복적으로 접하게 된다면 그때 더 깊이있게 공부하기도 해요.
| "영화를 추가로 불러오지 못했습니다.", | ||
| ); | ||
| }); | ||
| it("모달을 띄울 시 영화 상세 정보 요청이 실패하면 상세 정보 에러 메시지를 보여준다.", () => { |
There was a problem hiding this comment.
P3.
사람마다 상이할 수 있으나 에러 케이스만 모아두는 것 보다, 각 페이지별 시나리오로 보아두는게 더 좋을 것 같아요.
현재는 에러 시나리오가 1~2개이지만 이후 에러 케이스가 늘어나면 각 페이지에서 어떤 에러가 발생 할 수 있는지 확인이 어려울 가능성이 높아요.
테스트 코드는 에러 케이스를 사전에 발견하는 목적도 있지만 해당 스펙이 어떻게 동작하는지 흐름을 파악하기 위한 용도로도 사용 될 수 있다는 것을 알고계시면 좋을 것 같아요.
-
별도 에러 파일로 나누면 좋을 내용
- 공통된 에러 케이스 (토큰 만료, not fount 등등)
-
페이지 별로 묶어두면 좋을 내용
- 각 페이지 행동 중 발생 할 수 있는 에러 케이스
| @@ -0,0 +1,13 @@ | |||
| export type MockMovie = { | |||
There was a problem hiding this comment.
P2;
실제 API의 응답을 MOCK 데이터로 활용할 것 같은데요. 별도의 MockMovie 타입을 만드는 이유가 있을까요?
| cy.wait("@getPopularMovies"); | ||
| }); | ||
|
|
||
| it("영화 포스터 또는 제목을 클릭하면 세부 정보 모달을 띄울 수 있고, 별점을 매길 수 있다.", () => { |
There was a problem hiding this comment.
P2;
우선 테스트 시나리오를 유저 행동 기반으로 작성한 점은 정말 좋다고 생각해요.
다만 현재 해당 테스트 시나리오는 한번에 2개를 검증하고 있는데 이렇게 하신 의도가 있을까요?
따로따로 나눠서 해도 되지 않을까 해서요!
만약 별점을 매기기 위해 모달을 반드시 띄워야 했다면 beforEach 등으로 모달을 띄우는 과정을 기본으로 설정해둬도 좋을 것 같아요.
| try { | ||
| await loadMovies({ state, reset: false }); | ||
| if (state.movieList[0]) showBackgroundMovieInfo(state.movieList[0]); | ||
| if (state.movieList[0]) { |
There was a problem hiding this comment.
단순 궁금증.
만약 영화 결과가 빈 값으로 내려오면 어떻게 되나요?
| const Rating_Key = "user-rate"; | ||
|
|
||
| export const loadUserRate = () => { | ||
| const saved = localStorage.getItem(Rating_Key); |
There was a problem hiding this comment.
P2;
우선 localStorage를 직접 호출해서 사용하는 것이 아니라 추상화 해서 사용한 점은 정말 잘하셨다고 생각해요.
이렇게 만들 경우 사용하는 쪽에서 별도로 Rating_Key 를 실수하거나, 데이터를 잘 못 덮어쓰는 경우는 없어지기 때문이에요.
다만 아래의 상황들을 유연하게 대응하기 위해선 어떻게 해볼 수 있을지 고민해보시면 좋겠어요.
(지금 상황에선 피트가 하신 정도로도 충분하다고 생각되나, 공부하는 측면에서 고민해보신다 생각해주셔도 좋아요)
- 현재는 UserRate만 존재하는데 만약 검색어 히스토리 등도 로컬스토리지로 만든다면 어떻게 할 수 있을까?
- 현재는 LocalStorage를 사용하고 있지만, 상황에 따라 Cookie, SessingStorage, 또는 Server Base Storage(Radis, Key Value 기반의 DB) 등을 쉽게 변경 가능하게 해야 한다면? 어떻게 하면 효율적으로 할 수 있을까?
| // }; | ||
|
|
||
| export const getInputElement = (selector: string) => { | ||
| export const getElement = <T extends typeof HTMLElement>( |
| </div> | ||
| </div> | ||
| `; | ||
| // const background = getElement(".background-container", HTMLElement); |
There was a problem hiding this comment.
이 주석 외에도 주석 코드가 많은데 불필요하면 지워도 좋을 것 같아요!
| // 로컬 스토리지에 저장 | ||
| saveUserRate(state.userRating); | ||
|
|
||
| renderUserRate(movieId, userRate); |
There was a problem hiding this comment.
P3;
해당 컨트롤러가 기대하는 역할이 무엇인지 명확하게 해보면 좋을 것 같아요.
제가 코드를 읽었을 때 createRatingController가 정확히 어떤 역할을 하는지 한눈에 잘 들어오지 않는 것 같아요.
initUserRating과 ratingUserRate 이름을 보았을 때 단순히 데이터를 관리하고 불러오는 목적처럼 보이고 실제로 initUserRating에서는 데이터를 가져오는 역할만 해요.
근데 ratingUserRate는 render 역할도 하다보니 예측이 잘 되지 않는 것 같아요.
우선 열린 질문으로 해당 컨트롤러는 어떤 역할을 기대했고, 어떻게 관리하는게 좋을 것 같은지, 이유가 무엇인지 고민해보시면 좋겠어요!
2 participants
🎯 미션 소개
🕵️ 셀프 리뷰(Self-Review)
제출 전 체크 리스트
리뷰 요청 & 논의하고 싶은 내용
안녕하세요 루트! 2단계도 잘 부탁드립니다~😊 제가 이번 미션에서는 최대한 리뷰어님에게 좋은 것들을 얻어가고자 핑퐁을 많이 하려고 노력하는 것 같습니다. 그러나 이런 모습에서 너무 급하게 PR을 제출한 것은 아닌가? 라고 생각이 드신다면 더 생각하고 PR을 날려도 된다고 말해주시면 정말 감사하겠습니다!
1) 이번 단계에서 가장 많이 고민했던 문제와 해결 과정에서 배운 점
bindMovieEvents()메서드에 존재하지만, 모달을 닫는 이벤트를bindMovieEvents()메서드가 아닌 모달을 렌더링하는 메서드 안에서 추가했습니다. 처음에는 일관성을 지키는 것이 중요하다고 생각해서 bind 메서드 내부에 선언하려고 했지만, 너무 가벼운 로직이므로 오히려 bind 내부에서 선언하고 끌고와서 핸들러를 인자로 주는 것 자체가 비용이 더 크다고 생각했습니다.2) 이번 리뷰를 통해 논의하고 싶은 부분
요구사항이 늘어나면서 기존에 한 파일에 있던 controller, bind, view 메서드들을 역할에 맞게 분리했습니다. 적절하게 분리가 되었는지, 부족하다면 어떤 부분에서 부족한지 피드백해주시면 감사하겠습니다.
에러 핸들링에 대한 요구사항이 미션에 존재하는데, api 호출 등이 아닌 모달 show, hide, 별점 기능 등에 에러 핸들링이 필요할까요?
모달에서 사용자의 별점과 별 이미지를 렌더링하는데, 만약 별 이미지가 경로가 잘못되거나 파일이 깨지는 경우까지도 고려해서 alt를 설정해야 할까요? 별점의 별 이미지 각각이 alt인 것이 의미가 있나 싶어서 여쭤봅니다. (별 이미지가 아닌 경우에도 img 태그는 alt가 무조건 있는 편이 좋을까요? 있다면 대체 이미지로 해야 할까요? 아니면 단순히 문자열로 해야 할까요?)
제가 생각한 반응형 디자인이란, 모든 가로 사이즈에도 어색하지 않도록 margin과 padding같은 레이아웃 속성에 auto를 적극적으로 활용하는 디자인이었습니다. 하지만 미션 요구사항 중 하나인 반응형 디자인 및 figma 디자인에는 데스크톱 화면(가로에 영화 5개)/태블릿 화면(가로에 영화 3개)/모바일 화면(가로에 영화 1개)로 나와있습니다. 그래서 처음에는 figma 디자인에 따라 5/3/1로 구현했고, 생각을 바꿔서 5/4/3/2/1이 가능하도록 구현했습니다. 이런 경우에는 5/4/3/2/1과 같이 모든 경우를 신경써야 하는 것이 맞는지, 아니면 5/3/1로 크게 기기 환경으로 구분해서 몇 가지만 신경쓰면 되는지 궁금합니다!
요구사항이 추가되면서 E2E 테스트 명세도 변경되었는데요, 그 중 세부정보 모달 테스트의 경우 별점 기능과 나누기 애매하다고 판단해서 둘을 한번의 흐름으로 잡고 묶어서 테스트를 진행했습니다. 제가 결정한 이 방식에 확신이 들지 않아서, 적절하게 테스트를 작성했는지 궁금합니다!
저는 미션을 하다가 모르는 것이 생기면 일단 노션에 바로 적어놓고, 미션을 이어나가기 전에 간단하게 내가 왜 몰랐는지, 어떻게 해결할 수 있는지, AI는 뭐라고 했는지 등을 적어놓는 습관을 기르고 있습니다.(모르는 것이 생겼지만 미션의 흐름이 중요하다면 일단 모르는 것 제목과 나의 질문만 적어놓습니다) 루트는 미션(or 프로젝트)을 진행하다가 아예 모르는 것이 나타나면(문법, 처음 보는 기술, 단어 등) 혼자 어떤 과정을 거쳐서 이해하고 적용하는지 그 흐름을 알고 싶습니다!
✅ 리뷰어 체크 포인트