refactor: Detail 렌더링 최적화 by chysis · Pull Request #930 · githru/githru-vscode-ext

chysis · 2025-10-05T18:14:27Z

Related issue

Closes #861

Result

Before

▲ 기존 Detail 컴포넌트 구조

기본적으로 최대 5개의 commit을 보여주고, toggle button을 클릭하면 전체 commit이 렌더링되는 구조였습니다.
전체 commit이 많을 경우 렌더링에 소요되는 시간을 최대한 줄이고자 가상화를 도입하기로 결정했습니다.

After

DetailSummary와 commitNodeList를 react-virtualized의 List로 묶어서 가상화를 적용했습니다.
기존의 커밋 펼치기/접기 toggle button을 제거하고, 아래에 content가 더 있음을 나타내는 scrollable indicator를 추가했습니다.
- 마지막 가상화 아이템이 렌더링되면 floating button이 사라집니다.

다음은 구현한 모습입니다.

_2025_10_06_02_55_21_522.mp4

Work list

가상화 적용

CellMeasurer를 사용하여 각 commit item의 height를 자동 계산
DetailSummary를 첫 번째 아이템으로, 나머지 commit item을 이어 붙여서 하나의 가상화 아이템 리스트 생성

scrollable indicator 추가

밑에 아이템이 남아있는 경우에만 indicator를 띄워야 했고, 여러 가지 시도(스크롤 좌표로 직접 계산, intersection observer 등)를 했습니다.
List 컴포넌트의 onRowsRendered prop을 통해 마지막 행이 렌더링 되면 indicator를 끄도록 하여 오차 없이 구현했습니다.

가상화 관련 로직 훅으로 분리

Discussion

더미 데이터를 활용한 성능 비교 테스트는 별도로 진행할 예정입니다. 다음과 같이 진행하려 하는데, 어떻게 보시는지 궁금합니다.
- 측정 지표: React 커밋 시간(렌더링 비용 관련), 스크롤 중 FPS & Frame drop rate
- 측정 환경: 동일한 화면 크기와 데이터 크기, CPU 4x throttle, 자동 스크롤 스크립트 작성하여 3회 측정 후 중앙값 비교

nxnaxx

LGTM!! 첨부해 주신 사진이랑 동영상 덕분에 이해하기 편했습니다👍🏻
기존 toggle button 대신에 indicator로 바꾸니 추가 커밋들이 잘려 보이지 않아서 훨씬 좋네요. 고생하셨습니다!

chae-dahee

가상화와 UIUX 등 기능적으로 많은 고민이 담기신 PR 같습니다 👍 고생하셨습니다!
테스트 측정 환경, 지표 좋은 것 같습니다! 유의미한 결과가 나오면 너무 좋겠네요ㅎㅎ

chae-dahee · 2025-10-08T14:38:42Z

+export type VirtualizedItem =
+  | {
+      type: "summary";
+      data: ClusterNode["commitNodeList"];
+    }
+  | {
+      type: "commit";
+      data: Commit;
+    };


타입을 따로 추출하신 이유가 궁금합니다! 다른 Detail 파일들에서도 사용이 될 예정인가요?
지금은 hook 파일에서만 사용되는 것 같아, 내부에 정의하는건 어떠신지 여쭤봅니다~

다른 파일에서는 우선 사용 계획이 없습니다! 말씀해주신 것처럼 hook에서만 사용되고 있어서, hook 내부에 함께 두는 것이 자연스러운 것 같아요👍

원활한 작업을 위해 현재 PR은 우선 머지하고 리팩토링 이슈를 따로 등록해서 진행하겠습니다!

ytaek

앗!! 어제 리뷰 커멘트 다 달아놓고 approve를 안 눌러놨었네요! ㅜ.ㅜ
LGGGGTM 입니다!!!

커멘트들 중에서 기능상 이슈가 아닌 Refactoring이 필요한 부분이라면
이슈로 등록하고 다음 PR에서 진행하셔도 좋겠습니다!

ytaek · 2025-10-06T13:35:10Z

+    () =>
+      new CellMeasurerCache({
+        fixedWidth: true,
+        defaultHeight: 120,


요런 설정류의 값은 별도로 분리해도 괜찮겠습니다!

상수화 작업 필요할 것 같습니다. 다음 이슈에서 적용하겠습니다!

ytaek · 2025-10-06T13:51:14Z

 }
+
+export type VirtualizedItem =
+  | {


오 type이 다른 걸 묶는 방법을 고민많이 하셨을것 같군요!!!!

저도 이전에 이렇게 해결했었는데, 혹시 다른 방법이 있는지 gpt로 좀 찾아봤습니다 ㅎㅎ

요런 것도 있고. 재밌네요.

이게 사용하신 방법 같고,

이건 oop스타일인데 ts에는 왠지 구미가 안 땡기는 것 같고

위에꺼를 oop 스타일 말고 함수 스타일(?) 로 한 것 같구요.

그런데 3,4 번은 뭔가 model에다가 필요없는 정보들까지 넣어버리는 느낌이 들기도 하는군요 ㅎㅎ
그냥 궁금해서 한번 찾아봤습니다 ^^;

Discriminated union 방식으로 타입을 정의했고, 이를 기반으로 Type Guard 함수를 사용해서 렌더링하고 있습니다.👍

ytaek · 2025-10-06T13:52:11Z

+  const { cache, virtualizedItems, showScrollIndicator, handleRowsRendered } =
+    useVirtualizedList(commitNodeListInCluster);
+
+  const renderCommitItem = useCallback(


usecallback까지!!! 꼼꼼하시군요 :)

ytaek · 2025-10-06T13:53:10Z

+export type VirtualizedItem =
+  | {
+      type: "summary";
+      data: ClusterNode["commitNodeList"];


어차피 type에 따라 다른 구조를 가질 꺼라면
data라는 변수명 말고, 그냥 바로 clusterNode, commit 이렇게 써도 되지 않을까요? ㅎㅎ

제가 고려했던 부분은 두 가지였습니다.

앞으로 더 많은 타입이 추가될 수 있는지 여부

일관된 패턴 적용

타입 확장을 염두에 두었고, 그럴 경우 공통된 필드명을 가져가는 것이 더 좋겠다고 생각했어요.
하지만 Detail 컴포넌트가 더 이상 확장될 계획이 없다면 개별 필드명을 사용해도 괜찮을 것 같습니다! 😊

이 부분에 대해서 어떻게 생각하시는지 궁금합니다

제가 고려했던 부분은 두 가지였습니다.

앞으로 더 많은 타입이 추가될 수 있는지 여부

일관된 패턴 적용

타입 확장을 염두에 두었고, 그럴 경우 공통된 필드명을 가져가는 것이 더 좋겠다고 생각했어요. 하지만 Detail 컴포넌트가 더 이상 확장될 계획이 없다면 개별 필드명을 사용해도 괜찮을 것 같습니다! 😊

이 부분에 대해서 어떻게 생각하시는지 궁금합니다

@chysis
아 저도 좀 아리까리한 것 같긴한데, 더 많은 타입이 추가 확장 되더라도,

어차피 typeGuard를 하셨기도 하고,

data의 type이 달라서 분기문으로 구분해서 해야 하므로,
변수명이 모호한 data라는 이름 보다는 차라리 type이라도 유추할 수 있는 이름이 좋지 않을까 생각했습니다.

그런데 virtualItem 이긴 하니까, 음 item이라고 단순하게 이름짓는 것도 괜찮을 것 같기도 하구요

지금 생각해보니 data로는 어떤 의미인지 유추하기가 힘든 점이 젤 큰 (?) 이슈였던 것 같네요 😺😺😺

ytaek · 2025-10-06T13:55:38Z

+    [virtualizedItems]
+  );
+
+  const rowRenderer = useCallback(


(사소) Detail component 가 많이 커지는 것 같은데,
VirtualList를 쓰는 경우가 아니라면, 별도의 component로 분리했어도 좋을 부분들이니
이 함수들도 detail component 바깥으로 분리해도 괜찮지 않을까용?

해당 부분 고려해서 다음 작업에서 다뤄보겠습니다 :)

…-render-performance

chae-dahee

LGTM!!! 👍 👍

nxnaxx

LGTM~~!

chysis · 2025-10-26T10:58:45Z

성능 측정 관련 변경 사항

기존 성능 측정 지표 중 스크롤 중 FPS & Frame drop rate가 있었습니다.
하지만 FPS는 이미 렌더링된 요소의 스크롤이 얼마나 부드러운지를 측정하는 지표로, 가상화를 통해 직접적으로 개선한 부분은 아니라 판단했습니다. 가상화는 DOM에 한 번에 로드되는 양을 줄여서 많은 데이터를 효율적으로 렌더링하기 위함이니까요.

자동 스크롤 스크립트를 작성해서 스크롤 중 FPS와 frame drop rate를 측정해보았으나, 거의 차이가 없었습니다.

따라서 가상화를 통해 직접적으로 개선한 부분으로 성능 측정 지표를 수정했습니다.

INP(Interaction to Next Paint) - 사용자 상호작용에 따른 화면 업데이트에 걸리는 시간
Visible DOM - DOM에 한 번에 로드되는 요소의 개수

성능 측정 환경은 다음과 같습니다.

동일한 화면 크기와 데이터 크기(commitItem 500개, 1000개)
CPU throttle 4x slowdown

성능 측정 결과

결과 바로가기 에서 확인하실 수 있습니다.

chysis added 4 commits October 6, 2025 00:15

refactor: react-virtualized 기반으로 DetailSummary와 CommitNodeList 가상화

b2aaa77

chore: 불필요한 indent 제거

ad607fe

refactor: index 기반으로 scrollable indicator 표시 여부를 결정하게 해서 정확도 개선

a38adbc

refactor: 가상화 관련 로직 훅으로 분리

9926391

chysis self-assigned this Oct 5, 2025

chysis requested review from a team as code owners October 5, 2025 18:14

chysis added new feature performance labels Oct 5, 2025

chysis added this to the v0.8.1 milestone Oct 5, 2025

nxnaxx previously approved these changes Oct 8, 2025

View reviewed changes

chae-dahee previously approved these changes Oct 8, 2025

View reviewed changes

ytaek previously approved these changes Oct 8, 2025

View reviewed changes

chae-dahee mentioned this pull request Oct 9, 2025

[fix]: Stem 정보 펼침: CSMNode 클릭 시 위 영역 CSMNode와 겹침 현상 #799

Closed

Merge remote-tracking branch 'upstream/main' into refactor/861-detail…

adf56db

…-render-performance

chysis dismissed stale reviews from ytaek, chae-dahee, and nxnaxx via adf56db October 11, 2025 08:47

chysis mentioned this pull request Oct 11, 2025

[new feature]: Detail 컴포넌트 리팩토링 #978

Open

chae-dahee approved these changes Oct 11, 2025

View reviewed changes

nxnaxx approved these changes Oct 11, 2025

View reviewed changes

chysis merged commit 2fb1333 into githru:main Oct 11, 2025
2 checks passed

Conversation

chysis commented Oct 5, 2025

Related issue

Result

Before

After

Work list

Discussion

Uh oh!

nxnaxx left a comment

Choose a reason for hiding this comment

Uh oh!

chae-dahee left a comment

Choose a reason for hiding this comment

Uh oh!

chae-dahee Oct 8, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

chysis Oct 11, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

ytaek left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

chae-dahee left a comment

Choose a reason for hiding this comment

Uh oh!

nxnaxx left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

chysis commented Oct 26, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

성능 측정 관련 변경 사항

성능 측정 결과

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

chae-dahee Oct 8, 2025 •

edited

Loading

chysis Oct 11, 2025 •

edited

Loading

ytaek left a comment •

edited

Loading

chysis commented Oct 26, 2025 •

edited

Loading