feat: 대학 목록 가상화 적용 - #581
Conversation
|
The latest updates on your projects. Learn more about Vercel for GitHub.
2 Skipped Deployments
|
Walkthrough
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Suggested reviewers
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 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: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@apps/university-web/src/components/university/UniversityCards/index.tsx`:
- Around line 36-46: `useEffect` in `UniversityCards` only recalculates
`scrollMargin` on `resize`, so it can miss height changes from wrapped
filters/search chips or async card count updates. Update the measurement logic
around `measureScrollMargin` by adding a `ResizeObserver` on the
header/container element that affects the top offset, while keeping the existing
`window.resize` listener and initial `requestAnimationFrame` measurement as
fallback. Make sure the observer is created and cleaned up in the same effect as
`window.addEventListener("resize", ...)` so `scrollMargin` stays accurate when
the content height changes.
- Around line 50-62: The virtualized list spacing is being applied twice in
UniversityCards because useWindowVirtualizer’s gap is added on top of the item
wrapper padding. Update UniversityCards/index.tsx to use only one source of
spacing: either remove the gap option (and clean up UNIVERSITY_CARD_GAP) while
keeping the pb-2.5 wrapper padding, or remove the wrapper padding and align
estimateSize with the actual item height. Keep getItemKey and scrollMargin
behavior unchanged.
🪄 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: Repository UI
Review profile: CHILL
Plan: Pro
Run ID: 18684a94-79f5-4f7a-b08d-233bf6b24847
📒 Files selected for processing (1)
apps/university-web/src/components/university/UniversityCards/index.tsx
| useEffect(() => { | ||
| measureScrollMargin(); | ||
|
|
||
| const animationFrameId = window.requestAnimationFrame(measureScrollMargin); | ||
| window.addEventListener("resize", measureScrollMargin); | ||
|
|
||
| return () => { | ||
| window.cancelAnimationFrame(animationFrameId); | ||
| window.removeEventListener("resize", measureScrollMargin); | ||
| }; | ||
| }, [measureScrollMargin]); |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟡 Minor
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# UniversityCards를 렌더링하는 부모와 위쪽 UI 구성을 확인
rg -nP -C8 '<UniversityCards' apps/university-web/srcRepository: solid-connection/solid-connect-web
Length of output: 4449
scrollMargin이 동적 높이 변화를 놓치지 않도록 ResizeObserver를 함께 소개해 보아요.
반갑게도 현재 코드는 창 크기 조절(resize) 때만 고도를 재고 있어요. 하지만 검색어나 필터 칩이 줄바꿈되거나 비동기 개수 카드가 들어오면 창 크기는 그대로인데 콘텐츠 높이만 춤을 출 수 있죠. 그럴 때 scrollMargin이 구린 값을 잡으면 스크롤 위치가 살짝 어긋나는 상황이 벌어질 수 있어요.
따라서 아래 두 가지 걸음을 함께 걷는 건 어떨까요?
-
부모 영역의 호흡을 감지하는 망을 치아요
- 학술 목록 상단 컨테이너(검색/필터/개수 UI 포함) 에
ResizeObserver를 얹어두면, 창 크기와 상관없이 높이 변화만으로도 고도 재측정을 자연스럽게 켤 수 있어요.
- 학술 목록 상단 컨테이너(검색/필터/개수 UI 포함) 에
-
정기적인 점검을 유지하면서도 유연하게 대응하아요
- 기존
resize이벤트 리스너는 창 크기 변화에 특화된 보조 수단으로 두어,ResizeObserver가 먼저 변화에 반응하도록 해요.
- 기존
// 예시 구조
const headerRef = useRef<HTMLElement | null>(null);
useEffect(() => {
// 기본 측정
measureScrollMargin();
const animationFrameId = window.requestAnimationFrame(measureScrollMargin);
window.addEventListener("resize", measureScrollMargin);
// 동적 높이 변화 감지
let resizeObserver: ResizeObserver | null = null;
if (headerRef.current) {
resizeObserver = new ResizeObserver(() => {
measureScrollMargin();
});
resizeObserver.observe(headerRef.current);
}
return () => {
window.cancelAnimationFrame(animationFrameId);
window.removeEventListener("resize", measureScrollMargin);
resizeObserver?.disconnect();
};
}, [measureScrollMargin]);🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@apps/university-web/src/components/university/UniversityCards/index.tsx`
around lines 36 - 46, `useEffect` in `UniversityCards` only recalculates
`scrollMargin` on `resize`, so it can miss height changes from wrapped
filters/search chips or async card count updates. Update the measurement logic
around `measureScrollMargin` by adding a `ResizeObserver` on the
header/container element that affects the top offset, while keeping the existing
`window.resize` listener and initial `requestAnimationFrame` measurement as
fallback. Make sure the observer is created and cleaned up in the same effect as
`window.addEventListener("resize", ...)` so `scrollMargin` stays accurate when
the content height changes.
| const virtualizer = useWindowVirtualizer({ | ||
| count: colleges.length, | ||
| estimateSize: () => ESTIMATED_UNIVERSITY_CARD_ROW_HEIGHT, | ||
| gap: UNIVERSITY_CARD_GAP, | ||
| getItemKey, | ||
| overscan: 6, | ||
| scrollMargin, | ||
| initialRect: { | ||
| width: 0, | ||
| height: INITIAL_VIEWPORT_HEIGHT, | ||
| }, | ||
| useFlushSync: false, | ||
| }); |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor
🧩 Analysis chain
🌐 Web query:
In @tanstack/react-virtualv3 useWindowVirtualizer, does thegap option add spacing in addition to the height returned by measureElement (i.e. element padding plus gap), or does gap replace/include item padding?
💡 Result:
In @tanstack/react-virtual v3, the gap option adds spacing in addition to the size of the items [1][2]. It does not replace or include existing padding within your items; rather, it provides a dedicated mechanism to inject consistent spacing between virtualized items without requiring manual margin or padding adjustments [1][2]. Because the library manages absolute positioning of items, traditional CSS margins on items often do not behave as expected [2][3]. The gap option is designed to handle this by increasing the calculated size/offset of the items so that they remain correctly separated [2]. If you are using dynamic measurements (measureElement), the virtualizer accounts for the measured size of the element and adds the gap value to the space allocated for that item in the virtual list's coordinate system [2][3]. In summary: - The gap option acts as an additive spacing value [2]. - It does not automatically consume or replace the internal padding of your rendered components [1][2]. - It is specifically intended to solve the problem of managing space in an absolutely positioned layout where standard margin-based spacing fails [2].
Citations:
- 1: https://tanstack.com/virtual/v3/docs/api/virtualizer
- 2: feat: add gap option to allow you to set the gap between items TanStack/virtual#689
- 3: Prop 'gap' doesn't work with the dynamic implementation TanStack/virtual#793
가상 리스트의 간격이 의도치 않게 두 배로 벌어지고 있어요
TanStack Virtual 공식 문서를 확인한 결과, useWindowVirtualizer의 gap 속성은 아이템의 측정된 높이 (measureElement 반환값, 내부 패딩 포함) 에 추가되는 값이라고 합니다. 현재 코드에서는 아이템 래퍼에 pb-2.5(10px) 패딩이 있고, 가상러에도 gap: 10px을 설정하여 실제 간격이 약 20px 로 나타나고 있습니다.
이 문제를 해결하고 시각적 일관성을 확보하기 위해 아래 변경을 제안드립니다.
-
간격 중복 제거 (권장)
가상러 설정의gap속성을 제거하고, 간격은 기존 아이템 래퍼의pb-2.5패딩 하나로 통일해주세요.UNIVERSITY_CARD_GAP상수는 더 이상 사용되지 않으므로 함께 정리하면 코드가 깔끔해집니다.const virtualizer = useWindowVirtualizer({ count: colleges.length, estimateSize: () => ESTIMATED_UNIVERSITY_CARD_ROW_HEIGHT, - gap: UNIVERSITY_CARD_GAP, getItemKey, overscan: 6, scrollMargin, -
대안:
gap우선 적용
만약 향후 일정한 간격을 유지하기 위해gap속성을 유지하고 싶다면, 아이템 래퍼의pb-2.5클래스를 제거하고estimateSize계산 시 패딩 값을 제외하여 일치시켜주세요. -
위치 안정성 참고
스크롤 마진 (scrollMargin) 측정 로직이 상단 콘텐츠 크기 변화 (필터 등) 를 감지하지 못할 경우 아이템 위치가 어긋날 수 있으니, 추후 유동적인 레이아웃이 추가될 시 고려해주시면 좋겠습니다.
📝 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.
| const virtualizer = useWindowVirtualizer({ | |
| count: colleges.length, | |
| estimateSize: () => ESTIMATED_UNIVERSITY_CARD_ROW_HEIGHT, | |
| gap: UNIVERSITY_CARD_GAP, | |
| getItemKey, | |
| overscan: 6, | |
| scrollMargin, | |
| initialRect: { | |
| width: 0, | |
| height: INITIAL_VIEWPORT_HEIGHT, | |
| }, | |
| useFlushSync: false, | |
| }); | |
| const virtualizer = useWindowVirtualizer({ | |
| count: colleges.length, | |
| estimateSize: () => ESTIMATED_UNIVERSITY_CARD_ROW_HEIGHT, | |
| getItemKey, | |
| overscan: 6, | |
| scrollMargin, | |
| initialRect: { | |
| width: 0, | |
| height: INITIAL_VIEWPORT_HEIGHT, | |
| }, | |
| useFlushSync: false, | |
| }); |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@apps/university-web/src/components/university/UniversityCards/index.tsx`
around lines 50 - 62, The virtualized list spacing is being applied twice in
UniversityCards because useWindowVirtualizer’s gap is added on top of the item
wrapper padding. Update UniversityCards/index.tsx to use only one source of
spacing: either remove the gap option (and clean up UNIVERSITY_CARD_GAP) while
keeping the pb-2.5 wrapper padding, or remove the wrapper padding and align
estimateSize with the actual item height. Keep getItemKey and scrollMargin
behavior unchanged.
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: e6851cef6a
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| height: `${virtualizer.getTotalSize()}px`, | ||
| }} | ||
| > | ||
| {virtualItems.map((virtualItem) => { |
There was a problem hiding this comment.
Because this component is rendered on the statically generated /university/[homeUniversity] pages, switching the render loop from colleges.map to virtualItems.map leaves only the initial viewport of UniversityCard anchors in the pre-rendered/no-JS HTML; the rest of the detail links are created only after client JS runs and the user scrolls. Since sitemap() currently emits only /university, /university/search, and /university/${slug} entries (apps/university-web/src/app/sitemap.ts:18-37), crawlers or no-JS users lose discovery of most detail pages generated by generateStaticParams, so please keep a server/no-JS fallback with all links or add an equivalent discoverable source.
Useful? React with 👍 / 👎.
Web Vitals / Lighthouse 성능 체크측정 조건
SSR HTML 기준
해석
|
|
추가 확인입니다. 위 표의 FCP/Speed Index는 Lighthouse mobile의 simulated throttling 값입니다. 같은 Lighthouse JSON의 trace observed 값 기준으로는 FCP 회귀가 보이지 않습니다.
네트워크 분해상 JS transfer는 622 KiB → 627 KiB로 약 +5 KiB 수준이고, 전체 transfer/HTML/이미지/DOM은 감소했습니다. 그래서 FCP 점수 하락은 TanStack Virtual 라이브러리 무게가 주 원인이라기보다 Lighthouse 시뮬레이션 모델/측정 노이즈 영향으로 보는 게 더 맞아 보입니다. |
FCP/LCP 상세 재측정추가로 Lighthouse 기본 simulate 값만 보지 않고, 실제 trace 기반 1. Lighthouse 기본 simulate vs 실제 trace provided
즉 Lighthouse 기본 2. 직접 브라우저 측정 PerformanceObserverChrome headless + mobile viewport + cache disabled, 5회 median:
실제 throttling 적용, 1.6Mbps/150ms/CPU 4x, 2회 median:
3. 네트워크 변화
JS 증가는 약 +4 KiB 수준이고, 이미지/HTML/DOM은 크게 감소했습니다. 4. 왜 simulate LCP가 13초인가LCP element는 이미지가 아니라 세 번째 카드의 학교명 텍스트( 폰트 요청을 막고 Lighthouse simulate를 다시 돌리면:
결론: 이미지/DOM 최적화 효과는 실제 브라우저 지표와 provided trace에서 개선으로 확인됩니다. 기본 Lighthouse simulate의 13초 LCP는 리스트 이미지가 아니라 2MB webfont가 모바일 throttling 모델에서 크게 잡힌 영향이 큽니다. |
before
|
이후
|
최종 성능 검증 정리PageSpeed Insights 실제 모바일 측정 기준으로 성능 개선을 확인했습니다.
로컬 상세 측정에서도 긴 리스트 병목이 줄어든 것을 확인했습니다.
정리하면, 이번 PR의 목표였던 긴 대학 목록 렌더링 부담은 확실히 줄었습니다. PageSpeed 기준 전체 성능 점수도 70에서 89로 올랐고, 사용자가 체감하기 쉬운 LCP/TBT가 개선됐습니다. 다만 FCP와 Speed Index는 PageSpeed 기준 악화되어 후속 개선 여지가 있습니다. 추가 분석 결과 JS 증가는 약 +4KiB 수준이라 TanStack Virtual 자체보다는, 2MB |
1 participant
관련 이슈
작업 내용
apps/university-web의 대학 목록 카드 렌더링을 TanStack Virtual 기반 window virtualizer로 변경했습니다.useWindowVirtualizer를 사용했습니다.scrollMargin을 측정하고, virtual row 위치 계산에 반영했습니다.getItemKey에 대학 id를 사용해 필터/검색 후에도 row key가 안정적으로 유지되도록 했습니다.role="listitem",aria-posinset,aria-setsize를 부여해 가상화 이후에도 리스트 문맥을 보강했습니다.성능 검증 요약
PageSpeed Insights 실제 측정
https://www.solid-connection.com/university/kyunghee주요 개선 지점은 LCP와 TBT입니다. 긴 대학 목록에서 초기 렌더링해야 하는 카드, 이미지, DOM 수가 줄면서 메인 스레드 점유와 큰 콘텐츠 렌더링 시간이 함께 개선됐습니다. FCP와 Speed Index는 PageSpeed 기준으로 악화되어 보이지만, 전체 성능 점수는 70에서 89로 개선됐습니다.
로컬 상세 측정
HEAD^(가상화 전) vsHEAD(가상화 적용)http://localhost:{3101,3102}/university/inhaUNIVERSITY_TERM_ID=1 NEXT_PUBLIC_UNIVERSITY_TERM_ID=1 pnpm --filter @solid-connect/university-web run build후next startTanStack Virtual 추가로 JS가 약간 늘었지만, 전송량 기준 증가는 약 4KiB 수준입니다. 반면 이미지 요청/전송량, DOM 수, 초기 카드 렌더링 수는 크게 줄었습니다.
FCP/LCP 해석
Lighthouse 기본
simulate값에서는 FCP/Speed Index가 악화된 것처럼 보였지만, 실제 trace 기반provided모드와 Chrome PerformanceObserver 직접 측정에서는 FCP/LCP가 악화되지 않았습니다.Lighthouse simulate에서 13초대 LCP가 나온 원인은 리스트 이미지가 아니라, 약 2MB인
PretendardVariable.woff2가 preload되어 모바일 throttling 모델에서 크게 잡힌 영향으로 확인했습니다. 폰트를 차단하면 simulate LCP도 13초대에서 1.5~2.4초대로 내려갑니다.결론
특이 사항
apps/web홈 화면의 대학 preview 리스트는 이번 범위가 아니어서 수정하지 않았습니다.node: 22.xengine 경고가 출력됩니다.UNIVERSITY_TERM_ID=1 NEXT_PUBLIC_UNIVERSITY_TERM_ID=1은 stage dev API 학기 확인용으로만 사용했습니다.검증
pnpm --filter @solid-connect/university-web run ci:check통과pnpm --filter @solid-connect/university-web run build통과http://localhost:3001/university/inhalistitem11개 렌더 확인listitem19개 렌더 확인listitem11개 렌더 확인listitem0개 확인리뷰 요구사항 (선택)
scrollMargin측정과 row transform 계산이 기존 페이지 스크롤 UX와 잘 맞는지 봐주세요.UniversityCards의 spacing과 충분히 동일하게 유지되는지 봐주세요.