Open
Conversation
- `BookmarkAdapter`, `BookmarkViewHolder`, `BookmarkClickListener` 등 기존 XML 기반 리사이클러뷰 관련 코드 삭제 - `fragment_library.xml` 및 관련 레이아웃 XML 파일 삭제 - `LibraryScreen`, `BookmarkItem`, `LibraryProfileSection` 등 Compose 기반 UI 컴포넌트 구현 - `LibraryViewModel`의 `LiveData`를 `StateFlow`로 변경하고 상태 관리 로직 개선 - `LibraryFragment`에서 `ComposeView`를 사용하여 UI를 렌더링하도록 변경 및 미디어 컨트롤러 연동 로직 수정
- XML 리소스 기반의 색상(HearitBlack1, HearitGray4 등) 및 폰트(Pretendard) 정의 추가 - `BookmarkItem`, `LibraryProfileSection`, `LibraryHeaderSection` 등에 구체적인 텍스트 스타일(fontSize, fontFamily) 적용 - 컴포넌트 간 간격(Spacer), 패딩, 마진 값을 XML 디자인 가이드에 맞춰 조정 - `LibraryScreen` 배경에 `HearitPurple3`와 `HearitBlack1`을 사용하는 세밀한 수직 그라데이션 적용 - 프로필 및 헤더 섹션의 아이콘 크기 및 틴트 설정 최적화
- `Color.kt`에 XML 기반 공통 컬러 및 시맨틱 컬러 추가 - `BookmarkItem`, `LibraryEmptyViews`, `LibraryScreen` 등 Library 컴포넌트들의 하드코딩된 색상과 폰트를 공통 테마(`HearitBlack1`, `Gray4`, `PretendardFontFamily` 등)로 교체 - 불필요한 주석 제거 및 코드 포맷팅 정리
Walkthrough라이브러리 기능을 RecyclerView 및 XML 기반 UI에서 Jetpack Compose로 마이그레이션합니다. LiveData 상태 관리를 StateFlow로 전환하고, 관련 어댑터, 뷰 홀더, 리스너 클래스를 제거한 후 새로운 Compose 컴포넌트로 대체합니다. Changes
Estimated code review effort🎯 4 (Complex) | ⏱️ ~75 minutes 🚥 Pre-merge checks | ✅ 1 | ❌ 2❌ Failed checks (1 warning, 1 inconclusive)
✅ Passed checks (1 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ 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 Tip You can customize the tone of the review comments and chat replies.Configure the |
![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
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
android/app/src/main/java/com/onair/hearit/presentation/library/LibraryFragment.kt (1)
248-253:⚠️ Potential issue | 🟠 Major
onStop에서onDestroyView로 MediaController 해제 시점 변경현재
onStop에서 MediaController를 해제하고 있으나, 백그라운드 재생 시나리오에서 컨트롤러 연결이 유지되어야 합니다. ScriptFragment 등 다른 Fragment에서는onDestroyView에서 해제하고 있으므로, 동일하게onDestroyView에서 정리하는 것이 적절합니다.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@android/app/src/main/java/com/onair/hearit/presentation/library/LibraryFragment.kt` around lines 248 - 253, 현재 LibraryFragment의 onStop에서 mediaController를 해제하고 있는데 백그라운드 재생을 위해 해제 시점을 onDestroyView로 옮겨야 합니다; mediaController?.removeListener(playerListener), mediaController?.release(), mediaController = null 호출을 onStop에서 제거하고 대신 Fragment의 onDestroyView 오버라이드(또는 기존 onDestroyView가 있다면 그곳)에 동일한 정리 로직을 추가하여 MediaController와 playerListener를 해제하도록 변경하세요.
🧹 Nitpick comments (4)
android/app/src/main/java/com/onair/hearit/presentation/library/component/LibraryHeaderSection.kt (1)
66-74: 접근성을 위한contentDescription국제화 권장하드코딩된 영어 문자열 대신
stringResource를 사용하면 접근성 서비스(TalkBack)에서 올바른 언어로 읽어줄 수 있습니다.♻️ stringResource 사용 제안
Image( - painter = painterResource(id = if (isPlaying) R.drawable.img_pause else R.drawable.img_play), - contentDescription = "Play All", + painter = painterResource(id = if (isPlaying) R.drawable.img_pause else R.drawable.img_play), + contentDescription = stringResource( + id = if (isPlaying) R.string.library_pause_all else R.string.library_play_all + ), modifier = Modifier .size(56.dp) .padding(4.dp) .clickable { onPlayAllClick() }, )🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@android/app/src/main/java/com/onair/hearit/presentation/library/component/LibraryHeaderSection.kt` around lines 66 - 74, Replace the hardcoded contentDescription in the Image composable inside LibraryHeaderSection with localized strings via stringResource; use different resource keys depending on isPlaying (e.g., R.string.play_all vs R.string.pause_all) so TalkBack reads the correct localized phrase, and ensure those string keys are added to strings.xml with appropriate translations; keep the clickable/onPlayAllClick behavior unchanged.android/app/src/main/java/com/onair/hearit/presentation/library/component/LibraryEmptyViews.kt (1)
57-74: 로그인 버튼의 접근성 개선 권장
Box+clickable조합은 동작하지만,Button컴포넌트나semantics { role = Role.Button }추가 시 접근성 서비스에서 버튼으로 인식됩니다.♻️ 접근성 향상을 위한 semantics 추가 제안
+import androidx.compose.ui.semantics.Role +import androidx.compose.ui.semantics.role +import androidx.compose.ui.semantics.semantics ... Box( modifier = Modifier .fillMaxWidth() .height(56.dp) .clip(RoundedCornerShape(8.dp)) .background(PrimaryPurple) - .clickable { onLoginClick() }, + .clickable( + onClick = onLoginClick, + role = Role.Button, + ), contentAlignment = Alignment.Center, )🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@android/app/src/main/java/com/onair/hearit/presentation/library/component/LibraryEmptyViews.kt` around lines 57 - 74, Replace the clickable Box with an accessible button role: either swap the Box for a Button composable or add semantics { role = Role.Button } to the Modifier on the existing Box; ensure the onLoginClick callback is passed to the Button's onClick (or retained on the clickable modifier) and keep the visual styling (height, clip, background, text styles) so accessibility services will announce it as a button (refer to the Box currently using Modifier.clickable and the onLoginClick handler).android/app/src/main/java/com/onair/hearit/presentation/library/component/LibraryProfileSection.kt (1)
86-90:contentDescription국제화 권장
LibraryHeaderSection.kt와 동일하게, 접근성을 위해stringResource를 사용하는 것이 좋습니다.♻️ stringResource 사용 제안
Icon( painter = painterResource(id = R.drawable.ic_setting), - contentDescription = "Setting", + contentDescription = stringResource(id = R.string.all_setting), tint = Color.Unspecified, )🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@android/app/src/main/java/com/onair/hearit/presentation/library/component/LibraryProfileSection.kt` around lines 86 - 90, Replace the hardcoded contentDescription "Setting" in the Icon call inside LibraryProfileSection (the Icon using painterResource(id = R.drawable.ic_setting)) with a localized stringResource lookup (e.g. stringResource(R.string.<setting_label>)); add the corresponding entry to strings.xml (use the same key used elsewhere, e.g. "setting" or match LibraryHeaderSection) so accessibility reads the localized value.android/app/src/main/java/com/onair/hearit/presentation/library/component/BookmarkItem.kt (1)
59-59: 잘못된 colorCode 형식에 대한 예외 처리 고려
Color.parseColor()는 잘못된 hex 문자열이 전달될 경우IllegalArgumentException을 발생시킵니다. 서버 데이터가 항상 유효하다고 보장되지 않는다면, 기본 색상으로 폴백하는 것이 안전합니다.🛡️ 안전한 색상 파싱 제안
Box( modifier = Modifier .size(60.dp) .clip(RoundedCornerShape(8.dp)) - .background(Color(android.graphics.Color.parseColor(bookmark.category.colorCode))), + .background( + runCatching { + Color(android.graphics.Color.parseColor(bookmark.category.colorCode)) + }.getOrDefault(Gray1) + ), contentAlignment = Alignment.Center, )🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@android/app/src/main/java/com/onair/hearit/presentation/library/component/BookmarkItem.kt` at line 59, The background color parsing in BookmarkItem.kt uses Color(android.graphics.Color.parseColor(bookmark.category.colorCode)) which can throw IllegalArgumentException for invalid hex strings; update this by wrapping the parse in a safe helper or try/catch (e.g., a private parseColorOrDefault function) that attempts android.graphics.Color.parseColor(bookmark.category.colorCode) and on failure returns a predefined fallback Color (e.g., Color.Gray or a theme color), then use that safe result in the .background(...) call so invalid server colorCode values don't crash the UI.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In
`@android/app/src/main/java/com/onair/hearit/presentation/library/component/BookmarkItem.kt`:
- Around line 115-120: The progress calculation in BookmarkItem is using
(lastPlayTime ?: 0L) / playTime without converting milliseconds to seconds,
causing ~1000x error; fix it by reusing the existing
calculateProgress(lastPlayTimeMillis: Long?, totalPlayTimeSec: Int) from
Extensions.kt or apply the same conversion (divide lastPlayTimeMillis by 1000
before dividing by playTime), updating the code in BookmarkItem (where progress
is computed) to call calculateProgress(bookmark.lastPlayTime, bookmark.playTime)
or perform the millisecond-to-second conversion so units match.
In
`@android/app/src/main/java/com/onair/hearit/presentation/library/component/LibraryScreen.kt`:
- Around line 45-60: The derived shouldLoadMore currently captures a stale
isLoading because remember { derivedStateOf { ... isLoading ... } } doesn't
track plain Boolean changes; fix it by including isLoading in the remember keys
so the derived state is recreated when loading changes (e.g., change to
remember(isLoading) { derivedStateOf { val lastVisibleItemIndex =
listState.layoutInfo.visibleItemsInfo.lastOrNull()?.index ?: 0 val
totalItemsCount = listState.layoutInfo.totalItemsCount !isLoading &&
totalItemsCount > 0 && lastVisibleItemIndex >= totalItemsCount - 3 } } ),
ensuring LaunchedEffect(shouldLoadMore) and onLoadMore() see the up-to-date
isLoading value.
---
Outside diff comments:
In
`@android/app/src/main/java/com/onair/hearit/presentation/library/LibraryFragment.kt`:
- Around line 248-253: 현재 LibraryFragment의 onStop에서 mediaController를 해제하고 있는데
백그라운드 재생을 위해 해제 시점을 onDestroyView로 옮겨야 합니다;
mediaController?.removeListener(playerListener), mediaController?.release(),
mediaController = null 호출을 onStop에서 제거하고 대신 Fragment의 onDestroyView 오버라이드(또는 기존
onDestroyView가 있다면 그곳)에 동일한 정리 로직을 추가하여 MediaController와 playerListener를 해제하도록
변경하세요.
---
Nitpick comments:
In
`@android/app/src/main/java/com/onair/hearit/presentation/library/component/BookmarkItem.kt`:
- Line 59: The background color parsing in BookmarkItem.kt uses
Color(android.graphics.Color.parseColor(bookmark.category.colorCode)) which can
throw IllegalArgumentException for invalid hex strings; update this by wrapping
the parse in a safe helper or try/catch (e.g., a private parseColorOrDefault
function) that attempts
android.graphics.Color.parseColor(bookmark.category.colorCode) and on failure
returns a predefined fallback Color (e.g., Color.Gray or a theme color), then
use that safe result in the .background(...) call so invalid server colorCode
values don't crash the UI.
In
`@android/app/src/main/java/com/onair/hearit/presentation/library/component/LibraryEmptyViews.kt`:
- Around line 57-74: Replace the clickable Box with an accessible button role:
either swap the Box for a Button composable or add semantics { role =
Role.Button } to the Modifier on the existing Box; ensure the onLoginClick
callback is passed to the Button's onClick (or retained on the clickable
modifier) and keep the visual styling (height, clip, background, text styles) so
accessibility services will announce it as a button (refer to the Box currently
using Modifier.clickable and the onLoginClick handler).
In
`@android/app/src/main/java/com/onair/hearit/presentation/library/component/LibraryHeaderSection.kt`:
- Around line 66-74: Replace the hardcoded contentDescription in the Image
composable inside LibraryHeaderSection with localized strings via
stringResource; use different resource keys depending on isPlaying (e.g.,
R.string.play_all vs R.string.pause_all) so TalkBack reads the correct localized
phrase, and ensure those string keys are added to strings.xml with appropriate
translations; keep the clickable/onPlayAllClick behavior unchanged.
In
`@android/app/src/main/java/com/onair/hearit/presentation/library/component/LibraryProfileSection.kt`:
- Around line 86-90: Replace the hardcoded contentDescription "Setting" in the
Icon call inside LibraryProfileSection (the Icon using painterResource(id =
R.drawable.ic_setting)) with a localized stringResource lookup (e.g.
stringResource(R.string.<setting_label>)); add the corresponding entry to
strings.xml (use the same key used elsewhere, e.g. "setting" or match
LibraryHeaderSection) so accessibility reads the localized value.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 96c3f44b-ed9f-4ad6-92ef-2a4a8f671212
📒 Files selected for processing (16)
android/app/src/main/java/com/onair/hearit/presentation/library/BookmarkAdapter.ktandroid/app/src/main/java/com/onair/hearit/presentation/library/BookmarkClickListener.ktandroid/app/src/main/java/com/onair/hearit/presentation/library/BookmarkOptionBottomSheet.ktandroid/app/src/main/java/com/onair/hearit/presentation/library/BookmarkViewHolder.ktandroid/app/src/main/java/com/onair/hearit/presentation/library/LibraryFragment.ktandroid/app/src/main/java/com/onair/hearit/presentation/library/LibraryViewModel.ktandroid/app/src/main/java/com/onair/hearit/presentation/library/component/BookmarkItem.ktandroid/app/src/main/java/com/onair/hearit/presentation/library/component/LibraryEmptyViews.ktandroid/app/src/main/java/com/onair/hearit/presentation/library/component/LibraryHeaderSection.ktandroid/app/src/main/java/com/onair/hearit/presentation/library/component/LibraryProfileSection.ktandroid/app/src/main/java/com/onair/hearit/presentation/library/component/LibraryScreen.ktandroid/app/src/main/java/com/onair/hearit/presentation/theme/Color.ktandroid/app/src/main/res/layout/fragment_library.xmlandroid/app/src/main/res/layout/item_bookmark.xmlandroid/app/src/main/res/layout/layout_library_when_no_bookmark.xmlandroid/app/src/main/res/layout/layout_library_when_no_login.xml
💤 Files with no reviewable changes (7)
- android/app/src/main/res/layout/layout_library_when_no_bookmark.xml
- android/app/src/main/res/layout/fragment_library.xml
- android/app/src/main/java/com/onair/hearit/presentation/library/BookmarkClickListener.kt
- android/app/src/main/res/layout/layout_library_when_no_login.xml
- android/app/src/main/res/layout/item_bookmark.xml
- android/app/src/main/java/com/onair/hearit/presentation/library/BookmarkViewHolder.kt
- android/app/src/main/java/com/onair/hearit/presentation/library/BookmarkAdapter.kt
Comment on lines
+115
to
+120
| val progress = | ||
| if (bookmark.playTime > 0L) { | ||
| (bookmark.lastPlayTime ?: 0L).toFloat() / bookmark.playTime.toFloat() | ||
| } else { | ||
| 0f | ||
| } |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# lastPlayTime이 사용되는 다른 위치에서 단위를 확인
rg -n "lastPlayTime" --type=kotlin -C3Repository: woowacourse-teams/2025-hEARit
Length of output: 40737
BookmarkItem에서 진행률 계산 시 단위 변환 누락
코드베이스의 다른 위치들(Extensions.kt의 calculateProgress, BindingAdapter.kt)에서는 lastPlayTime(밀리초)을 1000으로 나누어 초 단위로 변환한 후 playTime(초)로 나누고 있습니다. 그러나 현재 BookmarkItem.kt 라인 117의 코드는 이 변환을 수행하지 않아 진행률이 실제보다 약 1000배 크게 계산됩니다.
이미 Extensions.kt에 올바르게 구현된 calculateProgress(lastPlayTimeMillis: Long?, totalPlayTimeSec: Int) 함수가 존재하므로, 이 함수를 재사용하거나 동일한 변환 로직을 적용해야 합니다.
해결 방법
기존 calculateProgress 함수를 사용하거나, 인라인 수정 시:
val progress =
if (bookmark.playTime > 0L) {
- (bookmark.lastPlayTime ?: 0L).toFloat() / bookmark.playTime.toFloat()
+ ((bookmark.lastPlayTime ?: 0L) / 1000f) / bookmark.playTime.toFloat()
} else {
0f
}🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In
`@android/app/src/main/java/com/onair/hearit/presentation/library/component/BookmarkItem.kt`
around lines 115 - 120, The progress calculation in BookmarkItem is using
(lastPlayTime ?: 0L) / playTime without converting milliseconds to seconds,
causing ~1000x error; fix it by reusing the existing
calculateProgress(lastPlayTimeMillis: Long?, totalPlayTimeSec: Int) from
Extensions.kt or apply the same conversion (divide lastPlayTimeMillis by 1000
before dividing by playTime), updating the code in BookmarkItem (where progress
is computed) to call calculateProgress(bookmark.lastPlayTime, bookmark.playTime)
or perform the millisecond-to-second conversion so units match.
Comment on lines
+45
to
+60
| val shouldLoadMore by remember { | ||
| derivedStateOf { | ||
| val lastVisibleItemIndex = | ||
| listState.layoutInfo.visibleItemsInfo | ||
| .lastOrNull() | ||
| ?.index ?: 0 | ||
| val totalItemsCount = listState.layoutInfo.totalItemsCount | ||
| !isLoading && totalItemsCount > 0 && lastVisibleItemIndex >= totalItemsCount - 3 | ||
| } | ||
| } | ||
|
|
||
| LaunchedEffect(shouldLoadMore) { | ||
| if (shouldLoadMore) { | ||
| onLoadMore() | ||
| } | ||
| } |
There was a problem hiding this comment.
🧩 Analysis chain
🌐 Web query:
In Jetpack Compose, if a non-State parameter (e.g., Boolean isLoading) is referenced inside remember { derivedStateOf { ... } } without remember keys, will updates on recomposition be reflected, or can it keep a stale captured value?
💡 Result:
No, updates to a non-State parameter like Boolean isLoading referenced inside remember { derivedStateOf { ... } } without remember keys will NOT be reflected. The derivedStateOf will keep a stale captured value. The remember without keys creates the derivedStateOf object once and reuses it across recompositions. The derivedStateOf calculation lambda captures the value of isLoading at creation time (stale value). It only invalidates/recomputes when State objects read inside the lambda change. Since isLoading is a plain non-State Boolean parameter (not a State delegate like by mutableStateOf), no State is read, so it never invalidates even if the composable recomposes with a new isLoading value. To fix: Use remember(isLoading) { derivedStateOf { ... } } (recreates on isLoading change) or better, remember(isLoading) { isLoading && ... } or just if (isLoading) { ... } directly since no State reads are needed and derivedStateOf adds overhead.
Citations:
- 1: https://stackoverflow.com/questions/76456879/jetpack-compose-derivedstateof-that-depends-on-a-composable-function-paramete
- 2: https://stackoverflow.com/questions/78849205/jetpack-derivedstateof-doesnt-invoke-when-state-change
- 3: https://stackoverflow.com/questions/78451896/derivedstateof-not-responding-help-me-understand-why
- 4: https://medium.com/androiddevelopers/jetpack-compose-when-should-i-use-derivedstateof-63ce7954c11b
- 5: https://composables.com/docs/androidx.compose.runtime/runtime/functions/derivedStateOf
isLoading 캡처가 고정되어 페이지네이션 트리거가 오동작할 수 있습니다.
Line 45-53의 remember { derivedStateOf { ... isLoading ... } }는 isLoading 최신값을 반영하지 못합니다. derivedStateOf는 State 객체 변경에만 반응하고, 순수 Boolean 파라미터 변경은 감지하지 않기 때문에 캡처된 값이 고정됩니다. 결과적으로 Line 56-60에서 onLoadMore()가 중복 호출되어 불필요한 추가 요청이 발생합니다.
수정 제안
- val shouldLoadMore by remember {
+ val shouldLoadMore by remember(listState, isLoading, uiState) {
derivedStateOf {
val lastVisibleItemIndex =
listState.layoutInfo.visibleItemsInfo
.lastOrNull()
?.index ?: 0
val totalItemsCount = listState.layoutInfo.totalItemsCount
- !isLoading && totalItemsCount > 0 && lastVisibleItemIndex >= totalItemsCount - 3
+ uiState is BookmarkUiState.LoggedIn &&
+ !isLoading &&
+ totalItemsCount > 0 &&
+ lastVisibleItemIndex >= totalItemsCount - 3
}
}🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In
`@android/app/src/main/java/com/onair/hearit/presentation/library/component/LibraryScreen.kt`
around lines 45 - 60, The derived shouldLoadMore currently captures a stale
isLoading because remember { derivedStateOf { ... isLoading ... } } doesn't
track plain Boolean changes; fix it by including isLoading in the remember keys
so the derived state is recreated when loading changes (e.g., change to
remember(isLoading) { derivedStateOf { val lastVisibleItemIndex =
listState.layoutInfo.visibleItemsInfo.lastOrNull()?.index ?: 0 val
totalItemsCount = listState.layoutInfo.totalItemsCount !isLoading &&
totalItemsCount > 0 && lastVisibleItemIndex >= totalItemsCount - 3 } } ),
ensuring LaunchedEffect(shouldLoadMore) and onLoadMore() see the up-to-date
isLoading value.
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.
📌 변경 내용 & 이유
📸 스크린샷 (선택)
🧪 테스트 방법 (선택)
📢 논의하고 싶은 내용
🧩 관련 이슈
#859
Summary by CodeRabbit
릴리스 노트
새로운 기능
스타일
리팩토링