[2단계 - 상세 정보 & UI/UX 개선하기] 레스 미션 제출합니다. #312
Open
lee-eojin wants to merge 40 commits intowoowacourse:lee-eojinfrom
Open
[2단계 - 상세 정보 & UI/UX 개선하기] 레스 미션 제출합니다. #312lee-eojin wants to merge 40 commits intowoowacourse:lee-eojinfrom
lee-eojin wants to merge 40 commits intowoowacourse:lee-eojinfrom
Conversation
- CSS :has() 기반 hover로 JS inline style 제거 - background-size 주석 추가 - MOVIE_SELECTED disconnect 주석 추가
Walkthrough이 PR은 영화 애플리케이션을 step2 기능으로 확장합니다. 모달을 통한 영화 상세 보기, 별점(2-10점) 저장 및 로드 기능, 무한 스크롤 구현을 추가합니다. API 레이어를 Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 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: 17
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@__tests__/api.test.ts`:
- Around line 19-20: The tests currently call vi.restoreAllMocks() in afterEach
but do not undo globals created with vi.stubGlobal(), which can leave stale
global stubs like fetch between tests; update the afterEach cleanup to also call
vi.unstubAllGlobals() (or the appropriate vitest API/setting for global
teardown) so any vi.stubGlobal('fetch', ...) created in tests is removed after
each test; locate the afterEach block in __tests__/api.test.ts and add the
unstub call alongside vi.restoreAllMocks() or configure vitest to automatically
unstub globals if preferred.
In `@cypress/e2e/spec.cy.ts`:
- Around line 52-55: The spinner assertion is flaky because the real detail API
can return before Cypress sees the spinner; update the test "카드 클릭 시 스피너가 표시되다가
상세 정보로 전환된다" to intercept the detail API call (using cy.intercept, alias it, and
inject a deliberate network delay or use a stubbed delayed response), then click
the first ".thumbnail-list li", assert ".spinner" is visible while the
intercepted request is pending, call cy.wait('@yourAlias') to let the response
complete, and finally assert ".modal-description" is visible and ".spinner" is
gone.
In `@cypress/tsconfig.json`:
- Line 7: The TypeScript errors come from removing "cypress" from the "types"
array in cypress/tsconfig.json so Cypress global types (cy, Cypress, etc.) are
no longer available; restore "cypress" to the "types" array in
cypress/tsconfig.json (e.g., change "types": ["node"] to include "cypress") OR
alternatively add a triple-slash reference /// <reference types="cypress" /> at
the top of cypress/e2e/spec.cy.ts so that cy.visit(), cy.get(), and other
Cypress globals resolve; update whichever file (cypress/tsconfig.json or
spec.cy.ts) contains the project-wide type config or the test file respectively.
In `@README.md`:
- Around line 69-74: The fenced code block showing sprite notes lacks a language
tag and triggers markdownlint MD040; update that block in README.md (the
triple-backtick block containing "stars_sprite.png: [ 빈 별 | 채운 별 ]" and the
background-size/position lines) by adding an appropriate language identifier
(e.g., use "text" for plain explanation or "css" if you want to indicate styling
snippets) after the opening ``` so the linter recognizes the block type.
In `@src/dom.ts`:
- Around line 1-3: The current exports mainTitle, sentinel, and header cast
querySelector results with as HTMLElement which masks null and can cause runtime
errors (e.g., sentinel passed to ScrollObserver.observe in
registerSubscriptions). Replace these unsafe casts with a small helper (e.g.,
getRequiredElement(selector): HTMLElement) that calls document.querySelector,
throws a clear Error if null, and use it to initialize mainTitle, sentinel, and
header so missing DOM elements fail fast and remain type-safe; update any usages
(like registerSubscriptions/ScrollObserver) to rely on the guaranteed non-null
types.
In `@src/features/api/fetchApi.ts`:
- Around line 6-15: The request function currently only normalizes HTTP error
responses and lets fetch rejections (offline/CORS/DNS) bubble up; wrap the await
fetch(url) call in a try/catch inside request and convert any thrown/network
error into a normalized ApiError (or a suitable subclass) so the UI sees
consistent app-level messages; preserve the original error (e.g., as the message
or cause) when constructing ApiError and keep existing handling for response.ok
and status checks (UnauthorizedError, NotFoundError, TooManyRequestsError,
ServiceUnavailableError) intact.
In `@src/features/handler/controllerHandlers.ts`:
- Around line 47-50: The current handleMore() increments state.page before
calling loadMore, which advances the global page even if the network call fails;
change handleMore to compute a local nextPage (e.g., const nextPage = state.page
+ 1), call await loadMore(nextPage, state.searchQuery), and only assign
state.page = nextPage after loadMore resolves successfully so failed requests
don't skip pages; update the handleMore function accordingly, referencing
handleMore, state.page, loadMore, and state.searchQuery.
- Around line 17-25: The bug is that LAST_PAGE_REACHED is only emitted in
loadMore, so initial loads (loadPopular and the search equivalent) that already
hit the last page keep the intersection observer alive and trigger an extra
request; update the initial loaders (exported function loadPopular and the
analogous search loader used in lines 37-45, 52-58) to perform the same
last-page check as loadMore and publish APP_EVENTS.LAST_PAGE_REACHED when
data.page >= data.total_pages (or whatever pagination fields your MovieResponse
uses), so subscriptions.ts can unsubscribe immediately; keep the existing
APP_EVENTS.LOAD_START/ MOVIES_LOADED/ERROR behavior intact.
In `@src/features/rating/LocalStorageRatingRepository.ts`:
- Around line 10-12: The load method in LocalStorageRatingRepository should
normalize corrupted stored values to null: after reading localStorage via
getItem(`${RATING_KEY_PREFIX}${movieId}`) and converting with Number(value),
validate that the result is a finite number (and optionally within your valid
rating range, e.g. 0–5); if the converted value is NaN/invalid/out-of-range
return null, otherwise return the numeric rating (so code paths like load(...)
and downstream consumers such as StarRating.ts get either a valid number or
null).
In `@src/features/ui/Header.ts`:
- Around line 13-15: 현재 logoEl.addEventListener(..., { once: true })로 클릭 핸들러가 한
번만 실행되어 로고 클릭 후 요청이 실패하면 subscriptions.ts의 ERROR 흐름에서 헤더를 재렌더링하지 않아 핸들러만 제거된 채
동일 DOM이 남아 두 번째 클릭부터 홈 복귀가 안 됩니다; 해결 방법은 logoEl의 클릭 리스너에서 { once: true }를 제거하거나,
APP_EVENTS.LOGO_CLICK를 publish할 때 실패 시 subscriptions.ts의 ERROR 처리에서 헤더(또는
logoEl)의 핸들러를 재등록하도록 수정하여 logoEl 및 eventBus.publish(APP_EVENTS.LOGO_CLICK, ...)
동작이 실패 상황에서도 반복 클릭 가능하도록 보장하세요.
In `@src/features/ui/Modal.ts`:
- Around line 26-27: The fill method assigns untrusted API strings directly via
this.dialog.innerHTML = this.render(movie), exposing XSS through movie.title,
movie.overview, movie.genres (also used in the template between lines 48-79);
change Modal.fill/Modal.render to avoid raw innerHTML by either constructing the
DOM with createElement/textContent or by passing values through a sanitizer
(e.g., DOMPurify) before insertion, ensuring all movie.* fields are inserted as
text nodes or sanitized strings rather than interpolated HTML.
- Around line 21-24: openWithLoading() sets the loading markup via
this.renderLoading() and calls this.dialog.showModal() but doesn't attach the
close button handler (the same bug exists for the other loading/preview method
around lines 37-45); fix by invoking the same handler-binding routine used at
lines 29-31 (or extract that logic into a new
attachCloseHandler/initializeDialogControls method) immediately after setting
this.dialog.innerHTML (and before showModal()), so the close button in the
loading markup is wired up even while waiting for the detailed response.
In `@src/styles/main.css`:
- Around line 201-207: The .top-rated-movie rule uses fixed absolute top/left
values that don't respond to the same breakpoints you adjust the
background/typography for; change this by either (A) removing absolute
positioning and placing .top-rated-movie inside its container flow (use
position: relative on the container and margin/padding for placement) or (B)
keep absolute but add matching media queries (e.g., `@media` (max-width:800px) and
other breakpoints you use for the background/typography) that override
top/left/z-index to align with the scaled background; update the CSS where
.top-rated-movie is defined and ensure its parent container has a positioning
context if you use relative positioning.
In `@src/styles/modal.css`:
- Around line 161-163: The rule .star-list input[type="radio"] currently uses
display: none which removes the radios from the accessibility tree; change the
hiding strategy to a visually-hidden approach that keeps the inputs focusable
and screen-reader accessible (e.g., replace display:none with a pattern that
positions the input off-screen or clips it while retaining focusability/keyboard
interaction). Update the CSS for .star-list input[type="radio"] to use the
visually-hidden technique (position/size/overflow/clip or clip-path and still
tabbable) so the visual radio is hidden but the underlying control remains
reachable by keyboard and screen readers, and ensure any adjacent .star label
styles continue to show the selected state.
In `@src/styles/thumbnail.css`:
- Around line 27-35: The small-mobile overflow happens because .thumbnail-list
uses fixed 200px columns and .movie-section uses width: fit-content, causing
horizontal scroll between ~391–452px; fix by either expanding the single-column
breakpoint (change the `@media` threshold from 390px to 452px) or, better, make
the grid responsive (replace grid-template-columns: repeat(2, 200px) / repeat(1,
200px) with a fluid rule like repeat(auto-fit, minmax(150px, 1fr)) on
.thumbnail-list) and also ensure .movie-section no longer forces fit-content
(use max-width: 100% or remove fit-content) so the layout can shrink without
horizontal overflow.
In `@src/subscriptions.ts`:
- Around line 62-64: 현재 MODAL_CLOSED 이벤트 핸들러는 항상 scrollObserver.observe()를 호출해
LAST_PAGE_REACHED 이후에도 sentinel 감시를 재개하므로, 마지막 페이지 도달 여부를 추적하는 플래그(예:
isLastPageReached) 또는 기존 상수 LAST_PAGE_REACHED 상태를 활용해 이벤트 핸들러에서 재등록을 조건부로 수행하도록
변경하세요; 구체적으로 eventBus.subscribe(APP_EVENTS.MODAL_CLOSED, ...) 내부에서 현재 마지막 페이지
여부를 확인하고 false일 때만 scrollObserver.observe()를 호출하도록 수정하고, 마지막 페이지 상태를 갱신하는 로직(페이지
로드 완료를 처리하는 함수 또는 scrollObserver callback)에 isLastPageReached를 설정하도록 연동하세요.
- Around line 66-69: The current APP_EVENTS.ERROR handler in
eventBus.subscribe(...) indiscriminately clears the list and shows the error via
movieListView, which hides the list when loadMovieDetail() publishes an error;
change the flow so list vs modal errors are distinguishable: either (a) have
loadMovieDetail() publish a distinct event (e.g., APP_EVENTS.DETAIL_ERROR) and
add a separate subscriber that shows modal-specific errors without calling
movieListView.clearList(), or (b) include a context/source flag in the error
payload (e.g., message.source === 'detail') and update the APP_EVENTS.ERROR
handler to only call scrollObserver.disconnect(), movieListView.clearList(), and
movieListView.showError(message) when message.source !== 'detail' (for detail
errors keep the list intact and let the detail handler render the modal error).
Ensure you update loadMovieDetail() and the event subscription accordingly.
🪄 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: 4dc49fba-2ef9-44c7-abc8-a1ff7702e1ed
⛔ Files ignored due to path filters (1)
src/images/stars_sprite.pngis excluded by!**/*.png
📒 Files selected for processing (33)
.github/workflows/deploy.yml.prettierrcREADME.md__tests__/api.test.tscypress/e2e/spec.cy.tscypress/tsconfig.jsonindex.htmlsrc/constants/image.tssrc/constants/path.tssrc/dom.tssrc/errors.tssrc/features/api/fetchApi.tssrc/features/api/fetchMoviesApi.tssrc/features/handler/controllerHandlers.tssrc/features/handler/dataHandlers.tssrc/features/rating/IRatingRepository.tssrc/features/rating/LocalStorageRatingRepository.tssrc/features/ui/Header.tssrc/features/ui/Modal.tssrc/features/ui/MovieCard.tssrc/features/ui/MovieList.tssrc/features/ui/ScrollObserver.tssrc/features/ui/StarRating.tssrc/init.tssrc/main.tssrc/pubsub/AppEvents.tssrc/router.tssrc/styles/main.csssrc/styles/modal.csssrc/styles/thumbnail.csssrc/subscriptions.tssrc/utils/dom.tstypes/types.ts
💤 Files with no reviewable changes (3)
- src/utils/dom.ts
- src/features/api/fetchMoviesApi.ts
- src/init.ts
Comment on lines
19
to
20
| afterEach(() => { | ||
| vi.restoreAllMocks(); |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Expectation:
# - Either a Vitest config with `unstubGlobals: true`
# - Or an explicit `vi.unstubAllGlobals()` in test cleanup
rg -n 'stubGlobal\("fetch"|unstubAllGlobals\(|unstubGlobals' .Repository: woowacourse/javascript-movie-review
Length of output: 728
🏁 Script executed:
# Check the full afterEach block and surrounding context
cat -n __tests__/api.test.ts | sed -n '15,30p'Repository: woowacourse/javascript-movie-review
Length of output: 594
🏁 Script executed:
# Search for Vitest configuration files
fd -e ts -e js -e json '(vitest|vite)\.config' . -type fRepository: woowacourse/javascript-movie-review
Length of output: 249
🏁 Script executed:
# Check package.json for Vitest version and any config there
cat package.json | grep -A 20 -B 5 'vitest\|vite'Repository: woowacourse/javascript-movie-review
Length of output: 1205
🏁 Script executed:
# Correct fd command to find config files
find . -maxdepth 2 -type f \( -name 'vitest.config.*' -o -name 'vite.config.*' \)Repository: woowacourse/javascript-movie-review
Length of output: 61
🏁 Script executed:
# Also check if config is inline in package.json
cat package.json | grep -A 50 '"vitest"'Repository: woowacourse/javascript-movie-review
Length of output: 982
🏁 Script executed:
# Final check: ensure no unstubAllGlobals calls anywhere in the test file
rg 'unstubAllGlobals' __tests__/api.test.tsRepository: woowacourse/javascript-movie-review
Length of output: 61
🏁 Script executed:
# Check the entire afterEach and surrounding test structure for any cleanup patterns
cat -n __tests__/api.test.ts | head -80Repository: woowacourse/javascript-movie-review
Length of output: 2520
vi.stubGlobal()의 정리 방법을 재검토해 주세요.
vi.restoreAllMocks()는 mock과 spy를 복원하지만, vi.stubGlobal()로 생성한 전역 스텁은 별도로 정리해야 합니다.
다음 점들을 확인해 보세요:
- Vitest 문서에서 전역 스텁(global stubs) 정리 방법을 찾아보기
vi.unstubAllGlobals()가 필요한지, 아니면 vitest 설정에서 지원하는 옵션이 있는지 확인
현재 코드에서는 각 테스트마다 fetch를 새로 stubGlobal하고 있는데, 이전 테스트의 stub이 남아있으면 테스트 실행 순서에 따라 예상치 못한 동작이 발생할 수 있습니다.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@__tests__/api.test.ts` around lines 19 - 20, The tests currently call
vi.restoreAllMocks() in afterEach but do not undo globals created with
vi.stubGlobal(), which can leave stale global stubs like fetch between tests;
update the afterEach cleanup to also call vi.unstubAllGlobals() (or the
appropriate vitest API/setting for global teardown) so any
vi.stubGlobal('fetch', ...) created in tests is removed after each test; locate
the afterEach block in __tests__/api.test.ts and add the unstub call alongside
vi.restoreAllMocks() or configure vitest to automatically unstub globals if
preferred.
Comment on lines
+52
to
+55
| it("카드 클릭 시 스피너가 표시되다가 상세 정보로 전환된다", () => { | ||
| cy.get(".thumbnail-list li").first().click(); | ||
| cy.get(".spinner").should("be.visible"); | ||
| cy.get(".modal-description").should("be.visible"); |
There was a problem hiding this comment.
스피너 검증이 비결정적이라 플래키해질 수 있습니다.
상세 API 응답이 빠르면 .spinner가 보였다가 사라지는 구간을 Cypress가 놓쳐서 이 테스트가 간헐적으로 실패할 수 있습니다. 이 케이스는 상세 요청을 intercept해서 의도적으로 지연시키고, 응답 전/후 상태를 나눠서 검증하는 쪽이 더 안정적입니다.
🤖 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 52 - 55, The spinner assertion is flaky
because the real detail API can return before Cypress sees the spinner; update
the test "카드 클릭 시 스피너가 표시되다가 상세 정보로 전환된다" to intercept the detail API call
(using cy.intercept, alias it, and inject a deliberate network delay or use a
stubbed delayed response), then click the first ".thumbnail-list li", assert
".spinner" is visible while the intercepted request is pending, call
cy.wait('@yourAlias') to let the response complete, and finally assert
".modal-description" is visible and ".spinner" is gone.
| "types": ["node", "cypress"], | ||
| "module": "node16", | ||
| "moduleResolution": "node16", | ||
| "types": ["node"], |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "[1] cy/Cypress 사용 위치"
rg -n --type ts '\b(cy|Cypress)\b' cypress || true
echo
echo "[2] cypress 타입 참조(파일별)"
rg -n --type ts 'reference types="cypress"' cypress || true
echo
echo "[3] 각 spec/support 파일의 상단 참조 확인"
fd -e ts . cypress | sortRepository: woowacourse/javascript-movie-review
Length of output: 4760
🏁 Script executed:
cat -n cypress/tsconfig.jsonRepository: woowacourse/javascript-movie-review
Length of output: 593
🏁 Script executed:
# Check if spec.cy.ts has any imports or references that might provide Cypress types
head -20 cypress/e2e/spec.cy.tsRepository: woowacourse/javascript-movie-review
Length of output: 573
🏁 Script executed:
# Check if e2e.ts imports or references anything
head -20 cypress/support/e2e.tsRepository: woowacourse/javascript-movie-review
Length of output: 746
spec.cy.ts에 Cypress 타입 참조가 없어 타입 에러 발생
Line 7에서 types에서 cypress를 제거하면, cy 메서드를 사용하는 cypress/e2e/spec.cy.ts가 TypeScript 오류를 일으킵니다. spec.cy.ts는 /// <reference types="cypress" />가 없고, commands.ts의 타입 참조는 해당 파일에만 적용되기 때문입니다.
spec.cy.ts에서 cy.visit(), cy.get() 등이 수십 줄에 걸쳐 사용되고 있으므로, 다음 중 하나의 해결책이 필요합니다:
- tsconfig.json에 cypress 타입 유지 (권장):
- "types": ["node"],
+ "types": ["node", "cypress"],- 또는
spec.cy.ts상단에 개별 참조 추가 (파일마다 반복 필요):
+/// <reference types="cypress" />
describe("초기 진입", () => {📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| "types": ["node"], | |
| "types": ["node", "cypress"], |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@cypress/tsconfig.json` at line 7, The TypeScript errors come from removing
"cypress" from the "types" array in cypress/tsconfig.json so Cypress global
types (cy, Cypress, etc.) are no longer available; restore "cypress" to the
"types" array in cypress/tsconfig.json (e.g., change "types": ["node"] to
include "cypress") OR alternatively add a triple-slash reference /// <reference
types="cypress" /> at the top of cypress/e2e/spec.cy.ts so that cy.visit(),
cy.get(), and other Cypress globals resolve; update whichever file
(cypress/tsconfig.json or spec.cy.ts) contains the project-wide type config or
the test file respectively.
Comment on lines
69
to
+74
| ``` | ||
| main.ts DOM 이벤트 바인딩만 | ||
| ├─ init.ts EventBus 구독 등록 + UI 업데이트 | ||
| └─ controllerHandlers state 조작 + 이벤트 publish | ||
| └─ dataHandlers API 호출 (read*) | ||
| └─ fetchMoviesApi fetch + status 검증 + 커스텀 에러 throw | ||
| └─ errors.ts ApiError / UnauthorizedError / NotFoundError | ||
|
|
||
| pubsub/ | ||
| ├─ EventBus.ts subscribe / publish 메커니즘 | ||
| └─ AppEvents.ts 이벤트 상수 + payload 타입 매핑 | ||
|
|
||
| features/ui/ 컴포넌트 (Header, MovieList, MovieCard, MovieSkeleton) | ||
| state.ts 매 호출마다 바뀌는 값 (page, searchQuery) | ||
| stars_sprite.png: [ 빈 별 | 채운 별 ] | ||
| background-size: 200% 100% → 한 번에 절반만 보이게 | ||
| background-position: 0% → 빈 별 | ||
| background-position: 100% → 채운 별 | ||
| ``` |
There was a problem hiding this comment.
코드 펜스 언어를 지정해 주세요.
Line 69-74의 fenced code block은 언어가 빠져 있어서 markdownlint(MD040)에 걸립니다. 설명용이면 text, 스타일 예시면 css처럼 명시해 두는 편이 좋습니다.
🧰 Tools
🪛 markdownlint-cli2 (0.22.0)
[warning] 69-69: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@README.md` around lines 69 - 74, The fenced code block showing sprite notes
lacks a language tag and triggers markdownlint MD040; update that block in
README.md (the triple-backtick block containing "stars_sprite.png: [ 빈 별 | 채운 별
]" and the background-size/position lines) by adding an appropriate language
identifier (e.g., use "text" for plain explanation or "css" if you want to
indicate styling snippets) after the opening ``` so the linter recognizes the
block type.
Comment on lines
+1
to
+3
| export const mainTitle = document.querySelector(".main-title") as HTMLElement; | ||
| export const sentinel = document.querySelector(".scroll-sentinel") as HTMLElement; | ||
| export const header = document.querySelector(".background-container") as HTMLElement; |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
find . -type f -name "dom.ts" -o -name "subscriptions.ts" | head -20Repository: woowacourse/javascript-movie-review
Length of output: 114
🏁 Script executed:
git ls-files | grep -E "(dom|subscription)" | head -20Repository: woowacourse/javascript-movie-review
Length of output: 110
🏁 Script executed:
cat -n src/dom.tsRepository: woowacourse/javascript-movie-review
Length of output: 347
🏁 Script executed:
cat -n src/subscriptions.tsRepository: woowacourse/javascript-movie-review
Length of output: 2706
🏁 Script executed:
find . -type f -name "ScrollObserver*" -o -type f -name "*Observer*" | grep -i scrollRepository: woowacourse/javascript-movie-review
Length of output: 114
🏁 Script executed:
find . -type f \( -name "*.html" -o -name "index.*" \) | head -10Repository: woowacourse/javascript-movie-review
Length of output: 114
🏁 Script executed:
cat -n src/features/ui/ScrollObserver.tsRepository: woowacourse/javascript-movie-review
Length of output: 833
🏁 Script executed:
cat -n index.htmlRepository: woowacourse/javascript-movie-review
Length of output: 1955
querySelector 실패 시 타입 안전성이 우회되어 런타임 에러 위험이 있습니다.
Line 1~3의 as HTMLElement 캐스트는 querySelector()가 반환할 수 있는 null을 타입 시스템에서 숨깁니다. 현재 HTML에 해당 요소들이 존재하더라도, 선택자 변경이나 마크업 구조 변동 시 null이 런타임까지 전파되어 앱이 예상치 못한 위치에서 깨집니다. 특히 sentinel은 registerSubscriptions()에서 즉시 ScrollObserver에 주입되어 observe() 호출 시 오류가 발생합니다.
필수 DOM 요소 검증 함수를 별도로 분리하여, 선택자 조회 실패 시 명확한 타입 안전성과 초기화 단계에서의 명시적 에러를 확보하는 방안을 검토해 보세요.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/dom.ts` around lines 1 - 3, The current exports mainTitle, sentinel, and
header cast querySelector results with as HTMLElement which masks null and can
cause runtime errors (e.g., sentinel passed to ScrollObserver.observe in
registerSubscriptions). Replace these unsafe casts with a small helper (e.g.,
getRequiredElement(selector): HTMLElement) that calls document.querySelector,
throws a clear Error if null, and use it to initialize mainTitle, sentinel, and
header so missing DOM elements fail fast and remain type-safe; update any usages
(like registerSubscriptions/ScrollObserver) to rely on the guaranteed non-null
types.
Comment on lines
+201
to
+207
| .top-rated-movie { | ||
| position: absolute; | ||
| top: 181px; | ||
| left: 90px; | ||
| z-index: 2; | ||
| user-select: none; | ||
| } |
There was a problem hiding this comment.
.top-rated-movie 위치가 반응형 높이와 분리돼 있습니다.
Line 201-204의 고정 top/left 값은 Line 40-59에서 배경 높이와 타이포를 줄여도 함께 조정되지 않습니다. 800px 이하에서 텍스트 블록이 배경 기준선과 어긋나거나 아래로 밀릴 가능성이 커서, 이 위치도 breakpoint별로 같이 조정하거나 컨테이너 흐름에 묶는 편이 안전합니다.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/styles/main.css` around lines 201 - 207, The .top-rated-movie rule uses
fixed absolute top/left values that don't respond to the same breakpoints you
adjust the background/typography for; change this by either (A) removing
absolute positioning and placing .top-rated-movie inside its container flow (use
position: relative on the container and margin/padding for placement) or (B)
keep absolute but add matching media queries (e.g., `@media` (max-width:800px) and
other breakpoints you use for the background/typography) that override
top/left/z-index to align with the scaled background; update the CSS where
.top-rated-movie is defined and ensure its parent container has a positioning
context if you use relative positioning.
Comment on lines
+161
to
+163
| .star-list input[type="radio"] { | ||
| display: none; | ||
| } |
There was a problem hiding this comment.
별점 라디오를 display: none으로 숨기면 접근성이 깨집니다.
Line 161-162처럼 숨기면 라디오가 포커스를 받을 수 없어 키보드와 스크린리더로는 별점 입력을 완료할 수 없습니다. 시각적으로만 숨기고, 실제 컨트롤은 포커스 가능한 상태로 남겨 주세요.
접근성 유지용 숨김 방식 예시
.star-list input[type="radio"] {
- display: none;
+ position: absolute;
+ width: 1px;
+ height: 1px;
+ margin: -1px;
+ padding: 0;
+ overflow: hidden;
+ clip: rect(0, 0, 0, 0);
+ white-space: nowrap;
+ border: 0;
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| .star-list input[type="radio"] { | |
| display: none; | |
| } | |
| .star-list input[type="radio"] { | |
| position: absolute; | |
| width: 1px; | |
| height: 1px; | |
| margin: -1px; | |
| padding: 0; | |
| overflow: hidden; | |
| clip: rect(0, 0, 0, 0); | |
| white-space: nowrap; | |
| border: 0; | |
| } |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/styles/modal.css` around lines 161 - 163, The rule .star-list
input[type="radio"] currently uses display: none which removes the radios from
the accessibility tree; change the hiding strategy to a visually-hidden approach
that keeps the inputs focusable and screen-reader accessible (e.g., replace
display:none with a pattern that positions the input off-screen or clips it
while retaining focusability/keyboard interaction). Update the CSS for
.star-list input[type="radio"] to use the visually-hidden technique
(position/size/overflow/clip or clip-path and still tabbable) so the visual
radio is hidden but the underlying control remains reachable by keyboard and
screen readers, and ensure any adjacent .star label styles continue to show the
selected state.
Comment on lines
+27
to
+35
| @media screen and (max-width: 800px) { | ||
| .thumbnail-list { | ||
| grid-template-columns: repeat(2, 200px); | ||
| } | ||
| } | ||
|
|
||
| @media screen and (max-width: 800px) { | ||
| @media screen and (max-width: 390px) { | ||
| .thumbnail-list { | ||
| grid-template-columns: repeat(2, 1fr); | ||
| grid-template-columns: repeat(1, 200px); |
There was a problem hiding this comment.
작은 모바일 구간에서 가로 오버플로가 발생합니다.
여기서는 800px 이하에서도 2열을 유지해서 최소 453px 폭이 필요합니다. 그래서 391~452px 뷰포트에서는 .movie-section { width: fit-content; }와 합쳐져 가로 스크롤이 생깁니다. 단일 컬럼 전환 지점을 더 넓히거나, 열 너비를 유동적으로 가져가는 쪽으로 다시 확인해 보세요.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/styles/thumbnail.css` around lines 27 - 35, The small-mobile overflow
happens because .thumbnail-list uses fixed 200px columns and .movie-section uses
width: fit-content, causing horizontal scroll between ~391–452px; fix by either
expanding the single-column breakpoint (change the `@media` threshold from 390px
to 452px) or, better, make the grid responsive (replace grid-template-columns:
repeat(2, 200px) / repeat(1, 200px) with a fluid rule like repeat(auto-fit,
minmax(150px, 1fr)) on .thumbnail-list) and also ensure .movie-section no longer
forces fit-content (use max-width: 100% or remove fit-content) so the layout can
shrink without horizontal overflow.
Comment on lines
+62
to
+64
| eventBus.subscribe(APP_EVENTS.MODAL_CLOSED, () => { | ||
| scrollObserver.observe(); | ||
| }); |
There was a problem hiding this comment.
마지막 페이지 이후에도 observer가 다시 살아납니다.
Line 62-64는 모달이 닫힐 때마다 항상 observe()를 호출합니다. LAST_PAGE_REACHED 이후 모달을 한 번 열고 닫으면 sentinel 감시가 다시 시작돼 추가 페이지 요청이 재개됩니다. 마지막 페이지 여부를 따로 기억하고, 닫힘 시 재등록 조건에 포함시키는 쪽이 안전합니다.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/subscriptions.ts` around lines 62 - 64, 현재 MODAL_CLOSED 이벤트 핸들러는 항상
scrollObserver.observe()를 호출해 LAST_PAGE_REACHED 이후에도 sentinel 감시를 재개하므로, 마지막 페이지
도달 여부를 추적하는 플래그(예: isLastPageReached) 또는 기존 상수 LAST_PAGE_REACHED 상태를 활용해 이벤트
핸들러에서 재등록을 조건부로 수행하도록 변경하세요; 구체적으로 eventBus.subscribe(APP_EVENTS.MODAL_CLOSED,
...) 내부에서 현재 마지막 페이지 여부를 확인하고 false일 때만 scrollObserver.observe()를 호출하도록 수정하고,
마지막 페이지 상태를 갱신하는 로직(페이지 로드 완료를 처리하는 함수 또는 scrollObserver callback)에
isLastPageReached를 설정하도록 연동하세요.
Comment on lines
+66
to
+69
| eventBus.subscribe(APP_EVENTS.ERROR, (message) => { | ||
| scrollObserver.disconnect(); | ||
| movieListView.clearList(); | ||
| movieListView.showError(message); |
There was a problem hiding this comment.
상세 조회 실패가 메인 목록 에러로 잘못 처리됩니다.
src/features/handler/controllerHandlers.ts의 loadMovieDetail()도 APP_EVENTS.ERROR를 발행하는데, Line 66-69는 항상 목록을 지우고 메인 영역에 에러를 렌더합니다. 그래서 상세 조회 실패 시에는 기존 목록이 사라지고, 사용자는 열린 모달 안에서 로딩 상태만 보게 됩니다. 목록용 에러와 모달용 에러를 분리하는 편이 안전합니다.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/subscriptions.ts` around lines 66 - 69, The current APP_EVENTS.ERROR
handler in eventBus.subscribe(...) indiscriminately clears the list and shows
the error via movieListView, which hides the list when loadMovieDetail()
publishes an error; change the flow so list vs modal errors are distinguishable:
either (a) have loadMovieDetail() publish a distinct event (e.g.,
APP_EVENTS.DETAIL_ERROR) and add a separate subscriber that shows modal-specific
errors without calling movieListView.clearList(), or (b) include a
context/source flag in the error payload (e.g., message.source === 'detail') and
update the APP_EVENTS.ERROR handler to only call scrollObserver.disconnect(),
movieListView.clearList(), and movieListView.showError(message) when
message.source !== 'detail' (for detail errors keep the list intact and let the
detail handler render the modal error). Ensure you update loadMovieDetail() and
the event subscription accordingly.
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) 이번 단계에서 가장 많이 고민했던 문제와 해결 과정에서 배운 점
안녕하세요 루트! 이번 2단계에서도 잘 부탁드립니다.
dm으로도 말씀드렸지만, 이번 단계에서 5일간 최대한 AI를 사용하지 않고 구현해보고 싶었습니다. (E2E, 단위 테스트는 도움을 받았습니다) AI를 사용하면 구현 단계에서 고민하는 시간이 짧아지고, 그러다 보면 제가 구현하려고 했던 아이디어를 정확하게 발현하지 못한다고 느꼈습니다. 동시에 생각의 확장도 어려워진다고 생각했습니다. 한편으로는 멀리 보았을땐 실무에서, 그리고 곧 있을 팀 프로젝트에서는 결국 AI를 적극 활용하는 방향으로 개발하게 될 거라고 생각합니다. 그래서 오히려 지금 이 기회에 한 번이라도 AI 의존을 줄이고, 직접 구조도를 그려보며 설계하고, 에러가 생겼을 때 스스로 디버깅하는 훈련을 해보고 싶었습니다. 그 과정에서 구현의 흐름을 따라가며 시간이 꽤 오래걸리더라도, 특히 "UX"라는 또하나의 중요한 과제에 대해서 오래 고민하게 되었고, 결과적으로 후회 없는 경험이었습니다.
직접 짜는 맛도, 고민하는 맛도 있었습니다. 생각보다 하나의 문제에 정말 깊게, 오래 빠져들게 되었습니다. 그 과정에서 왜 이렇게 동작하는지같은 질문을 끝까지 파고드는 습관이 조금이나마 생긴 것 같습니다. 그렇기에 많이 부족하고, 기준도 명확하지 못한 빈틈이 많이 있을것이라 생각합니다. 지적해주시면 열심히 다시 리팩토링하면서 배워나가겠습니다!
그렇게 그 과정에서 겪었던 트러블 슈팅들과, 궁금한 점(개발 과정에서 고민했던 설계 문제)을 작성하였습니다.
전체 아키텍처
별점 매기기
이외에도 수없이 많은 고민과정이 있었지먼 해당 부분이 이번 미션에서 가장 많이 고민했던 문제였고, 그만큼 가장 많이 배운 부분입니다. 별점 매기기를 구현하기 시작 전까지 다양한 아이디어에 대해 고민했었습니다.
이중에서 단점을 지우고나니, 4번에 가장 가깝게 좁혀지고 있었습니다. 다만 같이 이야기 한 크루가 백그라운드 이미지를 갈아끼울때 깜빡임이 없는지를 생각해 봐야한다고 했을 때, 바로 조금 다른 느낌의 아이디어가 떠올랐습니다. 채워진 별 이미지랑 빈 별 이미지를 하나의 이미지 파일로 합친다음에
background-position으로그냥 분기처리 해버리는식에 대해 떠올랐고, 그렇게 저는 별 이미지는star_empty.png와star_filled.png를 하나의 이미지로 합쳐서background-position만으로 상태를 전환하도록 구현하였습니다.우선 모달이 열리면
StarRating이mount됩니다. 먼저IRatingRepository.load(movieId)로 저장된 별점을 불러오고, 없으면 "평가하기" 텍스트를 표시합니다. 별은 radio input으로 구현했습니다.DOM 순서는 10점→2점(역순)이고,
flex-direction: row-reverse로 화면에 1→5번 별 순서로 표시됩니다. 이 역순 배치는 디버깅과정에서 리팩토링을 한 결과인데요, CSS ~ 형제 선택자가, 이후 형제만 선택 가능한 특성을 활용하기 위해서 구현에 필요한 과정이였습니다.hover시 별이 채워지는 동작은CSS :has()선택자만으로 처리됩니다.hover중에 옆 텍스트는 변경되지 않고,hover를 벗어나면 기존 확정 상태로 돌아오게끔 UX를 개선하였습니다.별을 클릭하면
radio의change이벤트가 발생합니다. 이때IRatingRepository.save(movieId, rating)으로 별점을 저장하고, 텍스트를 해당 점수의 라벨(최악이에요/별로예요/보통이에요/재미있어요/명작이에요)로 변경합니다.별점 UX 개선 과정
처음에는 hover와 텍스트가 묶여 있었습니다.
mouseover이벤트로 별을 채우면서 동시에 텍스트도 바꾸는 방식이었는데, 별점을 매긴 뒤 별 위를 마우스로 왔다갔다 하면 텍스트가 "최악이에요" → "보통이에요" → "명작이에요"로 정신없이 바뀌었습니다. 확정된 점수를 보고 싶은 사용자 입장에서 상당히 혼란스러운 경험이었습니다.이를 해결하기 위해
hover(시각)와click(상태 확정)의 책임을 분리했습니다.hover는CSS만으로 별 채움 상태를 보여주고,click이 발생했을 때만JavaScript가 텍스트를 변경하고 저장합니다. 이후hover를 JS에서 관리하던mouseover/mouseleave핸들러도 제거하고CSS :has()기반으로 완전히 전환했습니다. 사실 추가적으로 텍스트 조차 CSS로 관리를 하고자 시도하였습니다. 현재 텍스트는radio change이벤트에서 JS가 직접innerHTML을 바꿉니다. 두 가지를 CSS만으로 통일하려면 별마다 숨겨진<span>텍스트를 CSScontent로 처리해야 하는데, 몇번 시도를 해보았지만 너무 복잡하다고 판단해서 JS로 분리했습니다.2) 이번 리뷰를 통해 논의하고 싶은 부분: ScrollObserver는 sentinel을 외부에서 주입받아야 할지 궁금합니다
무한스크롤 구현에서 두 가지 방향을 고민했습니다.
A. 외부에서 sentinel을 주입받는 방식 (현재 구현)
이렇게 하면
sentinel이 무엇인지ScrollObserver에서 모르게 됩니다. 어떤HTMLElement든 받아서 감시할 수 있고, 테스트할 때도 가짜element를 넣어줄 수 있습니다. 다만 어차피 사용하는 쪽에서sentinelDOM 요소를 알고 있어야 하고,dom.ts에서 미리 꺼내두는 책임이 생깁니다.B. 클래스 내부에서 DOM을 직접 조회하는 방식
이렇게 하면 사용하는 쪽에서
new InfiniteScroll()한 줄이면 끝이라 굉장히 간결합니다.sentinel이 무엇이고 어떤selector를 쓰는지는 클래스가 알아서 처리합니다. 다만,.scroll-sentinel이라는 특정selector에 결합되는 느낌을 받았습니다.현재는 A 방식을 선택했습니다.
dom.ts에서 정적 DOM 요소를 한곳에서 관리하는 패턴을 이미 쓰고 있어서,sentinel도 같은 흐름으로 주입하는 게 일관성 있다고 판단했습니다. 게다가 ui가 DOM을 알게끔 설계하고 싶지 않았습니다. 다만 B 방식처럼 클래스가 자기 관심사를 캡슐화해서 알고있는 것도 장점이 있다고 생각이 들었습니다.결국 여기서 제가 생각했을때에는 "의미적으로 가까운 것을 함께 두는 것"과 "의존성 주입이라는 설계 기준에 맞추는 것" 사이의 선택이라고 생각했습니다.
sentinel은InfiniteScroll의 관심사이니 내부에 두는 게 자연스럽다는 생각도 들지만, 그 기준으로 가면Modal도document.querySelector(".modal")을 내부에서 하고,MovieList도 내부에서 DOM을 찾는 게 자연스러워집니다. 그러면 결국 모든 컴포넌트가 각자 DOM을 찾게 되고, 어떤 요소를 어디서 조회하는지가 코드 전체에 흩어집니다. 그래서 의미적 직관보다는, DOM 조회는 한곳에서 하고 컴포넌트는 주입받아서 쓴다는 기준을 우선했는데, 이 기준이 적절한 방향인지가 루트의 의견을 알려주시면 부족한 설계 과정에 많은 참고가 될 것 같습니다.✅ 리뷰어 체크 포인트